GNOME Bugzilla – Bug 624421
Port EphyNetMonitor to GDBus
Last modified: 2011-09-04 19:38:41 UTC
Bug to hold patches for such noble task.
Created attachment 165942 [details] [review] Adapt to GtkNotebook signal signature changes Bug #624421
Created attachment 165943 [details] [review] ephy-net-monitor: port to GDBus Bug #624421
(In reply to comment #1) > Created an attachment (id=165942) [details] [review] > Adapt to GtkNotebook signal signature changes > > Bug #624421 Worth saying that epiphany builds with non-master GTK+ with this patch. It fixes the build with gtk+ master without breaking it for older ones.
Comment on attachment 165942 [details] [review] Adapt to GtkNotebook signal signature changes OK.
Comment on attachment 165942 [details] [review] Adapt to GtkNotebook signal signature changes Attachment 165942 [details] pushed as 4098085 - Adapt to GtkNotebook signal signature changes
Created attachment 172080 [details] [review] ephy-embed-single: set_network_status needs to notify Otherwise ephy-window never finds out that the network-status has changed. Bug #624421
Created attachment 172081 [details] [review] ephy-net-monitor: port to GDBus An updated patch, considering the GSettings migration and some minor changes. Bonus documentation of get_net_status. To do: get an initial value of the State property of NM, instead of just defaulting to "true"
*** Bug 646675 has been marked as a duplicate of this bug. ***
Review of attachment 172080 [details] [review]: OK to go!
Review of attachment 172081 [details] [review]: Patch looks fine in general. ::: src/ephy-net-monitor.c @@ +39,2 @@ guint active : 1; + gboolean status; I think NETWORK_UP/NETWORK_DOWN is more clear than having a boolean, to be honest. @@ +104,3 @@ + g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES | + G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START, Indentation a bit off? @@ +204,2 @@ gboolean +ephy_net_monitor_get_net_status (EphyNetMonitor *monitor) This should probably be is_active or something, if it's going to return a boolean.
Created attachment 188851 [details] [review] Apply your comments and also take care of getting a decent initial state from NM service. The point about a proper UI for the different NM states is in another bug, we can do it this cycle I think, shouldn't be too hard. Anyway, what do you think, should we commit this? -- ephy-net-monitor: port to GDBus Also sporting some improvements: - Getting a proper initial state from NM when starting up. - Making sure is_active() always query NM for the current state. - Consider the new NM states: anything equal or better than LAN is considered online. Can be detailed in a further patch with proper UI. Bug #624421
Comment on attachment 172080 [details] [review] ephy-embed-single: set_network_status needs to notify Attachment 172080 [details] pushed as dff30b7 - ephy-embed-single: set_network_status needs to notify
Review of attachment 188851 [details] [review]: ::: src/ephy-net-monitor.c @@ +1,3 @@ /* * Copyright © 2005, 2006 Jean-François Rameau + * Copyright © 2010 Igalia S.L. Wrong year! @@ +36,3 @@ { + NETWORK_DOWN, + NETWORK_UP This seems a bit gratuitous? :) Not that it's important anyway... @@ +251,3 @@ + managed, (priv->status == NETWORK_UP)); + + return (managed || (priv->status == NETWORK_UP)); I don't think this is logically equivalent to what we had before. Before: TRUE if net is not managed OR if it is managed and it's UP. Now: TRUE if it's managed OR if it's UP => it will be TRUE for managed && DOWN, which is wrong. Or it will be FALSE if it's not managed but it happens to be UP, which is also wrong. Is the change intentional? I'm not sure it makes sense, but maybe it just lacks an explanation. How can we test this btw?
Done by Claudio.