Re: [Tracker] Enhancements to Removable Media Support

["Christensen, Brandon" <Brandon Christensen windriver com>]

Sorry about the large patch, I'll keep that in mind for future
submissions.  

Regarding the database schema change mentioned below:
Tracker was not using an SQLite AUTOINCREMENT column to automatically
generate IDs because there are two Services Tables that require the
Service ID be unique across the two tables.  To enforce this, Tracker
has a function that gets the MAX service ID from both tables, then gets
the MAX of those two numbers and returns that value plus one as the next
ID.  The only problem with this is if the row with the largest ID is
deleted.
To fix this I've made the ID column of the services tables
AUTOINCREMENT.  Then, instead of finding the largest ID for the ID
column in each table, the function now looks up the next autoincrement
value for both tables in the special SQLITE_SEQUENCE table and returns
the max of two numbers plus 1.  Since the SQLITE_SEQUENCE table is not
changed when rows are deleted from the Services tables, the function
will no longer return IDs that have already been used by deleted items.

The suggestion to expose the TrackerConfig object is a good one.  The
additions to the API were made because there were already existing
configuration APIs and we chose to follow this convention.

For the glade changes we added the new configuration options to the
tracker-preferences tool.

This patch was based on the official release tarball for tracker-0.6.94.
The changes have not been merged into the master branch.  Is this a show
stopper for acceptance of the patch?

Thanks for your feedback,
Brandon


-----Original Message-----
From: Martyn Russell [mailto:martyn lanedo com] 
Sent: Monday, June 15, 2009 4:03 AM
To: Christensen, Brandon
Cc: tracker-list gnome org
Subject: Re: [Tracker] Enhancements to Removable Media Support

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