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

This is a bug I would very much like to fixed as well. Maybe even letting the user define a network proxy for a special network type/essid, so that the proxy would change automatically. 

My particular use case is:

I hardly ever use wired network, only when I am at my university, and they use a network proxy. If network manager would know that I wanted my proxy settings 'on' when I was plugged in with a wire, and not when I was using wireless, it would make my day quite a bit easier... Thanks

At a minimum, as an interim solution, if the hostname of the proxy does not resolve or cannot be contacted, then the system should cascade to "direct connection to the Internet" and try.

I have a proxies for work, both "manual" and "automatic".  When I am out of work, the servers does not resolve.  UpdateManager (Ubuntu 08.04) fails to connect, showing me an error message that the current proxy server does not resolve.  I would rather see the tool try a direct Internet connection before complaining that there is a problem with the proxy.  However, if direct Internet connection also fail, the error message should show that proxy resolution failed, to keep the user well informed.

Currently, I need to change manually the network setting each time I go to work or go outside of work.

Yes... making the it go back to direct connection if there proxy is not responding could be a nice way to solve this. And of course notifying the user

Created attachment 115496 [details]
Proxy settings panel on Mac OS X (10.3)

As an implementation example, on Mac OS X, proxy settings are configurable by physical interface. (A set of all interfaces' settings can be saved as a location like in GNOME.)

Sounds like a nice project for gnome-love.

Created attachment 116276 [details] [review]
Add locations

Thanks for your patch!

Some comments:

In general it helps to review your changes if you also explain a bit what you're trying to do, what solution you have chosen, and maybe why you have chosen it if you've also considered other alternatives. Otherwise the reviewer will have to make assumptions and spend a lot more time on code that might not be entirely obvious at first glance. Also post a screenshot of what it looks like. That will make it easier to comment for people who either can't code, or if the patch doesn't compile (like in this case ;-)).

What did you use for editing the glade file? Replacing the entire file very often results in more bugs (re)added than fixed so if possible, reducing the amount of changes in the file to a minimum would be much preferable.

+#define LOCATION_LIST_KEY "/apps/control-center/network/location_list"

You can get all gconf directories in a directory using gconf_client_all_dirs so this key shouldn't be needed.

+#define load_string_to_key(key) if (iter==NULL) return;\
+				gconf_client_set_string(client,key,iter->data,NULL);\
+				iter=g_slist_next(iter);
+#define load_int_to_key(key)	if (iter==NULL) return;\
+				sscanf(iter->data,"%d",&t);\
+				gconf_client_set_int(client,key,t,NULL);\
+				iter=g_slist_next(iter);
+#define load_bool_to_key(key)	if (iter==NULL) return;\
+				if (*(gchar*)iter->data=='0') gconf_client_set_bool(client,key,FALSE,NULL);\
+				else gconf_client_set_bool(client,key,TRUE,NULL);\
+				iter=g_slist_next(iter);

Those define games are pretty ugly. Having that code inline would make the functions in questions much more readable and understandable.

