257758 - Add UI for WPAD and option for using the configured SOCKS proxy for DNS lookups to proxy preferences

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, enhancement)

Description

[Stefan Borggraefe]

I think another radio button on this preferences page will make the it to large,
so it will be cut in some configurations.

So I propose to change the page to look and act like this:

( ) Direct connection to the Internet

( ) Automatically discover proxy settings

( ) Automatic proxy configuration URL:
    __________________________________ [Reload]

( ) Manual proxy configuration
    Proxy: _____________ Port: _______ [Advanced]
    No Proxy for: ____________________
                  Example: ... bla ...

Pressing the Advanced button will open a dialog for configuring different
proxies for different protocols. This will look similar to the options I left
out from my new design of this preferences page.

Entering a proxy into the manual proxy configuration will set the same proxy for
all protocols except SOCKS. When a different proxy was already entered in the
Advanced dialog for a certain protocol, this proxy setting will not be altered
when the proxy on the main preferences page was changed.

I will also need to add a new menu item to the context-menu of the
offline-indicator and to reword the existing "auto" option since there are now
two different "auto" options. Perhaps "auto discover" and "auto URL"?

Suggestions/criticism welcome! :-)

Comment 1

[[e-mail]]

Auto (PAC) vs Auto (WPAD) ?

If we got rid of that copy button it would make room for WPAD ;-)

Comment 2

[Stefan Borggraefe]

(In reply to comment #1)
> Auto (PAC) vs Auto (WPAD) ?

In this case these abbreviations should also be mentioned in the preferences
page. I'm not sure whether this is more likely to help ot to confuse users. For
someone who knows what these things are, this would probably be helpful, but
what about the "average end-user"?

> If we got rid of that copy button it would make room for WPAD ;-)

;-)

Comment 3

[Stefan Borggraefe]

Attachment: Screenshot

Mockup of what I described in comment 0.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

>( ) Manual proxy configuration
>    Proxy: _____________ Port: _______ [Advanced]

What will you show here if HTTP, FTP and HTTPS all have a different proxy?

Comment 5

[Stefan Borggraefe]

