Re: [Tracker] Request for review of writeback branch

[Martyn Russell <martyn lanedo com>]

On 22/11/09 13:45, Philip Van Hoof wrote:
> Hi guys!
Hi Philip/Carlos,

I started reviewing the code. I have committed some code clean ups and 
listed those here. Warning, some of these are pedantic :)
- Please align function declaration variables

- The copyright is 2009, not 2008 - copy+paste error ;)

- Remember to include config.h

- Put #defines above typedefs where possible.

- Don't use (), use (void) for functions.

- I replaced the s/g_message/g_critical/ made when we can't get the 
D-Bus name, we use a critical because it is critical to the app's 
operation. This is a g_critical() every where else too. So we should 
change all not just one if we change this.
- Don't declare struct members more than once per line (for clarity).

- We should use _new() not _create() function names, that's typically 
used everywhere else.
- Define all #includes in order of their locality. With the exception of 
config.h which is needed first, that means for example, stdlib.h then 
internal libs, then files in the same directory.
- Header includes should be inside the ifndef voodoo.

- I reversed the module list since they are all inserted into the list 
with prepend.
- We use g_dir_open() APIs to get the list of modules. We should really 
use GIO. I cringe when I see the old APIs used - but it is nice and easy 
to use so :)
- We use sync GIO API for querying the content type in 
tracker_writeback_file_update_metadata(). Is this OK or should it be 
async? I think sync should be ok.
- Fixed a memory leak in tracker_writeback_file_update_metadata() if we 
can't get the file info. We were leaking GFile.
- Generally I don't cast g_timeout* functions to GSourceFunc or any 
others because if the GSourceFunc changes then we don't get told by the 
compiler and things just fail. This is a general example, but I tend to 
do the same for all other places.
- Please use func (), not func().

- The module entry functions like writeback_module_create() would be 
better with the tracker_ prefix for namespace reasons. Feels weird 
having a publicly declared function without the tracker prefix. I am not 
sure if we should do this or not though.
- Removed HAVE_EXEMPI from the xmp writeback module. We don't do this in 
the mp3 module for ID3LIB and we simply don't build the module if we 
don't have support instead.
- This is probably a cut and paste issue again, but linking order 
matters for building on Windows. So the .la files we have in Makefile.am 
should basically have the least significant libraries first (i.e. ending 
up with glib towards the bottom of the list). Unless the MinGW compilers 
are fixed now of course :) So if you wonder why I tend to reorder those 
occasionally when I see them in the wrong order, that's why ;)
- I fixed the documentation building for libtracker-miner, was not 
linking with libtracker-common. Seems to work now.
- The DBus xml file has:
  <annotation name="org.freedesktop.DBus.GLib.CSymbol"
              value="tracker_writeback_dbus"/>

  Not sure why we need this, there are no methods described.
  Also shouldn't we put the async annotation in there?

- There is currently no .service file either. So the writeback service 
won't be started unless done manually for now.
- I removed the writeback hashtable ref in tracker-store. It didn't seem 
necessary.
--

So I have done some basic tests to make sure it works end to end, but I 
haven't looked into the miner's code yet. I will reply later today with 
more comments if I have any.
It looks really good though Philip and Carlos. Thanks for getting 
started on this.
I have committed some code clean ups.

--
Regards,
Martyn