+void load_location(GConfClient *client,gchar* from)
+{
+	int t;
+	GSList *val=gconf_client_get_list(client,from,GCONF_VALUE_STRING,NULL),*iter;

This is with respect to all code generally: Please use this coding style instead:

static void
load_location (GConfClient *client, const gchar *from)
{
	int t;
	GSList *val = gconf_client_get_list (client, from, GCONF_VALUE_STRING, NULL);
	GSList *iter;

You can put multiple variable declarations on one line, but it helps readability if you only do that when you don't initialize them at the same time or only use short and simple assignments like = NULL or = 1.

It also helps to include empty lines to show where logical blocks start or end.

+	GSList *val=gconf_client_get_list(client,from,GCONF_VALUE_STRING,NULL),*iter;
+	iter=val;
+	load_string_to_key(PROXY_AUTOCONFIG_URL_KEY);
+	load_int_to_key(SOCKS_PROXY_PORT_KEY);
...

I think it would be a lot cleaner to not do this generically and make it explicit instead like this:

  key = g_strconcat (from, "/autoconfig_url", NULL);
  s = gconf_client_get_string (client, key, NULL);
  g_free (key);
  if (s != NULL) {
    gconf_client_set_string (client, PROXY_AUTOCONFIG_URL_KEY, s, NULL);
    g_free (s);
  } else {
    gconf_client_unset (client, PROXY_AUTOCONFIG_URL_KEY, NULL);
  }
  ...

+	load_bool_to_key(USE_PROXY_KEY);
+	GSList *ignore=NULL;

Do not declare variables in the middle of a code block. This isn't supported by all compilers.

+	gconf_client_notify(client,IGNORE_HOSTS_KEY);

Why is this necessary?

+	for (a=list;a!=NULL;a=g_slist_next(a))
+		g_free(a->data);

This should be
g_slist_for_each (list, (GFunc) g_free, NULL);

+	if (gconf_key_check(str)) ...

This function doesn't seem to exist,

+	//Locations

Don't use C++ style comments. They aren't compatible with some compilers.

Thanks for your review!

For editing glade file I used Glade Interface Designer 3.4.5
I think 
gconf_client_notify(client,IGNORE_HOSTS_KEY);
necessary because Gconf don't notify automatic on my system when I re-set list (gnome 2.23.6)
And also If I open 2 gnome-network-preferences And in one change IGNORED_LIST In second list doesn't change.

Thanks again, your review very usefull for me
I will correct my code.
But I have one question:
Why "define games are pretty ugly"? I used it for decrease duplicated code and may be from macros name's simple understood purpose of code. Am I right?

(In reply to comment #9)
> I think 
> gconf_client_notify(client,IGNORE_HOSTS_KEY);
> necessary because Gconf don't notify automatic on my system when I re-set list
> (gnome 2.23.6)
> And also If I open 2 gnome-network-preferences And in one change IGNORED_LIST
> In second list doesn't change.

That is because the capplet doesn't listen to changes to that key for some reason. If it did, it should update automatically as it does for the other settings.

> Why "define games are pretty ugly"? I used it for decrease duplicated code and
> may be from macros name's simple understood purpose of code. Am I right?

Well, yes, but the way you used them has two major problems. The first is that the macros rely on variables defined in the parent function which isn't obvious so it's a potential trap for other people modifying the code. The second is that a "return" inside a macro (which looks like a function) has the unexpected effect of terminating the calling function. Again, this is a possible pitfall for other people looking at the code. Therefore, if you want to use macros they should be self-contained and not have any side-effects on the code in the calling function.

(That said, there are of course exceptions to everything, but they should be just that, exceptions. In general, in any bigger project where several people work together on one code base, readability and maintainability trump intransparent typing/size optimizations.)

Created attachment 116320 [details] [review]
Add locations(corrected)

Created attachment 116321 [details]
Screenshot

+                <property name="label" translatable="yes">gtk-apply</property>

Stock items aren't translatable.

+                <property name="response_id">0</property>

You should remove these if they are not used for dialog response signals (that's a bug in glade, actually).

My previous comment about coding style still stands.

+	GSList *val=gconf_client_get_list(client,from,GCONF_VALUE_STRING,NULL);
+	GSList *iter=val;
+
+	/*load PROXY_AUTOCONFIG_URL_KEY*/
+	if (iter!=NULL)
+	{
+		gconf_client_set_string(client,PROXY_AUTOCONFIG_URL_KEY,iter->data,NULL);\
+		iter=g_slist_next(iter);
+	}

This looks like API abuse to me. (Does it even work? Maybe I really should try,..) gconf_client_get_list is for reading values for keys which store lists. You seem to be using it to read all keys in a directory. There's gconf_client_all_entries for that. Additionally, the current implementation might return keys in alphabetical order, but I don't think there's any API guarantee for that, so we shouldn't rely on it.

The same applies to save_location as well, of course.

Let's focus on the screenshot for a bit, though.

This should be a basis for a discussion and I'd also like to encourage other people to chime in. I'd prefer a non-editable combobox for the locations with "Add..." and "Delete" buttons next to it. When pressing "Add...", you can pop up a dialog asking for the location name. That way you don't need separate save and apply buttons because it's obvious when to save things. Our configuration dialogs shouldn't have apply buttons if we can avoid it. They should always be instant-apply.

Yes It work.
load_location recive "from" This is list of all stored value. And load_location read all value from this list.

Yes, I prefer also a non-editable combo box. 

As for the load/save_location code, I agree with Jens, you seem to rely on the keys coming out in a specific order, but I think it would be better to check the key name instead of relying on them coming in a specific order. Think that GConf entries can be changed by hand.

Also, we usually put { at the same line as the statement they open, so:

if (....)
{

should be

if (....) {

Also, we add a extra space before the ( in function calls.

Created attachment 117097 [details] [review]
GtkComboBoxEntry -> GtkComboBox & Other

Maxim, did you understand what Rodrigo and I were saying? I am asking because it looks like you didn't address any of the concerns except for the combo box conversion. If you need clarification for anything please ask. I'd really like to put you on the right track...

Sorry may be I don't understand something.
But 
1. Keys may come in any order
2. I add a extra space before the ( in function calls
3. I try to use same code style and in this case
I don't correct "if" statement(becouse all other code use same statement as mine)
Should I corect this?
4. I correct all in glade file

Hm, ok, so that was a bit incomplete and you understood a few things a bit differently than I intended. I'll try to clarify.

> 1. Keys may come in any order

Yes, plus you may get keys you don't expect or not get keys you do expect. My main point was "don't use gconf_client_(g/s)et_list" for what you are trying to do. Explicitly get the handful of keys you need using gconf_client_(g/s)et_(string/int/boolean/etc).

> 2. I add a extra space before the ( in function calls

You didn't add them everywhere, though, and if you look at the example I give in my first comment you should also add spaces in a number of other places (e.g. before and after =, +,..., after , and so on.

> 3. I try to use same code style and in this case
> I don't correct "if" statement(becouse all other code use same statement as
> mine)
> Should I corect this?

That I don't quite understand.

> 4. I correct all in glade file

The glade file indeed looks better, although you seem to mix spaces and tabs for indentation. Don't do that.

+                <property name="items" translatable="yes"> </property>

And don't mark empty strings as translatable.

(In reply to comment #19)
> Yes, plus you may get keys you don't expect or not get keys you do expect. My
> main point was "don't use gconf_client_(g/s)et_list" for what you are trying to
> do. Explicitly get the handful of keys you need using
> gconf_client_(g/s)et_(string/int/boolean/etc).

Should I add validation code (that is really list) 
Or 
I should corrected code and store keys in different directory for different location (Now I store in different list)?

One directory per location would be the best solution, I think.

Created attachment 117215 [details] [review]
rewrited patch

One list per location -> One directory per location
&
Correct code style
&
Correct glade style

Created attachment 117263 [details]
proposed mockup

So, here's my quick and dirty proposal for what the dialog might look like instead.

Regarding your patch update:

In the glade file you now use tabs consistently (consistency=good) while the file uses spaces everywhere else (no consistency=bad).

+	skey = g_strdup (key + t);
+	GConfValue* val = gconf_client_get (client, key, NULL);

Don't declare variables in the middle of a block. Several occurences.

I'm still somewhat hesitant about your approach to load/save_location. Why don't you just do it like this (pseudo-code):

key = create_key (location, PROXY_AUTOCONFIG_URL);
url = gconf_client_get_string (key);
g_free (key);

key = create_key (loation, USE_PROXY_KEY);
use_proxy =gconf_client_get_bool (key);
g_free (key);
...

While the code might (might!) be a tad longer, it's certainly much clearer in what it attempts to do.

Additionally, if the user doesn't have any locations in gconf, the currently active configuration should show up as "Default" or something, not as an empty combobox.

+		GtkDialog *dialog = GTK_DIALOG (gtk_message_dialog_new (NULL, 0, GTK_MESSAGE_ERROR, GTK_BUTTONS_CANCEL, "Incorrect location name"));

Strings like this need to be marked for translation using the appropriate gettext macro, in this case _("Incorrect location name")

+	gconf_client_suggest_sync (client, NULL);

If this call is needed something is most likely wrong with the code.

Created attachment 117409 [details] [review]
corrected patch

I rewrote load/save_location & corrected apperance

+		<widget class="GtkButton" id="delete_button">
+		  <property name="visible">True</property>
+		  <property name="can_focus">True</property>
+		  <child>
+			<widget class="GtkHBox" id="hbox5">
+			  <property name="visible">True</property>
+			  <property name="homogeneous">False</property>
+			  <property name="spacing">2</property>
+			  <child>
+				<widget class="GtkImage" id="image1">
+				  <property name="visible">True</property>
+				  <property name="stock">gtk-delete</property>
+				  <property name="icon_size">4</property>
+				  <property name="xalign">0.5</property>
+				  <property name="yalign">0.5</property>
+				  <property name="xpad">0</property>
+				  <property name="ypad">0</property>
+				</widget>

You shouldn't add images to a button like this. GTK+ has a setting gtk-button-images which you can use to disable images on buttons. If the widgets have been created like this that doesn't work. The easiest way around this is to add the image in code when setting up the dialog instead of doing it in glade. We're doing this in a number of other capplets as well (e.g. the a11y one).

I also get a lot of compiler warnings like this one:
gnome-network-preferences.c:870: warning: passing arg 3 of `gconf_client_notify_add' from incompatible pointer type

+	tstr = gconf_client_get_string (client, fk, NULL);
+	gconf_client_set_string (client, dest, tstr, NULL);
...
+	tstr = gconf_client_get_string (client, fk, NULL);
+	if (tstr != NULL) gconf_client_set_string (client, dest, tstr, NULL);

You need to check for NULL for all strings from gconf, not just for some.

+	gconf_client_notify (client, LOCATION_DIR);

You shouldn't need to notify manually. Something's wrong if you do.

+	askdialog = gtk_dialog_new_with_buttons ( _("Save As"),

The title should be "Create New Location" or something like that.

+	content_area = gtk_dialog_get_content_area (GTK_DIALOG (askdialog));

You may want to think about adding that dialog to the glade file as well.

+		key_name = g_strconcat (LOCATION_DIR, "/", gtk_entry_get_text (GTK_ENTRY (text)), NULL);
+		if ( gconf_valid_key (key_name,NULL) )

You should check for an empty key separately. In the other cases you can make the key valid using gconf_escape_key.

Now, some more general remarks.

I think it would be good to not keep locations around redundantly. So, if the user selects a new location, save the current location, copy new location to current, then delete the new location from GConf. That way you don't have to update in places all the time and monitor both settings for changes etc.

With regards to the locations combobox, the "New Location" entry should be separated from the other entries and look differently like in my mock-up. This means you cannot use combo_box_append_text and you need to create your own GtkTreeModel. It also should be last in the box.

And the "Default" entry should only be present if the "current location" entry in GConf in empty/non-existent. And the delete button should be disabled if there aren't at least two locations.

That is all I can think of for now. ;-)

Created attachment 117811 [details] [review]
corrected patch

glade file corrected & fix all warnings & now checks all string from gconf & now location name store escaped & "New location" at end of the list and separated from other & Default can't be deleted & does not monitor both settings for changes.
I think Default entry should always present.

+	..</child>

What are the dots doing there?

+		<widget class="GtkButton" id="delete_button">
+		  <property name="visible">True</property>
+		  <property name="can_focus">True</property>
+		  <property name="sensitive">False</property>		  
+		  <child>
+			<widget class="GtkHBox" id="hbox5">
+			  <property name="visible">True</property>
+			  <property name="homogeneous">False</property>
+			  <property name="spacing">2</property>
+			</widget>
+		  </child>

You don't need the child (see below).

+gboolean
+location_combo_separator (GtkTreeModel *model,
+						 GtkTreeIter *iter,
+						 gpointer data)
+{
+	int i;
+	gchar *n = NULL;
+	GtkTreePath *path = gtk_tree_model_get_path (model, iter);
+	n=gtk_tree_path_to_string (path);
+	gtk_tree_path_free (path);
+	sscanf (n, "%d", &i);
+	g_free (n);
+	if (i == gtk_tree_model_iter_n_children (model, NULL) - 2) return TRUE;
+	else return FALSE;
+}

Why don't you just check against "" here? Seems much simpler.

+		gchar* key_esc = gconf_escape_key (gtk_entry_get_text (GTK_ENTRY (text)), -1);
+		key_name = g_strconcat (LOCATION_DIR, "/", key_esc, NULL);
+		if ( gconf_valid_key (key_name,NULL) )
+		{

You cannot get invalid keys here.

+			k = 1;

Use a boolean instead?

+	gtk_container_add (GTK_CONTAINER (WID ("hbox5")), gtk_image_new_from_stock ("gtk-delete", 4));
+	gtk_container_add (GTK_CONTAINER (WID ("hbox5")), gtk_label_new(_("Delete Location")));

That's no improvement over the glade version. It does exactly the same thing. What you want is gtk_button_set_image, and set the label in glade. Also, use the proper define instead of "4" for the size and GTK_STOCK_DELETE instead of "gtk-delete", and give the widgets you reference from code useful names.

> I think Default entry should always present. 

Why? Let's assume the following screnario:

I have my normal setup as "Default". Now I find some great unsecured WLAN I'd like to use instead, and set it up as "WLAN". After a while it turns out the WLAN is run 24/7, and there's no need for you to keep your old connection profile, so you delete "Default". "WLAN" is now your only profile. You could of course add a new "Default" whenever you wanted to, but why should I have to reconfigure "Default" to be "WLAN" and then delete "WLAN" just to get rid of the obsolete profile?

I also think the "New Location..." entry should make it more obvious that it's not a regular entry which is why I used italics in my mockup (and which probably means you can't use a simple text-based combobox. I might be wrong there, though).

All in all, we're getting there.

Created attachment 118065 [details] [review]
corrected patch

All should be fine now.

Created attachment 118156 [details] [review]
cleaned up

I've taken the patch and cleaned it up with regards to coding style and other minor issues. I've also added a few inline comments (look for "FIXME") where changes are still necessary. I hope those are detailed enough, if anything is unclear please ask.

Created attachment 118304 [details] [review]
corrected patch

Created attachment 119476 [details] [review]
updated

Ok, so I've shuffled things around a bit. Still a couple of FIXMEs left in the code.

Actually it would be nice if the "location" selection could be based on the current network domain, just like Vista does. When connecting via DHCP, the DHCP server gives us DNS configuration entries which are then put in /etc/resolv.conf. The "domain" or "search" entry could be used as keys to automagically select a location. I guess this has to be done by NetworkManager more than by Gnome's network-preferences.

Example: I plugin at three different locations (two clients and home). Both clients use proxies (one is authenticated and needs an exclusion list of hosts, the other one is anonymous and doesn't need an exclusion list). At home I am directly connected to the Internet.
When I connect at Client 1, DHCP gives me a "domain clienta.com", and Network Manager selects proxy.clienta.com with port 3128, username and password and sets the exclusion list.
When I connect at Client 2, DHCP gives me a "domain clientb.com", and Network Manager selects proxy.clientb.net with port 8080, and clears the authentication and exclusion fields.
When I connect at home, DHCP provides no "domain" information (just nameserver entries), and Network Manager selects "Direct connection".

Created attachment 120880 [details] [review]
updated patch

Unfortunately doesn't apply against trunk.

Created attachment 121343 [details] [review]
Updated patch. Now it can be apply for current trunk.

Created attachment 121374 [details] [review]
another update

Great. Seems to work well now, we're almost there. I mostly fixed a few compiler warnings in this iteration. I think there's only two things left to do now:

* the new location dialog still looks bad, and spacing and padding is all wrong if you look at the HIG. The entry and the label should probably be placed in an HBox instead of a VBox, too.

* if you try to create a duplicate location, the new location dialog is closed and a new error dialog is opened. It would be great if instead the new location dialog stayed open and the error message was shown inline in that window.

Created attachment 121406 [details] [review]
Corrected "new location" dialog style && duplicate location message now show inline in "new location" dialog

Hm, interesting. That's a solution I didn't expect. ;-)

It does have two problems, though: First, you should never use fixed colour values. Depending on what theme settings the user has, it might make certain elements/fonts completely invisible/unreadable. Even if the red is not a problem, it's rather hard to find out what's the matter with only the tooltip for explanation.

What I was thinking of when I proposed this behaviour was an invisible label that you'd only make visible at the time where you now turn the entry red. An additional label of course has the problem that it affects the dialog size when you show it which isn#t that great either. Maybe you've got another idea?

Oh, and the spacing in the dialog is still rather huge, but I can't find the knob to turn it down, either. If we don't succeed there, we'll just turn the dialog over to the HIG police when we're done with the rest.

Created attachment 121816 [details] [review]
Patch for this bug

Now I use style->mid color. 
And I remove tooltip_text
& use visible label (but with background color) and that change color at the time when entry become style->mid color.

I have committed this with a few small fixes and changes.

I also switched to using the invisible label since I believe it's a bit better than meddling with the styles, but as I said I'm not quite happy with it, either. In any case, I do expect some more changes to the "New location" window wrt HIG compliance, but that's not really my field of expertise.

Anyway, thanks a lot Maxim for your endurance. ;-)

2008-11-08  Jens Granseuer  <...>

        Patch by: Maxim Ermilov <...>

        * gnome-network-preferences.c: (cb_dialog_response),
        (copy_location_create_key), (copy_location),
        (get_current_location), (location_combo_separator),
        (cb_current_location), (update_locations),
        (cb_location_new_text_changed), (location_new),
        (cb_location_changed), (cb_delete_button_clicked), (setup_dialog):
        * gnome-network-preferences.glade: add support for network profiles
        (bug #477040)