GNOME Bugzilla – Bug 601280
Soup Fly improvement to handle state history and connection states.
Last modified: 2013-05-27 16:11:08 UTC
Created attachment 147286 [details] [review] Soup Fly imporvement This patch makes Soup Fly handle the state of the connections and also the history of the previous states each message or connection has been. The new Soup Fly window is divided in two areas, in the superior area two tables are avaliable, one of them shows the messages and the other one shows the connections. Once an element is selected, a table is shown in the inferior area which shows each state the connection or message has been in, and at which time the state was reached. This patch needs the one proposed in https://bugzilla.gnome.org/show_bug.cgi?id=598163#c7 to be commited to work.
Comment on attachment 147286 [details] [review] Soup Fly imporvement The patch is now being redone.
Created attachment 147892 [details] [review] Soup Fly improvement This patch is based on the previous one, but it will work once the patch proposed in https://bugzilla.gnome.org/show_bug.cgi?id=598163#c10 is commited.
Review of attachment 147892 [details] [review]: Just update the style fixes and let's get it in. Even better if you have an updated version. ::: extensions/soup-fly/soup-fly.c @@ +27,2 @@ #include <glib/gi18n-lib.h> +#include <glib/gprintf.h> Isn't glib.h enough? @@ +268,3 @@ +*/ +static gchar +*message_get_uri (FlyMessageData *data, gint state) Style nitpick, the * goes in the first line. @@ +274,3 @@ + if (data->uri_entries) { + if (state < 0) + entry = g_list_last (data->uri_entries); Style: include the { } for the if. You are doing so for the if just after this else. @@ +334,3 @@ + ((old_state == CONNECTION_STATE_TUNNELING) && (state == CONNECTION_STATE_INUSE)) ) + return; + if (state == old_state) return; Mmm, this looks a bit messy, can't you do it only in one line?, anyway try to put blank lines after the variables declared so it's obvious this can return here. @@ +480,3 @@ + if (state == CONNECTION_STATE_CLEANEDUP) { + int connection_number; + FlyConnectionData *con_data; Put a blank line after the variables, nitpick :) @@ +485,3 @@ + -1); + if ( (con_data = g_hash_table_lookup (priv->connection_data, &connection_number)) ) + free_connection_data(con_data, logger); In general try to add a blank line after an if() without {}, so it's obvious what was the line executed by the condition. Also don't put the extra space between the parenthesis in that if ( (. @@ +710,3 @@ + G_TYPE_INT, + G_TYPE_INT, + G_TYPE_STRING) ); That's GTK_TREE_MODEL (gtk_..., note the space after MODEL and not after the (. @@ +760,2 @@ + column = gtk_tree_view_column_new (); + gtk_tree_view_column_set_title (column, _("C. #")); Can you add a comment for translators?
Created attachment 152859 [details] [review] Soup Fly improvement This patch solves the problems commented by Diego Escalante in c3.
> This patch solves the problems commented by Diego Escalante in c3. I forgot to mention that this version of the patch also adds two labels at the top of the connection list and the message list.
Review of attachment 152859 [details] [review]: I got this warning José: (epiphany:14226): Gtk-WARNING **: gtk_scrolled_window_add(): cannot add non scrollable widget use gtk_scrolled_window_add_with_viewport() instead Also try attaching with git format-patch -1 <commit id> so it can be applied with git bz apply or git am. Overall, just style fixes to apply, watch those spaces after/before parenthesis. It's if (blah) instead of if ( blah ). Btw can you extract http headers from Soup? It would be a bit too much for this extension maybe but it would be useful for some people surely. We had livehttpheaders for mozilla. ::: extensions/soup-fly/soup-fly.c @@ +685,1 @@ + if ( !SOUP_IS_ADDRESS(host) ) No space in the parenthesis: if (!SOUP...) @@ +892,3 @@ +static void +generate_containers_and_buttons (SoupFly *logger) +{ Can't you use GtkBuilder for this? It seems it's a lot of code that could be more suited for a .ui file made in glade. @@ +1175,3 @@ + GObject *obj; + g_return_if_fail ( (obj = G_OBJECT (connection)) ); + g_return_if_fail ( !g_object_get_data (obj, "FlyConnectionData") ); No space after parenthesis: g_return_if_fail ((obj.... )); @@ +1229,3 @@ + g_object_get (conn, + "message", &msg, + NULL); If it fits one line, just go ahead.
Created attachment 157990 [details] [review] Soup Fly improvements New version of the patch, which uses GtkBuilder and solves the style problems appointed in comment 6.
just played around with this and it looks good, but i saw g_warnings about g_object_weak_unref() on the console, so there's something not quite right
Comment on attachment 157990 [details] [review] Soup Fly improvements Maybe since we've gone 2 years without committing this we don't actually need it, but if it's going to land now, it should be redone to use the SoupMessage::network-event signal rather than SoupSession::connection-created and SoupSession::tunneling.
According to its developer, epiphany-extensions is not under active development anymore. (For reference: https://mail.gnome.org/archives/gnome-i18n/2013-May/msg00035.html and bug 700924.) It is unlikely that there will be any further active development. Closing this report as WONTFIX as part of Bugzilla Housekeeping - Please feel free to reopen this bug report in the future if anyone takes the responsibility for active development again.