Yeah, I suppose we could make sure to update the text widgets, too.

Created attachment 139961 [details] [review]
Patch for the synchronization of text widgets in gnome-network-properties

Adding a callback function "sync_hosts", in order to synchronize all the text widgets in real time. The use of gtk_entry_get_text and gtk_entry_set_text makes it crash.

I downloaded the code from svn://svn.gnome.org/svn:gnome-control-center/trunk
and took a look at trunk/capplets/network/gnome-network-properties.c.

I thought I would write a function called "sync_hosts", that would copy the
value of the GtkEntry "http_host_entry" to the other GtkEntries
("*_host_entry"), if the user ticked the "Use the same proxy for all protocols"
button. This function should be called every time the user types something in
the "http_host_entry", so I decided to use it as a callback for the
"key-release-event".

The fact is it ends up segfaulting whenever a key is pressed in the GtkEntry
"http_host_entry".

According to valgrind, glade_xml_get_widget is responsible for that :

==22600== Process terminating with default action of signal 11 (SIGSEGV)
==22600==  Access not within mapped region at address 0xE10
==22600==    at 0x40B8867: glade_xml_get_widget (in
/usr/lib/libglade-2.0.so.0.0.7)
==22600==    by 0x804DAEB: sync_hosts (gnome-network-properties.c:1079)

GDB also points out that something is wrong with glade_xml_get_widget.

Am I not using gtk_entry_set_text and gtk_entry_get_text correctly ?


PS : my patch (not working !) comes as an attachment in message #2

Thanks for the patch!

(In reply to comment #3)
> Am I not using gtk_entry_set_text and gtk_entry_get_text correctly ?

First off, if you still have glade in your code you're not working against the latest version (which you should do). Glade has been replaced by GtkBuilder.

Then, if you look at the description of the key-release-event signal in the GTK+ docs, the signal handler function signature looks like this:

gboolean            user_function               (GtkWidget   *widget,
                                                 GdkEventKey *event,
                                                 gpointer     user_data)

while yours is

void                sync_hosts                 (gpointer user_data)

That won't work.

Comment on attachment 139961 [details] [review]
Patch for the synchronization of text widgets in gnome-network-properties

diff --git a/capplets/network/gnome-network-properties.c b/capplets/network/gnome-network-properties.c
index d874146..4b740d4 100644
--- a/capplets/network/gnome-network-properties.c
+++ b/capplets/network/gnome-network-properties.c
@@ -79,6 +79,8 @@ enum {
 
 #define GNOMECC_GNP_UI_FILE (GNOMECC_UI_DIR "/gnome-network-properties.ui")
 
+#define NUMBER_OF_PROTOCOLS 4
+
 static GtkWidget *details_dialog = NULL;
 static GSList *ignore_hosts = NULL;
 static GtkTreeModel *model = NULL;
@@ -1107,6 +1109,85 @@ connect_sensitivity_signals (GtkBuilder *builder, GSList *mode_group)
 	}
 }
 