(In reply to comment #4)
> >( ) Manual proxy configuration
> >    Proxy: _____________ Port: _______ [Advanced]
> 
> What will you show here if HTTP, FTP and HTTPS all have a different proxy?

HTTP.

And changing these fields will only change the proxies that were configured to
the same host+port as the HTTP proxy.

Comment 6

[Stefan Borggraefe]

Attachment: Updated Screenshot Proxy Preferences

Comment 7

[Stefan Borggraefe]

Attachment: Updated Screenshot Advanced Proxy Preferences

Comment 8

[Stefan Borggraefe]

Attachment: Patch

I have finally finished my work on this. I leave out any Help changes from the
patch for now until reviewers agree with my UI decisions.

I changed the "Use HTTP settings for SSL, FTP and Gopher" push-button into a
checkbox. As long as it is selected, the entered HTTP Proxy is also used for
the other protocols. This is immediatly reflected in the UI (the textfields for
SSL, FTP and Gopher are disabled and their texts change as you type into the
HTTP textfields). So this is quite different (I think better) than what I
described in comment 0.

I also put the SOCKS options and the options for the protocol specific proxies
in their respective group boxes together with explanatory descriptions, because
I think both proxy types are quite different in nature. This will hopefully
lessen the number of users that configure a SOCKS proxy by accident when there
is none in their network (a problem I sometimes witnessed on #mozillazine).

The patch also fixes bug 50524.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 170193 [details] [diff] [review]
Patch

+<!ENTITY  socksRemoteDNS.label        "Use for resoving hostnames (recommended
for SOCKS v5)">

"resolving"

Comment 10

[[e-mail]]

Comment on attachment 170193 [details] [diff] [review]
Patch

>+function lockSettings()
>+{
>+  for (var i = 0; i < parentSettings.length; i++) {
>+    var locked = parentSettings[i].getAttribute("disabled");
>+    if (locked)
>+      settings[i].setAttribute("disabled", locked);
>+  }
>+}
This doesn't work for radiogroups, you need to use .disabled instead.

Comment 11

[[e-mail]]

Comment on attachment 170193 [details] [diff] [review]
Patch

>+               oncommand="DoEnabling();"/>ee
E by gum! (This is an English joke.)

>+  // Give older profiles a sane default for network.proxy.share_proxy_settings
Well, that's not exactly true... this code will run for new profiles too.

>+  if (shareSettings.getAttribute("value") == 2)
>+    shareSettings.setAttribute("value", defaultForShareSettingsPref());
An alternative approach is not to set the pref, then the value is copied from
the prefdefval attribute instead. This gives you the option of using a boolean
pref, and using (say) a blank prefdefval, while managing to simplify
defaultForShareSettingsPref too.

>+  // See bug 115720.
Too terse :-P The one for utilityOverlay.js is better.

>+  return ((httpProxy == ftp.getAttribute("value") &&
>+           httpProxy == gopher.getAttribute("value") &&
>+           httpProxy == ssl.getAttribute("value")) &&
>+          (httpProxyPort == ftpPort.getAttribute("value") &&
>+           httpProxyPort == sslPort.getAttribute("value") &&
>+           httpProxyPort == gopherPort.getAttribute("value"))) ? 1 : 0;  
I'm not sure why you wrote ((a && b && c) && (x && y && z)) instead of a && b
&& c && x && y && z plus you left trailing spaces.

>+function unlock(elements)
I'm not sure unlock is the best name for this function, as the point is that it
doesn't affect locked elements.

>+    autoURL.value = URIFixup.createFixupURI(proxyURL.value,
I hope you meant to change the second proxyURL too.

>+  openDialog("chrome://communicator/content/pref/pref-proxies-advanced.xul",
>+             "AdvancedProxyPreferences",
>+             "chrome,titlebar,centerscreen,resizable=no,modal",
>+             document);
The dialog can access your document via opener.document.

>+            <checkbox id="networkProxyShareSettings" label="&reuseProxy.label;"
>+                      accesskey="&reuseProxy.accesskey;"
>+                      oncommand="DoEnabling(); DoProxyCopy();"/>
>+  // SOCKS version is 4 is radio button 0, SOCKS version 5 is radio button 1.
>+  socksVersion.selectedIndex = socksVersion.value - 4;
If you give the radio buttons the appropriate values then setting
socksVersion.value should select the appropriate button (this is new behaviour
that you may not be aware of).

>+function transferSettings(from, to, fromPropertyToAttribute)
>+{
>+  for (var i = 0; i < from.length; i++)
>+  {
>+    var fromValue;
>+
>+    fromPropertyToAttribute ? fromValue = from[i].value
>+                            : fromValue = from[i].getAttribute("value");
>+
>+    // Use "" instead of "0" as the default for port numbers.
>+    // "0" doesn't make sense as a port number.
>+    if (fromValue == "0") 
>+      fromValue = "";
>+
>+    fromPropertyToAttribute ? to[i].setAttribute("value", fromValue)
>+                            : to[i].value = fromValue;
>+  }
>+}
This is trying to be too clever. It would probably be smaller as two functions
(the 0 check would not be necessary when transferring back to the opener).
However I don't think it's doing the right thing for the http proxy because
it's using the attribute and the textbox really only understands the property.
I suppose you could get it to work with some extra code in openAdvancedDialog.

Comment 12

[Stefan Borggraefe]

Attachment: Patch V1.1

Addressed all of biesi's and Neil's comments. (I even looked up the "E by gum!"
joke. ;-) )

- network.proxy.share_proxy_settings is now a bool pref. I tried this before,
but I didn't know about prefdefval and so I failed. With Neil's hint it works
now. :-)

- I split transferSettings() into transferSettingsToMainProxiesPane() and
transferSettingsToAdvancedDialog(). With this change it also made sense to move
some more code into these functions.

- The former unlock() function is now called enableElementsNotLocked().
lockSettings() is now called disableLockedElements() accordingly.

- I hope I now properly deal with the value property and the value attribute.
You need to be very careful with this, I can tell you. An alternative to
prevent this added complexity would be to use hidden XUL widgets instead of
<data> elements. But this would be kind of hacky and would probably cost more
in terms of footprint I guess.

- In addition to the review comments I also changed when DoProxyCopy() in the
main pref pane is called. It is now bound to the blur event instead of the
input event. This way it is called less (but still sufficient) often.

Comment 13

[[e-mail]]

