Re: [Tracker] REVIEW: Xavier Claessens' "priority" branch

[Xavier Claessens <xclaesse gmail com>]

Le mercredi 19 février 2014 à 02:00 +0100, Carlos Garnacho a écrit :
> Hey,

> So from looking at the code, the implementation looks quite correct, and
> it looks like a positive addition, I just have minor improvements:

\o/

> 0. Nice cleanup to tracker-extract's tracker-config.c, long time
> overdue :)

Thanks. I think it is the right way of doing it. I copied what
tracker-miner-fs does. But Martyn wasn't totally sure in his comment:
  https://bugzilla.gnome.org/show_bug.cgi?id=719802#c26.

Do you confirm it's good?

> 1. tracker_decorator_set_priority_rdf_types() is added, but there's no
> matching GObject property, would be great to get one.

Hm. I don't keep the strv of rdf types, only a GArray of their ids, so a
getter is a bit more complex -but not impossible- to write. I'll do that
if you think it's important.

> 2. In tracker-decorator.c:607:
>    /* FIXME: Can we know the class_name_id ? */
>    element_add (decorator, subject, 0, FALSE);
>
>    Probably the right approach there is accumulating those, and querying
> those subjects similarly to how tracker_decorator_started() does.

Right. If I understand correctly that code path is only hit on testing
purpose. So I just added a quick sync query.

> 3. In tracker-extract-controller.c:41:
>    static void
>    files_miner_status_changed (TrackerExtractController *self,
>                                const gchar              *status)
>
>    If we just care about status equality with "Idle", I'd make this
> function take a boolean, or wrap one taking a boolean, so we don't need
> to make up status strings in some calls.

Ok, done that.

>    Also, that function does if (..) if (...), when those are mutually
> exclusive, would seem clearer with an if (...) else if (...)

Done.

> 4. In tracker-extract-controller.c:190:
>    self->priv->watch_id = g_bus_watch_name_on_connection (conn,
>        "org.freedesktop.Tracker1.Miner.Files",
>        ...)
>
>    The arguments there aren't indented properly

Fixed.

> 5. In tracker-extract-decorator.c:578:
>                       goto out;
>       }
>    out:
>
>    That goto feels a bit pointless :)

I done that in case we ever add something else inbetween. I'm pretty
sure the compiler is smart enough to optimize out a jmp instruction.
Anyway, removed it if you think it's better :)

> And that's all I could spot in that branch, nice work Xavier :)

Thanks!

Branch updated, top 4 commits fixes each of your comments:
 http://cgit.collabora.com/git/user/xclaesse/tracker.git/log/?h=priority

Regards,
Xavier Claessens.