+/* When using the same proxy for all protocols, updates every host_entry
+ * as the user types along */
+gboolean
+sync_hosts (GtkWidget *widget,
+			GdkEventKey *event,
+			gpointer data)
+{
+	GConfClient *client;
+	gboolean same_proxy;
+	const gchar *http_host;
+	GtkBuilder *builder;
+	GtkEntry *current_entry;
+	char *hosts[] = {
+		"http_host_entry", 
+		"secure_host_entry", 
+		"ftp_host_entry", 
+		"socks_host_entry"};
+	int i;
+
+	client = gconf_client_get_default ();
+	same_proxy = gconf_client_get_bool (client, USE_SAME_PROXY_KEY, NULL);
+
+	if (same_proxy)
+	{
+		builder = GTK_BUILDER (data);
+		current_entry = GTK_ENTRY (gtk_builder_get_object (builder, "http_host_entry"));
+
+		http_host = g_strdup (gtk_entry_get_text (current_entry));
+
+		for (i = 1; i < NUMBER_OF_PROTOCOLS; i++)
+		{
+			current_entry = GTK_ENTRY (gtk_builder_get_object (builder, hosts[i]));
+			gtk_entry_set_text (current_entry, http_host);
+		} 
+
+		g_free ( (gpointer) http_host);
+	}
+	
+	return FALSE;
+}
+
+
+
+/* When using the same proxy for all protocols, copies the value of the
+ * http port to the other spinbuttons */
+void
+sync_ports (GtkSpinButton *spinbutton,
+            gpointer data)
+{
+	GConfClient *client;
+	gboolean same_proxy;
+	GtkBuilder *builder;
+	gdouble http_port;
+	GtkSpinButton *current_spin_button;
+	char *ports[] = {
+		"http_port_spinbuton",
+		"secure_port_spinbutton",
+		"ftp_port_spinbutton",
+		"socks_port_spinbutton"};
+	int i;
+
+	client = gconf_client_get_default ();
+	same_proxy = gconf_client_get_bool (client, USE_SAME_PROXY_KEY, NULL);
+
+	if (same_proxy)
+	{
+		builder = GTK_BUILDER (data);
+		current_spin_button = GTK_SPIN_BUTTON (gtk_builder_get_object (builder, "http_port_spinbutton"));
+		http_port = gtk_spin_button_get_value (current_spin_button);
+
+		for (i = 1; i < NUMBER_OF_PROTOCOLS; i++)
+		{
+			current_spin_button = GTK_SPIN_BUTTON (gtk_builder_get_object (builder, ports[i]));
+			gtk_spin_button_set_value (current_spin_button, http_port);
+		}
+	}
+}
+
+
 static void
 setup_dialog (GtkBuilder *builder)
 {
@@ -1256,6 +1337,12 @@ setup_dialog (GtkBuilder *builder)
 			  "activate", G_CALLBACK (cb_add_url), builder);
 	g_signal_connect (gtk_builder_get_object (builder, "button_remove_url"),
 			  "clicked", G_CALLBACK (cb_remove_url), builder);
+
+	/* Making sure every text widget is updated when using the same proxy */
+	g_signal_connect (gtk_builder_get_object (builder, "http_host_entry"),
+				"key-release-event", G_CALLBACK (sync_hosts), builder);
+	g_signal_connect (gtk_builder_get_object (builder, "http_port_spinbutton"),
+				"value-changed", G_CALLBACK (sync_ports), builder);
 }
 
 int

Sorry I double-posted again, I should be able to post and update the patch without double-posting...

Well, I updated my first patch, and it seems to be working.

I hope it's useful.

Hm, not sure what happened, but please attach patches instead of posting them inline. That's going to break most of them.

Anyway, the patch looks pretty good. A few minor issues:

+       char *hosts[] = {
+               "http_host_entry", 
+               "secure_host_entry", 
+               "ftp_host_entry", 
+               "socks_host_entry"};

If you size the array with "hosts[NUMBER_OF_PROTOCOLS]" that will make sure nobody forgets to update it should the number of protocols change. Defensive programming is almost always a good idea. Also, the array should be "const", and preferably declare variables locally at the beginning of the block they are being used in instead of the beginning of the function (especially if you also initialize them already).

+       client = gconf_client_get_default ();

You never unref this. Memory leak!

+               http_host = g_strdup (gtk_entry_get_text (current_entry));

The strdup creates an unnecessary copy. _set_text will already make one.

+               g_free ( (gpointer) http_host);

The cast isn't necessary.

Now, there's one more general issue with your approach. Currently, with each key typed you need to make a trip to GConf to get get the same_proxy value. Now, GConf internally caches stuff so that's not too much of an issue but that's really an implementation detail so if you can avoid it (easily) you should. And in this case you can. Connect to the value-changed signal of the same_proxy checkbox (or its GConfPropertyEditor) and connect/disconnect your callbacks as appropriate so they only get called when same_proxy is TRUE.

