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

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

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

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.



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]