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

[Philip Van Hoof <spam pvanhoof be>]

Hi guys,

I noticed that the nice guys at Itsme have prepared a new miner-rss
branch on their gitorious*

So here is a new review. My conclusion: woah, a lot of improvements!
Thanks a lot. The remaining issues are really small. 

Martyn or JÃrg will probably do a final review after you guys fixed
these small issues, and then we can start talking in 'merging' terms.

* http://gitorious.org/tracker-miner-rss/tracker-miner-rss


I did a git diff master for this after co -b miner-rss origin/miner-rss

Here we go:

Please uses spaces for all indentation in configure.ac

+AC_ARG_ENABLE([miner_rss],
+              AS_HELP_STRING([--enable-miner-rss],
+                            [enable miner rss [[default=auto]]]),,
+             [enable_miner_rss=auto])
+
+if test "x$enable_miner_rss" != "xno" ; then
+   PKG_CHECK_MODULES(LIBGRSS,
+                    [libgrss-0 >= $LIBGRSS_REQUIRED],
+                    [have_libgrss=yes],
+                    [have_libgrss=no])
 
diff --git a/data/Makefile.am b/data/Makefile.am
index 1849a51..c3e682d 100644
--- a/data/Makefile.am
+++ b/data/Makefile.am
@@ -15,17 +15,31 @@ tracker-miner-fs.desktop.in: tracker-miner-fs.desktop.in.in
        @sed -e "s|@libexecdir[ ]|${libexecdir}|"       \
             -e "s|@VERSION[ ]|${VERSION}|" $< > $@
 
+if USING_MINER_RSS
+tracker-miner-rss.desktop.in: tracker-miner-rss.desktop.in.in

I think it's @(SED), not @sed here. Check with abustany, he used this in his miner-web branch too. We better 
all use the same one.

+       @sed -e "s|@libexecdir[ ]|${libexecdir}|"       \
+            -e "s|@VERSION[ ]|${VERSION}|" $< > $@
+endif

 
 EXTRA_DIST = $(desktop_in_files)

Why is one called tracker-miner-feeds.desktop and the other tracker-miner-rss.desktop?

And why is Name and Comment different (ATOM is added to the first)

diff --git a/data/miners/tracker-miner-feeds.desktop.in b/data/miners/tracker-miner-feeds.desktop.in
@@ -0,0 +1,6 @@
+[Desktop Entry]
+Encoding=UTF-8
+_Name=RSS/ATOM Feeds
+_Comment=RSS/ATOM feed miner
+DBusName=org.freedesktop.Tracker1.Miner.RSS
+DBusPath=/org/freedesktop/Tracker1/Miner/RSS
\ No newline at end of file
diff --git a/data/tracker-miner-rss.desktop.in.in b/data/tracker-miner-rss.desktop.in.in
new file mode 100644
index 0000000..e01fc59
--- /dev/null
+++ b/data/tracker-miner-rss.desktop.in.in
@@ -0,0 +1,6 @@
+[Desktop Entry]
+Encoding=UTF-8
+Name=RSS
+Comment=RSS feeds fetcher
+DBusName=org.freedesktop.Tracker1.Miner.RSS
+DBusPath=/org/freedesktop/Tracker1/Miner/RSS


Misalignment of one \ character:

+tracker_miner_rss_SOURCES =                                            \
+       tracker-main.c                                          \
+       tracker-miner-rss.h                                             \
+       tracker-miner-rss.c





diff --git a/src/tracker-miner-rss/tracker-miner-rss.c b/src/tracker-miner-rss/tracker-miner-rss.c
new file mode 100644
index 0000000..c12081f
--- /dev/null
+++ b/src/tracker-miner-rss/tracker-miner-rss.c

+#include "tracker-miner-rss.h"
+
+#include <stdio.h>
+#include <dbus/dbus-glib.h>
+#include <libtracker-miner/tracker-miner.h>
+#include <libtracker-common/tracker-sparql-builder.h>

Move libgrss.h include above Tracker's includes:

+#include <libgrss.h>

Much better :)

+#define GET_PRIV(obj) (G_TYPE_INSTANCE_GET_PRIVATE ((obj), TRACKER_TYPE_MINER_RSS, TrackerMinerRSSPrivate))



+static void
+change_status (FeedsPool *pool,
+               FeedChannel *feed,
+               gpointer user_data)
+{
+       gint avail;
+       double prog;
+       TrackerMinerRSS *miner;
+       TrackerMinerRSSPrivate *priv;
+
+       miner = TRACKER_MINER_RSS (user_data);
+       priv = GET_PRIV (miner);
+       avail = feeds_pool_get_listened_num (priv->pool);
+
+       priv->now_fetching++;
+       if (priv->now_fetching > avail)
+               priv->now_fetching = avail;


Perhaps add some brackets here:
Else it's not clear why and how you are casting this

+       prog = (double) priv->now_fetching / (double) avail;

        prog = ((double) priv->now_fetching) / ((double) avail);


+       g_object_set (miner, "progress", prog, "status", "Fetching...", NULL);
+}


Call data "user_data" (just for consistency)

+static void
+item_verify_reply_cb (GObject *source_object,
+                      GAsyncResult *res,
+                      gpointer data)
+{
+       time_t t;
+       gchar *uri;
+       gchar *subject;
+       gchar **values;
+       gdouble latitude;
+       gdouble longitude;
+       const gchar *tmp_string;
+       const GPtrArray *response;
+       GError *error;
+       TrackerSparqlBuilder *sparql;
+       FeedItem *item;
+       FeedChannel *feed;
+       TrackerMinerRSS *miner;
+       gboolean has_geopoint;
+
+       miner = TRACKER_MINER_RSS (source_object);
+       response = tracker_miner_execute_sparql_finish (TRACKER_MINER (source_object),
+                                                       res,
+                                                       &error);

Check for error, not for response here (or for both):

+       if (response == NULL) {
+               g_warning ("Unable to verify item: %s\n", error->message);
+               g_error_free (error);
+               return;
+       }
+
+       values = g_ptr_array_index (response, 0);

Use g_strcmp or g_strcmp0 here:

+       if (strcmp (values [0], "1") == 0)
+               return;

So then (above) rename data to user_data here:

+       item = data;




+static void
+retrieve_and_schedule_feeds (TrackerMinerRSS *miner)
+{
+       const gchar *sparql;

This is also fine, yes.

+       sparql = "SELECT ?chanUrl ?interval ?chanUrn WHERE "
+                "{ ?chanUrn a mfo:FeedChannel . "
+                "?chanUrn mfo:feedSettings ?settings . "
+                "?chanUrn nie:url ?chanUrl . "
+                "?settings mfo:updateInterval ?interval }";
+
+       tracker_miner_execute_sparql (TRACKER_MINER (miner),
+                                     sparql,
+                                     NULL,
+                                     feeds_retrieve_cb,
+                                     NULL);
+
+}


+static gchar*
+get_message_subject (FeedItem *item)
+{

Why aren't you letting feed_item_get_id return rss:// in front? And then just avoid all the string copying 
for this. Just an idea.

+       return g_strdup_printf ("rss://%s", feed_item_get_id (item));
+}



+typedef struct _TrackerMinerRSS TrackerMinerRSS;
+typedef struct _TrackerMinerRSSClass TrackerMinerRSSClass;

Nicely opaque :)

+struct _TrackerMinerRSS {
+       TrackerMiner parent;
+};
+
+struct _TrackerMinerRSSClass {
+       TrackerMinerClass parent;
+};
+



-- 
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://pvanhoof.be/blog
http://codeminded.be