Re: [Tracker] tracker-miner-rss 0.4.0 (and related stuffs)

[Michele Tameni <michele amdplanet it>]

On Fri, Mar 12, 2010 at 6:47 PM, Martyn Russell <martyn lanedo com> wrote:
> On 04/03/10 14:43, Roberto Guido wrote:
> >
> 1. Did you intend for miner-rss to be LGPL license? Doesn't make much sense
> to me since it is primarily used for libraries. Perhaps you want GPL here.
> My guess is this is a copy and paste error ;)

yes, a copy and paste error, fixed :)

> 2. tracker-main.c needs to include config.h like we do in all other source
> files. Nothing is used from it yet, but it is something we do in all our
> source files.

Done

> 3. This is incorrect coding style:
>
> + main (int argc, char **argv) {
>
> Please use:
>
> + main (...)
> + {

Done

> 4. This should be tracker-common.h which includes all other headers to avoid
> future incompatibility issues (tracker-rss-reader.c):
>
> + #include <libtracker-common/tracker-sparql-builder.h>
>
> Also, headers should be in order of location, i.e. system --> higher level.
> The "tracker-rss-reader.h" should be the last include generally.

Done

> 5. Comments should have * on each line (noticed for TODO statements)

Done

> 6. Generally we add the private struct after setting vtable properties.
>
> + g_type_class_add_private (object_class, sizeof (TrackerMinerRSSPrivate));

Ok, Done

> 7. These need lining up:
>
> + subjects_removed_cb (DBusGProxy *proxy,
> + Â Â Â Â Â Â Â Â Â Â Âgchar **subjects,
> + Â Â Â Â Â Â Â Â Â Â Âgpointer user_data)

Done

> 8. This should be translated using _("Initializing")
>
> + g_object_set (object, "progress", 0.0, "status", "Initializing", NULL);

Done

> 9. In tracker_miner_rss_init(), I would suggest you do the wrap != NULL
> check all before you create memory for other things. To avoid memory
> allocation where initialisation fails.

Done

> 10. I see you're using:
>
> + double prog;
>
> Please use gdouble there, gdouble is the type we expect for progress.

Done

> 11. The "Fetching" status should be translated.

Done

> 12. I do worry that there is no checking for the result or when the query
> has finished here:
>
> + tracker_miner_execute_update (TRACKER_MINER (miner),
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âtracker_sparql_builder_get_result (sparql),
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂNULL,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂNULL,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂNULL);
>
> It feels a bit unfinished there. Is more work needed here?

Done

> 13. In our coding style, arrays don't have a space between the index and
> name of the array:
>
> + if (g_strcmp0 (values [0], "1") == 0)

Done

> 14. In item_verify_reply_cb(), uri should be freed.

Done

> 15. I notice that tracker_sparql_builder_object_string() is used a lot, but
> we should be using _object_unvalidated() for strings which need utf-8
> checking. If we don't do this, you risk having your process exit() by d-bus.

Ok, Done

> 16. Coding style is incorrect here:
>
> + static const gchar*

Done

> 17. I notice paused and resumed set the progress to 100%, that sounds
> incorrect to me. Progress usually doesn't change in those states.

Fixed

> --
>
> Generally, it looks really good. Just my nazi coding style issues to fix -
> don't worry I am famous for it :)
>
> The only thing I would add is, I think it would be useful to have some sort
> of logging so progress and general debugging can be tracked more easily.

Added

> It might also make sense to have a config option to disable the miner-rss
> (or perhaps do it some other way).

Not sure on how do this...

> I would be happy to merge this branch if you can make all of pvanhoof's
> changes and my changes.

Great!
We should have fixed all the things you and Philip told to us... hope
that now the miner is (almost) ready for hit the repo :)

Michele & Bob