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

[Philip Van Hoof <spam pvanhoof be>]

On Thu, 2010-03-04 at 15:43 +0100, Roberto Guido wrote:
> Still waiting for a review to include tracker-miner-rss in main Tracker's repository


Here's a first quick review. Conclusion: this branch still needs a lot
of restyling. Also don't use object->priv things, use gobject's standard
things for this. Check out, for example, tracker-ontology.c/h how we do
it ourselves.

diff --git a/src/tracker-miner-rss/tracker-main.c b/src/tracker-miner-rss/tracker-main.c
new file mode 100644
index 0000000..921f705
--- /dev/null
+++ b/src/tracker-miner-rss/tracker-main.c
@@ -0,0 +1,34 @@

...

+#include <stdlib.h>
+#include "tracker-miner-rss.h"
+
+int
+main () {
+       TrackerMinerRSS *miner;
+
+       g_type_init ();
+       g_thread_init (NULL);
+       miner = g_object_new (TRACKER_TYPE_MINER_RSS, "name", "RSS", NULL);
+       tracker_miner_start (TRACKER_MINER (miner));
+       g_main_loop_run (g_main_loop_new (NULL, FALSE));


Memory leaks on the g_main_loop_new and the miner


+       exit (0);
+}
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..d15b9b8
--- /dev/null
+++ b/src/tracker-miner-rss/tracker-miner-rss.c
@@ -0,0 +1,471 @@

+
+#define TRACKER_MINER_RSS_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), TRACKER_TYPE_MINER_RSS, 
TrackerMinerRSSPrivate))
+
+struct TrackerMinerRSSPrivate {
+       gboolean        paused;
+       gboolean        stopped;
+
+       FeedsPool       *pool;

Alignment, align now_.. underneath pool (add a space)

Also, don't use int, but gint

+       int             now_fetching;
+};

Alignment:

+static void tracker_miner_rss_started (TrackerMiner *miner);
+static void tracker_miner_rss_stopped (TrackerMiner *miner);
+static void tracker_miner_rss_paused (TrackerMiner *miner);
+static void tracker_miner_rss_resumed (TrackerMiner *miner);
+static void retrieve_and_schedule_feeds (TrackerMinerRSS *miner);
+static void update_updated_interval (TrackerMinerRSS *miner, gchar *uri, time_t *now);
+static void change_status (FeedsPool *pool, FeedChannel *feed, gpointer user_data);
+static void feed_fetched (FeedsPool *pool, FeedChannel *feed, GList *items, gpointer user_data);
+static gchar* get_message_subject (FeedItem *item);
+static const gchar* get_message_url (FeedItem *item);

Align like this:

static void tracker_miner_rss_resumed   (TrackerMiner    *miner);
static void retrieve_and_schedule_feeds (TrackerMinerRSS *miner);


+G_DEFINE_TYPE (TrackerMinerRSS, tracker_miner_rss, TRACKER_TYPE_MINER)
+
+static void
+tracker_miner_rss_finalize (GObject *object)
+{
+       TrackerMinerRSS *rss;
+
+       rss = TRACKER_MINER_RSS (object);

Please don't do this rss->priv stuff, use the standard private stuff of gobject

+       rss->priv->stopped = TRUE;
+       g_object_unref (rss->priv->pool);
+
+       G_OBJECT_CLASS (tracker_miner_rss_parent_class)->finalize (object);
+}
+
+static void
+tracker_miner_rss_class_init (TrackerMinerRSSClass *klass)
+{
+       GObjectClass *object_class = G_OBJECT_CLASS (klass);
+       TrackerMinerClass *miner_class = TRACKER_MINER_CLASS (klass);

When you did this, there's no need to assign a object->priv in init

+       g_type_class_add_private (object_class, sizeof (TrackerMinerRSSPrivate));
 ...
+}



+
+static void
+tracker_miner_rss_init (TrackerMinerRSS *object)
+{
+       DBusGProxy *wrap;


Please don't do this and use the standard private stuff of GObject.

Example in for example tracker-ontology.c/h

+       object->priv = TRACKER_MINER_RSS_GET_PRIVATE (object);


Use priv->now_fetching, not object->priv, no object->priv is allowed, Use the standard private stuff of 
gobject

+       object->priv->now_fetching = 0;
+
+       g_object_set (object, "progress", 0.0, "status", "Initializing", NULL);
...
+}