(In reply to comment #12)
>- In addition to the review comments I also changed when DoProxyCopy() in the
>main pref pane is called. It is now bound to the blur event instead of the
>input event. This way it is called less (but still sufficient) often.
Without looking I guess onchange would probably be better still.

Comment 14

[[e-mail]]

Comment on attachment 170376 [details] [diff] [review]
Patch V1.1

>+                       flex="1" class="uri-element" onblur="DoProxyCopy();"/>
Should be onchange like FixProxyUrl is.

>+function enableElementsNotLocked(elements)
Hmm... what about enableUnlockedElements?

>+  // Convenience Arrays
>+  settings = [http, httpPort, ssl, sslPort, ftp, ftpPort, gopher, gopherPort,
>+              socks, socksPort, socksVersion];
>+  parentSettings = [httpParent, httpPortParent, sslParent, sslPortParent,
>+                    ftpParent, ftpPortParent, gopherParent, gopherPortParent,
>+                    socksParent, socksPortParent, socksVersionParent];
If you need to transfer http and httpPort separately then maybe they shouldn't
be in the array?

>+function transferSettingsToAdvancedDialog()
Oh, I'm being very nitty today; you are the advanced dialog, so call this
receiveSettingsFromProxyPanel.

>+    prefValue == "0" ? settings[i].value = ""
>+                     : settings[i].value = prefValue;
Your ?: foo looks wrong (I can't see if it does b ? (a = c) : (a = d) or (b ?
(a = c) : a) = d, even if it in fact works. If you write it as a = b ? c : d it
makes it shorter and simpler too. [Update] Ok, so it does work.

>+function transferSettingsToMainProxiesPane()
A bit long; perhaps sendSettingsToProxyPanel instead?

>+  if (!shareSettings.checked)
>+    return;
>+
>+  ftp.value = gopher.value = ssl.value = http.value;
>+  ftpPort.value = gopherPort.value = sslPort.value = httpPort.value;
I think I would have preferred
  if (shareSettings.checked) {
    ...
  }

>+    var locked = parentSettings[i].getAttribute("disabled");
>+    if (locked)
This works, but it looks odd; you would either use .hasAttribute("disabled") or
.getAttribute("disabled") == "true" (and you might as well inline it in the if
too).

Comment 15

[Stefan Borggraefe]

Attachment: Patch V1.2

I left http and httpPort in the arrays so I can access these elements easily in
disableLockedElements(). I changed everything else like Neil suggested. Thanks
Neil! :-)

shaver, can you sr?

Comment 16

[Stefan Borggraefe]

Attachment: Patch V1.3

I found two problems with Patch V1.2:

- Locking didn't work in all cases. -> Initially disabling all elements
representing prefs that could possibly be locked, fixed this problem. Now the
idea

  1) disable everything
  2) enable everything that belongs to the selected proxy type and is not
locked

is implemented correctly.

- The locked state of network.proxy.socks_remote_dns and
network.proxy.share_proxy_settings was not reflected in the UI (the checkboxes
were not disabled when the prefs were locked) -> Adding both settings to the
settings and parentSettings arrays in prefs-proxies-advanced.js fixed this
problem. Since this addition to the arrays made the for loops in
receiveSettingsFromProxyPanel() and sendSettingsToProxyPanel() too ugly, I have
given up using them.

Comment 17

[Stefan Borggraefe]

Reflecting reality in the bug summary and the target milestone. ;-)

My patch also fixes bug 50524.

Comment 18

[[e-mail]]

(In reply to comment #16)
>Locking didn't work in all cases.
Could you give me an example?

Comment 19

[Stefan Borggraefe]

(In reply to comment #18)
> (In reply to comment #16)
> >Locking didn't work in all cases.
> Could you give me an example?

With Patch V1.2 configure a manual proxy so that the last radio button for
proxy.type is selected. Have some manual proxy options locked. Close the
preferences dialog and open it again.

Result: The locked prefs for the manual proxy configuration don't appear
disabled (in the main prefs pane and in the Advanced dialog).

Reason: They never were disabled. So enableUnlockedElements() practically was a
noop in this case.

This only worked as expected when you somehow disabled the manual pref elements
in some way, e.g. by clicking on the PAC option and then on the Manual option
again. In order to make this work properly I now disable everything in Startup()
(I just noticed that I forgot to disable the element networkProxyNone at this
point. :-( ).

Comment 20

[[e-mail]]

(In reply to comment #19)
>With Patch V1.2 configure a manual proxy so that the last radio button for
>proxy.type is selected. Have some manual proxy options locked. Close the
>preferences dialog and open it again.
> 
>Result: The locked prefs for the manual proxy configuration don't appear
>disabled (in the main prefs pane and in the Advanced dialog).
I just reapplied patch V1.2 and I had no problem locking the gopher proxy.

Comment 21

[[e-mail]]

Comment on attachment 170779 [details] [diff] [review]
Patch V1.3

>+  // Initially disable all elements representing prefs that may be
>+  // locked. All prefs that are in fact not locked will be enabled by
>+  // DoEnabling() again.
>+  disable([http, httpPort, ssl, sslPort, ftp, ftpPort, gopher, gopherPort,
>+           socks, socksPort, socksVersion, socksRemoteDNS, shareSettings,
>+           autoURL, autoReload]);
r=me if you remove this, because it works for me without it.

Comment 22

[Stefan Borggraefe]

Comment on attachment 170779 [details] [diff] [review]
Patch V1.3

Neil is right. I made a mistake when testing the locked prefs.

Comment 23

[Mike Shaver (:shaver emeritus)]

Comment on attachment 170779 [details] [diff] [review]
Patch V1.3

sr=shaver.  I had a slightly different redesign of the proxy pref panel in mind
for firefox, but that flight of fancy is neither here nor there.

Comment 24

[Stefan Borggraefe]

Fix checked in. Thanks for the reviews! :-)

I filed bug 279054 about updating Help accordingly.