Re: [Tracker] tracker-tags-plugin for Nautilus

[Martyn Russell <martyn lanedo com>]

On 08/11/09 13:40, Debarshi Ray wrote:
> Here is an initial feature incomplete version:
> http://rishi.fedorapeople.org/tracker-tags-plugin-0.1.0.tar.gz
>
> The addition and removal of tags should be working, but the list does
> not get updated immediately. You have to reopen the properties dialog
> again. It is not yet possible to attach tags to files.

I see. We need to listen to the notification signal for that. It should 
be available for tags.

See:

  http://live.gnome.org/Tracker/Documentation/SignalsOnChanges

> I have some queries:
> + I could not find an object that has signals indicating the addition,
> removal, etc. of nao:Tags. It seems that there are objects for other
> classes.

See the above. The only thing that is needed here, is the ontology to 
support it.

data/ontologies$ grep -i notify  *nao.ontology
        tracker:notify true .

Looks supported to me.

> + How shall we update the list? Shall we connect to a signal exported
> by Tracker over DBus, or shall we just add the tag label to the list
> when the async operation finishes?

I would always do this on the async response. If you don't and things 
fail, the user sees something which is not correct (a tag added).

> I feel we should use signals so
> that tag manipulations by other applications are also reflected.
> + Shall we allow the plugin to work with multiple files?

Yes we should. I would also not have 2 separate dialogs for add/manage 
tags. You can do this quite easily using the "view" or "manage" dialog 
and a widget underneath to add additional tags. Remember, you don't have 
to have files associated with a tag to create one :)

I would also have some statistics to make things look nicer (like how 
many files belong to each tag). That can come later.

So I have reviewed the branch and fixed some things along the way. Just 
a few comments:

- Please try to stick to the coding style as much as possible. We need 
to publish it somewhere, but it is roughly like this:

  http://developer.imendio.com/codingstyle

- Don't use if (NULL == ...) please use the item you're comparing with 
on the right hand side.

- I did fix some memory leaks

- I cleaned up the code so it is more like the rest of the code base.

- Generally don't use the tracker_ namespace for static functions (it is 
misleading and doesn't so easily if used other places).

- I removed a lot of the const declarations just for simplicity. There 
were also cases were you were returning const gchar * for a newly 
allocated string which never got freed.

All in all, good work! :) thanks for the patch/branch, it is much 
appreciated.

For the rest of the team, I would like to merge this with master at some 
point, does anyone have any objections to that?

--
Regards,
Martyn