+static void
+update_updated_interval (TrackerMinerRSS *miner, gchar *uri, time_t *now)
+{
+       TrackerSparqlBuilder *sparql;
+

...

+

Don't make long lines like this:

+       tracker_miner_execute_update (TRACKER_MINER (miner), tracker_sparql_builder_get_result (sparql), 
NULL, NULL, NULL);

Do it like this:

        tracker_miner_execute_update (TRACKER_MINER (miner), 
                                      tracker_sparql_builder_get_result (sparql), 
                                      NULL, 
                                      NULL, 
                                      NULL);


+       g_object_unref (sparql);
+}
+
+static void
+change_status (FeedsPool *pool, FeedChannel *feed, gpointer user_data)
+{
+       int avail;
+       double prog;
+       TrackerMinerRSS *miner;

No miner->priv, use standard gobject private stuff

+       miner = (TrackerMinerRSS*) user_data;
+       avail = feeds_pool_get_listened_num (miner->priv->pool);
+
+       miner->priv->now_fetching++;
+       if (miner->priv->now_fetching > avail)
+               miner->priv->now_fetching = avail;
+
+       prog = (double) miner->priv->now_fetching / (double) avail;
+       g_object_set (miner, "progress", prog, "status", "Fetching...", NULL);
+}


+static void
+item_verify_reply_cb (GObject *source_object, GAsyncResult *res, gpointer data)
+{
+
...

No long lines:

+
+       tracker_miner_execute_update (TRACKER_MINER (miner), tracker_sparql_builder_get_result (sparql), 
NULL, NULL, NULL);
+
+       g_object_unref (sparql);
+       g_free (subject);
+}


+static void
+check_if_save (TrackerMinerRSS *miner, FeedItem *item)
+{
+       FeedChannel *feed;
+       gchar *query;
+       gchar *communication_channel;
+       const gchar *url;
+
+       url = get_message_url (item);
+       feed = feed_item_get_parent (item);
+       communication_channel = (gchar*) g_object_get_data (G_OBJECT (feed), "subject");

No long lines

+       query = g_strdup_printf ("ASK { ?message a mfo:FeedMessage; nie:url \"%s\"; nmo:communicationChannel 
<%s> }",
+                               url, communication_channel);

No long lines