Created attachment 140620 [details] [review]
New version of the patch allowing entries in gnome-network-properties to be synchronized when using the same proxy for all protocols.

+               http_host = g_strdup (gtk_entry_get_text (current_entry));

The strdup creates an unnecessary copy. _set_text will already make one.


>>> Actually, I read in the documentation (http://library.gnome.org/devel/gtk/stable/GtkEntry.html#gtk-entry-get-text) that the string returned by gtk_entry_get_text "points to internally allocated storage in the widget and must not be freed, modified or *stored*.", which led me to think I had to use g_strdup, as I saw written in the cb_add_url function. Could you enlighten me ?  

+               g_free ( (gpointer) http_host);

The cast isn't necessary.

>>> When using only g_free (http_host), I'm getting the following warning when compiling :

gnome-network-properties.c: In function ‘sync_hosts’:
gnome-network-properties.c:958: warning: passing argument 1 of ‘g_free’ discards qualifiers from pointer target type
/usr/include/glib-2.0/glib/gmem.h:55: note: expected ‘gpointer’ but argument is of type ‘const gchar *’

I'm still looking for a way not to use that ugly cast without gcc complaining.


Connecting/Disconnecting the callbacks is indeed much more clever than what I did first.

(In reply to comment #8)
> >>> Actually, I read in the documentation (http://library.gnome.org/devel/gtk/stable/GtkEntry.html#gtk-entry-get-text) that the string returned by gtk_entry_get_text "points to internally allocated storage in the widget and must not be freed, modified or *stored*.", which led me to think I had to use g_strdup, as I saw written in the cb_add_url function. Could you enlighten me ?  

+                       gtk_entry_set_text (current_entry, http_host);

gtk_entry_set_text already makes a copy for you so there's no need for another one. If you wanted to store a pointer to the host yourself somewhere, then yes, you'd have to make a copy.

> >>> When using only g_free (http_host), I'm getting the following warning when compiling :
> 
> gnome-network-properties.c: In function ‘sync_hosts’:
> gnome-network-properties.c:958: warning: passing argument 1 of ‘g_free’
> discards qualifiers from pointer target type
> /usr/include/glib-2.0/glib/gmem.h:55: note: expected ‘gpointer’ but
> argument is of type ‘const gchar *’
> 
> I'm still looking for a way not to use that ugly cast without gcc complaining.

You declared host "const" which means the content is not supposed to be modified (or freed). If you just remove the const the warning will go away. If you don't make a copy in the first place you won't need the free at all, of course. ;-)

Created attachment 140740 [details] [review]
Patch for the synchronization of entries when using the same proxy for all protocols in gnome-network-properties, with correct use of gtk_entry_get_text.

Well, the whole thing perfectly works without g_strdup and g_free, as you thought it would.

So here's the new version of the patch. I noticed that the needed callbacks were not connected if the same proxy button was enabled at the start of the application, because they were only connected if the button was toggled. So they're now connected in both cb_use_same_proxy_checkbutton_clicked () and setup_dialog ().

Created attachment 140877 [details] [review]
updated patch

Hm, almost there.

I've made a few small changes to your patch, mostly to get rid of some unneeded global variables (pollution is bad). Now there are still two issues left:

1) You connect to "key-pressed" for the GtkEntry. This means you won't get notified if, e.g., someone pastes text into the widget using the mouse button. You also don't get notified if the setting is changed in GConf directly.

2) You connect to "value-changed" for the SpinButton. This avoids the problem with the GtkEntry but "value-changed" doesn't give you updates as you type so you only snychronize the ports e.g. when the focus leaves the SpinButton.

Both of these issues have a simple (and, since a spin button is also an entry, identical) fix: use a different signal. ;-)

GtkEntry has a "changed" signal that provides what you need. Funnily enough I couldn't find it in the GTK documentation. :-(

Anyway, its signature looks like this:

static void
user_func (GtkEntry *entry, gpointer user_data)

