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|@libexecd...@]|${libexecdir}|" \ -e "s|@versi...@]|${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|@libexecd...@]|${libexecdir}|" \ + -e "s|@versi...@]|${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 _______________________________________________ tracker-list mailing list tracker-list@gnome.org http://mail.gnome.org/mailman/listinfo/tracker-list