Created attachment 39358 [details] [review]
ephy-ext-rss.patch

Patch the ephy extension build system to use rss extension

Created attachment 39359 [details]
ephy-ext-rss.tar.gz

The tarball of all the needed files, extract in ephy-extension directory,
create a extensions/rss/ dir.

Blam: http://bugzilla.gnome.org/show_bug.cgi?id=171911

Straw: http://bugzilla.gnome.org/show_bug.cgi?id=171913

Liferea:
http://sourceforge.net/tracker/index.php?func=detail&aid=1172135&group_id=87005&atid=581686

About the proposed UI: this only takes the first feed if there are multiple
feeds of the same type. But some sites have more than one; e.g. mozilla.org has
two rss+xml feeds, "Mozilla Announcements" and ""Mozilla Weblogs". Shouldn't we
be able to subscribe to both, independenly?

Comment on attachment 39358 [details] [review]
ephy-ext-rss.patch

I'm working on an improved version to handle multiple feeds

Comment on attachment 39358 [details] [review]
ephy-ext-rss.patch

No changes here, sorry

Created attachment 39769 [details]
ephy-rss.tar.gz + multi-feed

This is the revised version.
It displays a dialog with a list much like the extension manager, that presents
all feeds on the current website, instead of just one feed.

By default all rss feeds are selected and atom are selected only if they are on
a different host than rss ones, this should work on most sites/blogs (try one
of
http://www.beatniksoftware.com/blog/ to see Atom/rss selection and
www.mozilla.org to see eveyrthing selected even if atom+rss (because different
host)

Of course this rule is empirical, but the user can still click manually on the
checkbox, if something's wrong..

First, thanks for the patch!

Now the review:

* The glade UI isn't HIG-compliant; but I can fix that easily.

* Only ever use one newline, not two in a row.

* ephy-rss-extension.h:
Contains a few mis-alignments.

* ephy-rss-extension.c:
static GtkActionEntry action_entries [] =
{
        { "RssInfo",
          NULL,
          N_("_Subscribe to site's news feed"),
          NULL,
          N_("Subscribe to this website's news feed in your favorite news reader"),
          G_CALLBACK (ephy_rss_display_cb) },
};

The menu item name is perhaps a bit long...

/* We got an rss feed from a tab */
static void
ephy_rss_ge_rss_cb (EphyEmbed *embed,
                   const char *type,
                   const char *title,
                   const char *address,
                   EphyWindow *window)

mis-aligned. (More of the same below, omitted.)

static void
ephy_rss_update_statusbar (EphyWindow *window, gboolean show)
{
        GObject *statusbar;
        GtkWidget *evbox;
        GtkWidget *frame;
        
        /* Show / Hide statusbar icon */
        statusbar = G_OBJECT (ephy_window_get_statusbar (window));
        g_return_if_fail (statusbar != NULL);

Should use the WindowData struct instead.

        frame = g_object_get_data (statusbar, STATUSBAR_FRAME_KEY);
        g_return_if_fail (frame != NULL);

        evbox = g_object_get_data (statusbar, STATUSBAR_EVBOX_KEY);
        g_return_if_fail (evbox != NULL);
        
evbox seems unused in this function.

        if(show)
        {
                gtk_widget_show (frame);
        }
        else
        {
                gtk_widget_hide (frame);                
        }

g_object_set (G_OBJECT (frame), "visible", show, NULL);

ephy_rss_update_action (EphyWindow *window,
                   EphyEmbed *embed)
[...]
        tab = ephy_tab_for_embed(embed);

Nit: space before '('. (More of the same below, omitted.)

       /* Disable the menu item when loading the page */
        action = gtk_ui_manager_get_action (GTK_UI_MANAGER
(ephy_window_get_ui_manager (window)),MENU_PATH "/RssInfo");

Store the action in the WindowData struct.

        if(embed != NULL)

Can't be NULL here!

                if(rss_feedlist_length (list) <= 0)
                        show = FALSE;
                else
                        show = TRUE;

show = rss_feedlist_length (list) > 0;

 impl_attach_tab (EphyExtension *extension,
                                 EphyWindow *window,
                                 EphyTab *tab)
[...]
        list = rss_feedlist_new ();
        g_object_set_data (G_OBJECT (embed), FEEDLIST_DATA_KEY, list);

Use g_object_set_data_full, with rss_feedlist_free as destroy notify func.

       /* Notify when a new rss feed is parsed */
        g_signal_connect_after (embed, "ge-location",
                                G_CALLBACK (ephy_rss_ge_location_cb), window);

Maybe use "ge_content_change" instead of location change event here.

impl_detach_tab (EphyExtension *extension,
                                EphyWindow *window,
                                EphyTab *tab)
[...]
        /* We don't want any new rss notif for this tab */
        g_signal_handlers_disconnect_by_func
                (embed, G_CALLBACK (ephy_rss_ge_rss_cb), extension);
        
The handler was connected with signal data 'window' not 'extension'!

        rss_feedlist_free ((FeedList *) g_object_get_data (G_OBJECT (embed),
FEEDLIST_DATA_KEY));
        g_object_set_data (G_OBJECT (embed), FEEDLIST_DATA_KEY, NULL);

Just set the data to NULL, after using g_object_set_data_full above (see comment
on attach_tab).

ephy_rss_create_statusbar_icon (EphyWindow *window)

        filename = g_build_filename (SHARE_DIR, ICON_FILENAME, NULL);
        LOG("Looking up icon at: %s", filename);
        pixbuf = gdk_pixbuf_new_from_file_at_size (filename, w, h, NULL);

Is there no appropriate stock icon? If no, maybe should file a RFE on
gnome-icon-theme :)

        g_object_set_data (G_OBJECT (statusbar), STATUSBAR_FRAME_KEY, frame);
        g_object_set_data (G_OBJECT (statusbar), STATUSBAR_EVBOX_KEY, evbox);

Add the frame and evbox to the WindowData struct instead.

ephy_rss_destroy_statusbar_icon (EphyWindow *window)
[...]
        g_signal_handlers_disconnect_by_func
                (evbox, G_CALLBACK (ephy_rss_statusbar_icon_clicked_cb), window);

Unnecessary, since the evbox is about to be destroyed.
/* Free window data */
static void
ephy_rss_free_window_data (WindowData *data)
{
        g_return_if_fail (data != NULL);

        g_object_unref (data->action_group);
        g_free (data);
}

Just unref the action group after inserting it into the UI manager, and you can
use g_free instead of this custom func.

impl_attach_window (EphyExtension *ext,
                                        EphyWindow *window)
[...]
        gtk_ui_manager_insert_action_group (manager, action_group, -1);

And now g_object_unref (action_group); (see above)

ephy_rss_extension_attach_to_dbus (EphyRssExtension *extension,
[...]
                LOG ("EphyNetMonitorExtension attached to SESSION bus");

Extension name :)

static void
ephy_rss_extension_dbus_finalize (EphyRssExtension *extension)
{
        /* Disconnect dbus */
        EphyShell *shell = ephy_shell_get_default();
        if ( shell != NULL)

Is shell == NULL, ever??

                        g_signal_handlers_disconnect_by_func(extension, 
                                        G_CALLBACK
(ephy_rss_connect_to_session_bus_cb),
                                        bus);
                        g_signal_handlers_disconnect_by_func(extension, 
                                        G_CALLBACK
(ephy_rss_disconnect_from_session_bus_cb),
                                        bus);

(bus, G_CALLBACK(...), extension) instead!

ephy_rss_extension_init (EphyRssExtension *extension)
[...]
        extension->priv->dialog = NULL;
Unnecessary, priv is initialised to 0 already.

        /* Dispose the dialog */
        if (extension->priv->dialog != NULL)
        {
                g_object_unref (extension->priv->dialog);
        }

Remove the weak pointer.


* rss-feedlist.h:

#include <glib-object.h>
#include <glib.h>

First glib, then glib-object.

/* We store the newsfeed associated to an embed in this property */
#define FEEDLIST_DATA_KEY "ephy-rss-extension-feedlist"

//G_BEGIN_DECLS

G_BEGIN_DECLS before the #define; and don't comment out the G_END_DECLS either.

The aligment is very off in this file.


* rss-feedlist.c:

rss_newsfeed_free (NewsFeed *feed)
{
        if(feed != NULL)
        {

"if ("

FeedList *
rss_feedlist_new (void)
{
        return g_new0 (FeedList, 1);
}

No. FeedList is just an alias for GSList*; return NULL here.

FeedList *
rss_feedlist_copy (const FeedList *feedlist)
{
        FeedList *copy = rss_feedlist_new ();
        
        g_slist_foreach (feedlist->list, &rss_feedlist_copy_foreach, copy);
        return copy;
}

FeedList *copy = NULL;
g_slist_foreach (feedlist, rss_feedlist_copy_foreach, &copy);
return copy;

(assuming order doesn't matter; else return g_slist_reverse (copy))

and write rss_feedlist_copy_foreach as:

static void
rss_feedlist_copy_foreach (NewsFeed *feed, GSList **list)
{
	*list = g_slist_prepend (*list, rss_newsfeed_copy (feed));
}

void
rss_feedlist_free (FeedList *feedlist)
{
        if(feedlist != NULL)
        {
                g_slist_foreach (feedlist->list, &rss_feedlist_free_foreach, NULL);
                g_slist_free (feedlist->list);
                g_free (feedlist);
        }
}

Just:
GSList *list = (GSList*) feedlist;
g_slist_foreach (list, (GFunc) rss_newsfeed_free, NULL);
g_slist_free (list);

and kill rss_feedlist_free_foreach.

guint
rss_feedlist_length (FeedList *feedlist)
{
        if(feedlist != NULL)
                return g_slist_length (feedlist->list);

        return 0;
}

just return g_slist_length ((GSList*) feedlist); or better just replace all uses
of rss_feedlist_length() with that expression.

void
rss_feedlist_add (FeedList *feedlist, const gchar *type, const gchar *title,
const gchar *address)
{
        if(feedlist != NULL)
        {
                /* Alloc the new feed structure */
                NewsFeed *feed = rss_newsfeed_new ();
                feed->type = g_strdup(type);
                feed->title = g_strdup(title);
                feed->address = g_strdup(address);
                
                feedlist->list = g_slist_append (feedlist->list, feed);
        }
}

Don't check for feedlist != NULL; NULL is a perfectly fine GSList*.
And 
GSList *list = (GSList *) feedlist; 
[...]
list = g_slist_prepend (list, feed);
AND you must return the list! and in all users, store the returned value!
(that's in 
ephy_rss_ge_rss_cb; you have to store the new list with g_object_set_data_full:

list = (FeedList *) g_object_steal_data (G_OBJECT (embed), FEEDLIST_DATA_KEY);
rss_feedlist_add (list, type, title, address);
g_object_set_data_full (....)

)


GSList *
rss_feedlist_get_list (FeedList *feedlist)
{
        return feedlist->list;
}

Completely unnecessary.

* rss-ui.h: Alignment!

* rss-ui.c: Alignment!

        store = gtk_list_store_new (N_COLUMNS,
                                        G_TYPE_POINTER,
                                    G_TYPE_STRING,
                                    G_TYPE_BOOLEAN,
                                    G_TYPE_STRING);

RSS_TYPE_FEEDLIST instead of G_TYPE_POINTER.

Wow, thanks for this in-depth review !

Here are my comments:
* The glade UI isn't HIG-compliant; but I can fix that easily.
Can you do it, i don't know HIG very well ?

* Only ever use one newline, not two in a row.
* space before (
I removed every occurence i found, some may have slipped through..


* ephy-rss-extension.c:
> The menu item name is perhaps a bit long...
I changed to "Subscribe to news feeds", a better name can be found, i don't
speak english well enough

> ephy_rss_extension_dbus_finalize
I have a problem here: i get:

[ ephy-rss-extension.c ] EphyRssExtension finalising
** (epiphany:26222): CRITICAL **: ephy_shell_get_dbus_service: assertion
`EPHY_IS_SHELL (shell)' failed

here:
/* Disconnect dbus */
EphyShell *shell = ephy_shell_get_default ();
EphyDbus *bus = EPHY_DBUS (ephy_shell_get_dbus_service (shell));
if i remove the shell = NULL test


>ephy_rss_create_statusbar_icon (EphyWindow *window)
>Is there no appropriate stock icon? If no, maybe should file a RFE on
gnome-icon-theme :)

I don't think so, i'll look again..

* rss-feedlist.c:
I was completely lost with that one :)
I think now i understand better the concept of alias in a boxed type, etc

>rss_feedlist_length (FeedList *feedlist)
>just replace all uses of rss_feedlist_length() with that expression.
For encapsulation's sake or at least what is left of it :) i prefer leave it as
a funtion, but i don't know C good-pratcice very well


The other comments are corrected as you suggested

I will attach thu updated sources when i sort out that stock icon thing

Created attachment 39856 [details] [review]
ephy-rss-ext.patch v3

Update patch to ephy-ext autotools

Created attachment 39857 [details]
ephy-rss-ext.tar.gz v3

Updates source package, uses a stock icon, too

Created attachment 45083 [details] [review]
ephy-rss-ext-dbus.patch

Patch against the tarball, fixes the shell == NULL issue at shutdown

Created attachment 45087 [details]
ephy-rss-ext-v3.1.tar.gz

Because i like spamming bugzilla..

New package remove all the dbus cruft, menu item is now "News Feed
Subscription" and the dialog is HIG-compliant (as much as extension-manager).

Checked in on HEAD. Thanks again!

Created attachment 45116 [details] [review]
ephy-rss-copy-dnd-iconfix.patch

Fixes issue with statusbar icon appearing/disappearing randomly
Add a popup context menu on the list of feeds, with "Copy the feed"
Add drag and drop support when you drag an item of the feed list the address
gets dropped

Created attachment 45117 [details]
rss-context-ui.xml

The context xml file to be placed in the extension dir

Reminder: the extension doesn't reload properly, the feeds are lost when
reloading. Check that..

Created attachment 45133 [details] [review]
ephy-rss-copy-dnd-iconfix.patch

cvs diff -u the xml file is still valid

Created attachment 45312 [details] [review]
ephy-rss-dbus-dnd.patch

Removed the use of uimanager to build the context menu, no need for xml anymore