Re: [Tracker] Enhancements to Removable Media Support

[Martyn Russell <martyn lanedo com>]

On 10/06/09 17:34, Christensen, Brandon wrote:
> Please see the attached patch for some enhancements to removable media
> support. The patch applies to tracker-0.6.94. I have also attached a PDF
> with more detail of the changes that are included in the patch.

Hi Brandon,

First of all, let me say, thank you for the patch.
I was unable to open your enhancements PDF, Evince says it is damaged?

The patch is rather large and would have to be split up into smaller 
patches from my initial look at it.

You patch changes a lot of the database schemas too, some of those 
changes are not possible, like setting the Services table ID to be an 
auto incremented number. We do this manually because ID is unique across 
multiple databases (for emails and files).

The extra DBus API there to get config information is also a bit of a 
waste I feel, because we could just put the TrackerConfig object on DBus 
and let people query the object themselves. I don't think adding an API 
here is really the best way forward on that. The other options which 
deal with removable device management aren't necessary and are done 
internally. This sort of thing shouldn't be exposed to users. We have a 
similar API in the indexer which the daemon calls already.

I am quite interested in your filter changes though. This is one area 
which has remained quite untouched since we got involved over a year 
ago. I dare say this is where your performance improvements are (if 
there were some in the enhancements PDF)

I don't like adding config options unless we need them either. It adds 
to the complexity and maintenance overhead of the whole project. So 
adding cache timeouts to the config isn't a good idea IMO. It isn't 
something which is likely to ever really change.

I see you changed the GIO event case statements - currently we are not 
using GIO so that change won't have any effect. I also don't know if 
that's a good change to make, any changes made to the monitor code need 
a LOT of testing - we have quite a complex module there.

The -glue.h files are built automatically, so no need to include those 
in the patch.

A large chunk of this patch is for glade changes. That would have to 
also be a separate patch. What did you change here? If you merely saved 
the files in glade-3 that could be why. We are not using glade-3 files 
yet IIRC.

Just out of interest, is this patch for master? The 0.6 branch has 
changed in a lot of the places you patch since you wrote it I think.

Once again, thank you for your patch, but I really would recommend you 
write smaller patches next time, it lessens the burden on us reviewing 
it and is less likely to bit rot before it gets applied ;)

--
Regards,
Martyn