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

[Martyn Russell <martyn lanedo com>]

On 04/03/10 14:43, Roberto Guido wrote:
> Still waiting for a review to include tracker-miner-rss in main
> Tracker's repository (which updated mirror is starving at
> http://gitorious.org/tracker-miner-rss/tracker-miner-rss/trees/miner-rss

Sorry for taking so long to look at this. By the way, Philip's comments 
are all really good and those would need fixing too. OK so here goes :)

--

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 ;)


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.


3. This is incorrect coding style:

+ main (int argc, char **argv) {

Please use:

+ main (...)
+ {


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.


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


6. Generally we add the private struct after setting vtable properties.

+ g_type_class_add_private (object_class, sizeof (TrackerMinerRSSPrivate));


7. These need lining up:

+ subjects_removed_cb (DBusGProxy *proxy,
+                      gchar **subjects,
+                      gpointer user_data)


8. This should be translated using _("Initializing")

+ g_object_set (object, "progress", 0.0, "status", "Initializing", NULL);


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.


10. I see you're using:

+ double prog;

Please use gdouble there, gdouble is the type we expect for progress.


11. The "Fetching" status should be translated.


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?


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)


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


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.


16. Coding style is incorrect here:

+ static const gchar*


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

--

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.

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

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

Thanks,

--
Regards,
Martyn