Hope this helps. If it doesn't feel free to ask.

Hello Jens. Excuse my off-topic, but I'm following this bug because I'm interested in what is the best way to resolve this issue :)

I'm wondering why is it necessary that "synchronize_hosts" returns always FALSE or is it just a typo?

Thanks

(In reply to comment #12)
> I'm wondering why is it necessary that "synchronize_hosts" returns always FALSE
> or is it just a typo?

No, it's not a typo. Look at the documentation of the "key-pressed-event": http://library.gnome.org/devel/gtk/stable/GtkWidget.html#GtkWidget-key-press-event

Created attachment 141309 [details] [review]
Updated patch using the "changed" signal.

Using the "changed" signal on the GtkEntry seems to be working perfectly. When the user pastes text using the mouse in the http entry, the others are now updated.

I thought it would be the same for the spinbutton, so I tried connecting to "changed" instead of "value-changed", but it's worst. When using the arrows, all spinbuttons are updated; but when typing directly in the spinbutton :
1) the other spinbuttons are not updated as I type;
2) even when the focus leaves the spinbutton, the values of the other spinbuttons are not updated.

I created a new patch so you can take a look at the code, because I think I need a hint here : I'm amazed that the GtkSpinButton does not react as the GtkEntry, even though a GtkSpinButton is a GtkEntry and they are connected to the same signal.

Thanks in advance.

Hm, I'm not sure why that doesn't work. Matthias, can you give us a hint here?

I also had the same problem while trying to fix that bug today (I didn't know it had already been reported).

I doesn't work with the "changed" signal because the spinbutton is validated and the validation is done if you press Enter or change the focus out of the spinbutton so if you change the port spinbutton and click "close" (keeping the mouse pressed) you'll see that the other fields get changed. Or, for example, if you click the host box again the other spinbuttons get correctly set too.

So I suppose that it is a normal behaviour to avoid validating the spinbutton everytime a character is typed.

*** Bug 600155 has been marked as a duplicate of this bug. ***

Not validating the changes every time is fine with me, but I'd still like to receive signals when the content changes.

Hi, the bug does not seem to have been fixed then? It has 18 comments regarding a possible fix and the status of the bug is still new.

If it's not working or it's too hard to fix, just hide the "Use the same proxy for all protocols" checkbox because for example, it will make people not use Epiphany browser in favor of Firefox or Iceweasel, etc. IMHO Epiphany kicks ass but these little stupid details are really anoying.

I have been using Linux and Gnome for *many* years and it took me a while to figure out, first that the Proxy had to be defined in Gnome (not in Epiphany which BTW should fire-up gnome-network-preferences from the browser preferences - I am creating this suggestion in the respective bug queue) and second, that I could not access gmail because the stupid checkbox is not working (thus https was not using the proxy). After mannually adding the proxy in all input boxes, everything is working fine.

The message here, is that we seem to get lost in a long technical discussion while not acting pragmatically to ease the user experience. I for one, refuse to switch to KDE 4, but many of my friends, sadly have.

Using Debian Lenny 32 Bit (thus probably present in Ubuntu as well).
GNOME gnome-network-preferences 2.22.2.1
GNOME epiphany-browser 2.22.3

Suppose you're right. I committed the version using "changed" for the host and "value-changed" for the port which, I suppose, is also consistent with the way the values are written to GConf, anyway. Thanks, Cyril!

commit eac0e48a958e399ef7dcc16cde7c00112366d53a
Author: Cyril Roelandt <...>
Date:   Thu Dec 10 12:59:31 2009 +0100

    [network] Keep proxies in sync when "use same proxy" is selected
    
    When the proxy host or port is changed after the "use same proxy
    for all protocols" option was selected, make sure we keep all
    proxies in sync. Closes bug #589952.
    
    Signed-off-by: Jens Granseuer <...>


> GNOME gnome-network-preferences 2.22.2.1

Sorry, but this won't ever be fixed for 2.22. That's prehistoric.