Created attachment 180800 [details] [review]
Reset Networks' list button

Commit branch: http://gitorious.org/glassrose-gnome/empathy/commits/Reset-network-list-button-641861

Review of attachment 180800 [details] [review]:

::: libempathy-gtk/empathy-irc-network-chooser-dialog.c
@@ +49,3 @@
 
+enum {
+	RESPONSE_RESET

Please set an explicit value. 0 is fine as the GTK_RESPONSE_ are all < 0.

@@ +403,3 @@
+      priv->network_manager);
+
+  for (l=networks; l!=NULL; l=g_slist_next(l))

missing space between = and !=

@@ +417,3 @@
+
+      filter_iter = iter_to_filter_iter (self, &iter);
+      select_iter (self, &filter_iter, TRUE);

Do we really want to select it?

@@ +585,3 @@
       GTK_STOCK_EDIT, GTK_RESPONSE_APPLY,
       GTK_STOCK_REMOVE, GTK_RESPONSE_REJECT,
+      "Reset _Networks List", RESPONSE_RESET,

This string should be translatable.

::: libempathy/empathy-irc-network-manager.c
@@ +390,3 @@
+static GSList *
+get_network_list (EmpathyIrcNetworkManager *self,
+    gboolean get_active)

I'd name it get_dropped to stay coherent.

::: libempathy/empathy-irc-network.c
@@ +235,3 @@
+  self->dropped = FALSE;
+
+  g_signal_emit (self, signals[MODIFIED], 0);

We shouldn't fire this sig if self->dropped was already FALSE.

::: libempathy/empathy-irc-network.h
@@ +63,3 @@
                               EmpathyIrcNetworkClass))
 
+void activate_network (EmpathyIrcNetwork *self);

This should be empathy_irc_network_... as that's a public function.

Created attachment 180826 [details] [review]
Reset Networks' list button

> ::: libempathy/empathy-irc-network-manager.c
> @@ +390,3 @@
> +static GSList *
> +get_network_list (EmpathyIrcNetworkManager *self,
> +    gboolean get_active)
> 
> I'd name it get_dropped to stay coherent.
> 

This method is used by both empathy_irc_network_manager_get_networks and empathy_irc_network_manager_get_dropped_networks so I used a generic name.



> ::: libempathy/empathy-irc-network.c
> @@ +235,3 @@
> +  self->dropped = FALSE;
> +
> +  g_signal_emit (self, signals[MODIFIED], 0);
> 
> We shouldn't fire this sig if self->dropped was already FALSE.
> 

I am calling this method (empathy_irc_network_activate) only on networks that are dropped, so I don't see any need to put another check!?

Rest changes made.
Public branch updated.

Review of attachment 180826 [details] [review]:

::: libempathy-gtk/empathy-irc-network-chooser-dialog.c
@@ +403,3 @@
+      priv->network_manager);
+
+  for (l=networks; l != NULL; l = g_slist_next(l))

still missing space between = and g_slist_next (l).

(In reply to comment #3)
> > ::: libempathy/empathy-irc-network.c
> > @@ +235,3 @@
> > +  self->dropped = FALSE;
> > +
> > +  g_signal_emit (self, signals[MODIFIED], 0);
> > 
> > We shouldn't fire this sig if self->dropped was already FALSE.
> > 
> 
> I am calling this method (empathy_irc_network_activate) only on networks that
> are dropped, so I don't see any need to put another check!?

It's to make your code more robust and less dependent of the part calling it.
Another option could be to add a g_return_if_fail (self->dropped); if you prefer to really ensure your existing logic.

robustness is good.
branch updated.

new branch: http://gitorious.org/glassrose-gnome/empathy/commits/Reset-network-list-button-642264

Thanks; merged to 2.34 and master.

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.