+       tracker_miner_execute_sparql (TRACKER_MINER (miner), query, NULL, item_verify_reply_cb, item);
+       g_free (query);
+}
+
+static void
+feed_fetched (FeedsPool *pool, FeedChannel *feed, GList *items, gpointer user_data)
+{
+       gchar *uri;
+       time_t now;
+       GList *iter;
+       FeedItem *item;
+       TrackerMinerRSS *miner;

Use

miner = TRACKER_MINER_RSS (user_data) for such casts

+       miner = (TrackerMinerRSS*) user_data;

No miner->priv, use standard gobject private stuff

+       miner->priv->now_fetching--;
+       if (miner->priv->now_fetching <= 0) {
+               miner->priv->now_fetching = 0;
+               g_object_set (miner, "progress", 1.0, "status", "Idle", NULL);
+       }
+
+       if (items == NULL)
+               return;
+
+       now = time (NULL);
+       uri = (gchar*) g_object_get_data (G_OBJECT (feed), "subject");
+       update_updated_interval (miner, uri, &now);
+
+       for (iter = items; iter; iter = g_list_next (iter)) {

Cast not needed here in C

+               item = (FeedItem*) iter->data;
+               check_if_save (miner, item);
+       }
+}
+
+static void
+feeds_retrieve_cb (GObject *source_object, GAsyncResult *res, gpointer data)
+{
+       int interval;
+       register int i;
+       gchar **values;
+       GList *channels;
+       const GPtrArray *response;
+       GError *error;
+       TrackerMinerRSS *miner;
+       FeedChannel *chan;
+
+       miner = TRACKER_MINER_RSS (source_object);

No long lines

+       response = tracker_miner_execute_sparql_finish (TRACKER_MINER (source_object), res, &error);
+
+       if (response == NULL) {
+               g_warning ("Unable to retrieve list of feeds: %s\n", error->message);
+               g_error_free (error);
+               return;
+       }
+
+       channels = NULL;
+
+       for (i = 0; i < response->len; i++) {
+               values = (gchar**) g_ptr_array_index (response, i);
+
+               chan = feed_channel_new ();

No long lines

+               g_object_set_data_full (G_OBJECT (chan), "subject", g_strdup (values [2]), g_free);
+               feed_channel_set_source (chan, values [0]);
+
+               /*
+                       How to manage feeds with an update mfo:updateInterval == 0 ?
+                       Here the interval is forced to be at least 1 minute, but perhaps those
+                       elements are to be considered "disabled"
+               */
+               interval = strtoull (values [1], NULL, 10);
+               if (interval <= 0)
+                       interval = 1;
+               feed_channel_set_update_interval (chan, interval);
+
+               channels = g_list_prepend (channels, chan);
+       }
+
+       feeds_pool_listen (miner->priv->pool, channels);
+}
+
+static void
+retrieve_and_schedule_feeds (TrackerMinerRSS *miner)
+{
+       gchar *sparql;

We usually don't do this, as it creates whitespace in the actual string:

+       sparql = g_strdup_printf ("SELECT ?chanUrl ?interval ?chanUrn WHERE             \
+                                  { ?chanUrn a mfo:FeedChannel .                       \
+                                    ?chanUrn mfo:feedSettings ?settings .              \
+                                    ?chanUrn nie:url ?chanUrl .                        \
+                                    ?settings mfo:updateInterval ?interval }");

Anything wrong with this?

        sparql = g_strdup ("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);

So why did you make a g_strdup? Just pass the const string to execute_sparql?

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


Not needed then

+       g_free (sparql);
+}



+static gchar*
+get_message_subject (FeedItem *item)
+{
+       return g_strdup_printf ("rss://%s", feed_item_get_id (item));
+}
+
+static const gchar*
+get_message_url (FeedItem *item)
+{
+       const gchar *url;
+
+       feed_item_get_real_source (item, &url, NULL);
+       if (url == NULL)
+               url = feed_item_get_source (item);
+       return url;
+}
+
+static void
+tracker_miner_rss_started (TrackerMiner *miner)
+{
+       TrackerMinerRSS *rss;
+
+       g_object_set (miner, "progress", 0.0, "status", "Initializing", NULL);
+
+       rss = TRACKER_MINER_RSS (miner);
+       retrieve_and_schedule_feeds (rss);

No miner->priv stuff, use standard gobject private stuff

+       feeds_pool_switch (rss->priv->pool, TRUE);
+
+       g_object_set (miner, "status", "Idle", NULL);
+}
+
+static void
+tracker_miner_rss_stopped (TrackerMiner *miner)
+{
+       TrackerMinerRSS *rss;
+
+       rss = TRACKER_MINER_RSS (miner);

No miner->priv stuff, use standard gobject private stuff

+       feeds_pool_switch (rss->priv->pool, FALSE);
+       g_object_set (miner, "progress", 1.0, "status", "Idle", NULL);
+}
+
+static void
+tracker_miner_rss_paused (TrackerMiner *miner)
+{
+       TrackerMinerRSS *rss;
+
+       rss = TRACKER_MINER_RSS (miner);

No miner->priv stuff, use standard gobject private stuff

+       feeds_pool_switch (rss->priv->pool, FALSE);
+       g_object_set (miner, "progress", 1.0, "status", "Paused", NULL);
+}
+
+static void
+tracker_miner_rss_resumed (TrackerMiner *miner)
+{
+       TrackerMinerRSS *rss;
+
+       rss = TRACKER_MINER_RSS (miner);

No miner->priv stuff, use standard gobject private stuff

+       feeds_pool_switch (rss->priv->pool, TRUE);
+       g_object_set (miner, "progress", 1.0, "status", "Idle", NULL);
+}
diff --git a/src/tracker-miner-rss/tracker-miner-rss.h b/src/tracker-miner-rss/tracker-miner-rss.h
new file mode 100644
index 0000000..dffb206
--- /dev/null
+++ b/src/tracker-miner-rss/tracker-miner-rss.h
@@ -0,0 +1,51 @@
+/*
+ * Copyright (C) 2009/2010, Roberto Guido <madbob users barberaware org>
+ *                          Michele Tameni <michele amdplanet it>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the
+ * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA  02110-1301, USA.
+ */
+
+#ifndef __TRACKER_MINER_RSS_H__
+#define __TRACKER_MINER_RSS_H__
+
+#include <libtracker-miner/tracker-miner.h>
+
+G_BEGIN_DECLS
+
+#define TRACKER_TYPE_MINER_RSS         (tracker_miner_rss_get_type())
+#define TRACKER_MINER_RSS(o)           (G_TYPE_CHECK_INSTANCE_CAST ((o), TRACKER_TYPE_MINER_RSS, 
TrackerMinerRSS))
+#define TRACKER_MINER_RSS_CLASS(c)     (G_TYPE_CHECK_CLASS_CAST ((c), TRACKER_TYPE_MINER_RSS, 
TrackerMinerRSSClass))
+#define TRACKER_IS_MINER_RSS(o)        (G_TYPE_CHECK_INSTANCE_TYPE ((o), TRACKER_TYPE_MINER_RSS))
+#define TRACKER_IS_MINER_RSS_CLASS(c)  (G_TYPE_CHECK_CLASS_TYPE ((c),  TRACKER_TYPE_MINER_RSS))
+#define TRACKER_MINER_RSS_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS ((o), TRACKER_TYPE_MINER_RSS, 
TrackerMinerRSSClass))
+
+typedef struct TrackerMinerRSS        TrackerMinerRSS;
+typedef struct TrackerMinerRSSPrivate TrackerMinerRSSPrivate;
+
+struct TrackerMinerRSS {
+       TrackerMiner parent;

No miner->priv stuff, use standard gobject private stuff

+       TrackerMinerRSSPrivate *priv;
+};
+
+typedef struct {
+       TrackerMinerClass parent;
+} TrackerMinerRSSClass;
+
+GType    tracker_miner_rss_get_type         (void) G_GNUC_CONST;
+
+G_END_DECLS
+
+#endif /* __TRACKER_MINER_RSS_H__ */


>

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