444157 - [Proxy] Migrate SeaMonkey's Proxies preferences to new pref window

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Ian Neal]

As well as migrating would be good to do some tidy up and implement the SM equivalent of bug 416274

Comment 1

[Tony Mechelynck [:tonymec]]

(In reply to comment #0)
> As well as migrating would be good to [...] implement the SM
> equivalent of bug 416274
> 

Isn't that (more or less) what the present "automatically discover" radio button means?

Comment 2

[Ian Neal]

(In reply to comment #1)
> (In reply to comment #0)
> > As well as migrating would be good to [...] implement the SM
> > equivalent of bug 416274
> > 
> 
> Isn't that (more or less) what the present "automatically discover" radio
> button means?
> 
No, it is a Linux/UNIX specific thing, see Bug 66057 for details.

Comment 3

[Ian Neal]

Attachment: pref-popups patch v0.1

This patch:
* Moves pref-proxies(-advanced) across to new pref window
* Adds new network.proxy.type of 5 for non-OSX *nix

Stuff missing:
* Removal of pref-proxies-advanced.js from jar.mn
* Advanced box for some reason is missing bottom of group box unless you resize so the description doesn't word wrap.

Comment 4

[Stefan [:stefanh]]

Does this works on mac as well (bug 125995)?

Comment 5

[[e-mail]]

Comment on attachment 328598 [details] [diff] [review]
pref-popups patch v0.1

I don't like this patch, because there's not really enough room in the pref panel to show the SSL, FTP and Gopher settings. Please leave them in the Advanced dialog. (Speaking of which, I need to look into why its size is wrong.)

>+  // Check for system proxy settings class and unhide UI if present
>+  if (Components.classes["@mozilla.org/system-proxy-settings;1"])
>+    document.getElementById("systemPref").removeAttribute("hidden");
stefan, does this answer your question?

Comment 6

[[e-mail]]

Comment on attachment 328598 [details] [diff] [review]
pref-popups patch v0.1

>-        style="width: 33em;"
>+            style="width: 48ch; height: 16em;"
Well part of the cause of that bug was easy enough to find :-)

Somehow the padding is coming out all wrong, but I'm not sure what attributes and styles are supposed to be in effect; maybe Mnyromyr might know?

Comment 7

[Stefan [:stefanh]]

(In reply to comment #5)
> (From update of attachment 328598 [details] [diff] [review])
> I don't like this patch, because there's not really enough room in the pref
> panel to show the SSL, FTP and Gopher settings. Please leave them in the
> Advanced dialog. (Speaking of which, I need to look into why its size is
> wrong.)
> 
> >+  // Check for system proxy settings class and unhide UI if present
> >+  if (Components.classes["@mozilla.org/system-proxy-settings;1"])
> >+    document.getElementById("systemPref").removeAttribute("hidden");
> stefan, does this answer your question?
>

I was mainly curious if it worked, since I haven't tested myself (note that if anyone is eager to try, I think you have to check on 1.9.1). But yes, I think it answers my question.

Comment 8

[Ian Neal]

Attachment: pref-proxies patch v0.2

Changes since v0.1:
* Went back to original SM UI look for main and child pref windows

Issues remaining:
* Missing border in advanced proxies pref window - if type="child" is set for the prefwindow then border appears but no matter what the instantapply setting you always get "OK" and "Cancel" buttons.

Comment 9

[[e-mail]]

(In reply to comment #8)
> Issues remaining:
> * Missing border in advanced proxies pref window - if type="child" is set for
> the prefwindow then border appears but no matter what the instantapply setting
> you always get "OK" and "Cancel" buttons.
Apparently this is by design; Firefox's proxies works like this too.

Comment 10

[[e-mail]]

Comment on attachment 329230 [details] [diff] [review]
pref-proxies patch v0.2

>+            windowtype="mozilla:preferences"
This looks wrong.

>+  // If not instant apply then backup current pref settings
>+  // for child prefwindow so that they can be restored in
>+  // the event of cancel being clicked on parent window
So, it seems that a true child prefwindow will already do this correctly - it will event create <preference> elements in the main window to store the saved settings if they do not already exist, so for instance the http proxy settings will reuse the existing preference object in the main window.

>+  // Check for system proxy settings class and unhide UI if present
>+  if (Components.classes["@mozilla.org/system-proxy-settings;1"])
if ("@mozilla.org/system-proxy-settings;1" in Components.classes)

>+  if (gShareSettings.value == "")
Hmm, what value do you get these days for an unset pref?

>+  if (gRadiogroup.value == 3)
>+    gRadiogroup.value = 0;
Shouldn't you be setting the <preference> object here?

>+    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                          .getService(Components.interfaces.nsIPrefBranch);
>+    var pacURL = prefs.getCharPref("network.proxy.autoconfig_url");
Just use .valueFromPreferences

Comment 11

[Ian Neal]

(In reply to comment #10)
> (From update of attachment 329230 [details] [diff] [review])
> >+            windowtype="mozilla:preferences"
> This looks wrong.
As in don't use this attribute or change it to "mozilla:preferenceschild" or similar?
> 
> >+  // If not instant apply then backup current pref settings
> >+  // for child prefwindow so that they can be restored in
> >+  // the event of cancel being clicked on parent window
> So, it seems that a true child prefwindow will already do this correctly - it
> will event create <preference> elements in the main window to store the saved
> settings if they do not already exist, so for instance the http proxy settings
> will reuse the existing preference object in the main window.
Indeed it does.
> 
> >+  // Check for system proxy settings class and unhide UI if present
> >+  if (Components.classes["@mozilla.org/system-proxy-settings;1"])
> if ("@mozilla.org/system-proxy-settings;1" in Components.classes)
Done.
> 
> >+  if (gShareSettings.value == "")
> Hmm, what value do you get these days for an unset pref?
null, so changed the check to == null
> 
> >+  if (gRadiogroup.value == 3)
> >+    gRadiogroup.value = 0;
> Shouldn't you be setting the <preference> object here?
gRadiogroup is the <preference> object.
> 
> >+    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
> >+                          .getService(Components.interfaces.nsIPrefBranch);
> >+    var pacURL = prefs.getCharPref("network.proxy.autoconfig_url");
> Just use .valueFromPreferences
Done, makes the code alot simpler

Comment 12

[Ian Neal]

On the main proxies pref pane should I look at dropping the "Port" to the next line to give more space to display the "Proxy" textbox? Have the "Advanced" button on the same line as "Port"?

Comment 13

[[e-mail]]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 329230 [details] [diff] [review] [details])
> > >+            windowtype="mozilla:preferences"
> > This looks wrong.
> As in don't use this attribute
Yes.

> > >+  if (gRadiogroup.value == 3)
> > >+    gRadiogroup.value = 0;
> > Shouldn't you be setting the <preference> object here?
> gRadiogroup is the <preference> object.
Then it's badly named...

Comment 14

[Ian Neal]

Attachment: pref-proxies patch v0.2a

Changes since v0.2:
* Removed windowtype attribute and added type="child" to pref-proxies-advanced.xul
* Adjusted size of child prefwindow
* Renamed gRadiogroup to gProxyType
* Made use of .valueFromPreferences instead of using PrefBranch service
* Adjusted pref-proxies UI layout to give more space for the proxy host textbox

Comment 15

[[e-mail]]

Comment on attachment 329258 [details] [diff] [review]
pref-proxies patch v0.2a

There's a lot of nits but they're all small ones :-)

>+            buttons="accept,cancel,help"
I don't think you need this on a <prefwindow>.

>+            style="width: 54ch; height: 30em;"
I don't think a height is a good idea; it should autosize the height.
I notice that your width has changed since an earlier patch!

>+            persist="screenX,screenY">
Would you mind changing the comma to a space?

>+          <row align="center">
>+            <hbox align="center" pack="end">
Since we have to put align="center" on the <hbox>es anyway,
you might as well remove it from the <row>s.

>+            <hbox align="left">
Please remove this obsolete align attribute.

>+      <grid>
>+        <columns>
>+          <column/>
>+          <column flex="1"/>
>+        </columns>
>+        <rows>
>+          <row align="center">
>+            <hbox align="center" pack="end">
>+              <label value="&socks.label;"
>+                     accesskey="&socks.accesskey;"
>+                     control="networkProxySOCKS"/>
>+            </hbox>
>+            <hbox align="center">
>+              <textbox id="networkProxySOCKS"
>+                       preference="network.proxy.socks"
>+                       flex="1"
>+                       class="uri-element"/>
>+              <label value="&port.label;"
>+                     accesskey="&SOCKSport.accesskey;"
>+                     control="networkProxySOCKS_Port"/>
>+              <textbox id="networkProxySOCKS_Port"
>+                       type="number"
>+                       preference="network.proxy.socks_port"
>+                       max="65535"
>+                       size="5"/>
>+            </hbox>
>+          </row>
>+        </rows>
>+      </grid>
I don't have a problem with you moving the radiogroup and checkbox, but, when there's only one row left, leaving the grid in is rather pointless ;-)

>+  gAutoURL = PrefElementById("networkProxyAutoconfigURL");
>+  gProxyType = PrefElementById("networkProxyType");
Is this really more readable then
gProxyType = document.getElementById("network.proxy.type"); ?

>+    document.getElementById("systemPref").removeAttribute("hidden");
.hidden = false; also works, if you prefer that.

>+  switch (Number(gProxyType.value))
Doesn't the pref object automatically return a Number these days?

>+      if (!gProxyType.locked)
>+      {
>+        EnableUnlockedElements(manual, true);
>+      }
Don't need the {}s.

>+  if (!gInstantApply)
>+    EnableUnlockedButton(aURL);
I notice that this doesn't update as you type, but it's better than what we had before and it might be the best we can do with the current prefwindow.

>+        <vbox align="left">
Please fix these to align="start" (note different from the above <hbox>!)

>+              <textbox id="networkProxyHTTP"
>+                       preference="network.proxy.http"
>+                       flex="1"
>+                       class="uri-element"/>
Actually this flex is unnecessary, as the <textbox> is the child of a <row> and so it always takes the full length. On the other hand, in this case, you do need align="center" on the row (it's a pity that pack doesn't work at all, and align doesn't work on columns; maybe I can figure that one out for 1.9.1).

>+            <row align="center">
Both <hbox>es have align, so you don't need it here.

>+              <label value="&noproxyExplain.label;"
>+                     control="networkProxyNone"/>
On my test system this fitted exactly, which was rather handy ;-)
Maybe we should make this a multiline <label> just in case?

>+<!ENTITY  HTTPPort.accesskey          "P">
>+<!ENTITY  SSLPort.accesskey           "o">
>+<!ENTITY  FTPPort.accesskey           "r">
Please ask KaiRo whether he minds these non-functional renames.

Comment 16

[Ian Neal]

(In reply to comment #15)
> (From update of attachment 329258 [details] [diff] [review])
> >+            style="width: 54ch; height: 30em;"
> I don't think a height is a good idea; it should autosize the height.
> I notice that your width has changed since an earlier patch!
I wanted to give the same size to the proxy host textboxes as under the old prefwindow, and as you noted the number textboxes take up more space...
> 
> >+  switch (Number(gProxyType.value))
> Doesn't the pref object automatically return a Number these days?
It doesn't seem so - the radio buttons have strings for values and the preference object stores them as strings - probably a bug but not in the scope of this bug

> >+<!ENTITY  HTTPPort.accesskey          "P">
> >+<!ENTITY  SSLPort.accesskey           "o">
> >+<!ENTITY  FTPPort.accesskey           "r">
> Please ask KaiRo whether he minds these non-functional renames.
KaiRo seems happy for these renames to happen.

Comment 17

[Ian Neal]

Attachment: pref-proxies patch v0.2b

Changes since v0.2a:
* Removed unneeded buttons line from pref-proxies-advanced.xul
* Removed unneeded height style from pref-proxies-advanced.xul
* Removed unneeded align attributes from various files
* Removed unneeded grid from SOCKS part of pref-proxies-advanced.xul
* Removed use of PrefElementById
* Made example a multiline description rather than singleline label
* Resolved various minor nits

Carrying forward sr= and requesting r=

Comment 18

[[e-mail]]

(In reply to comment #16)
> (In reply to comment #15)
> > >+  switch (Number(gProxyType.value))
> > Doesn't the pref object automatically return a Number these days?
> It doesn't seem so - the radio buttons have strings for values and the
> preference object stores them as strings - probably a bug but not in the scope
> of this bug
OK, but do you have to convert it to a number for the switch to work?

Comment 19

[Ian Neal]

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > >+  switch (Number(gProxyType.value))
> > > Doesn't the pref object automatically return a Number these days?
> > It doesn't seem so - the radio buttons have strings for values and the
> > preference object stores them as strings - probably a bug but not in the scope
> > of this bug
> OK, but do you have to convert it to a number for the switch to work?
> 
Yes, or include both the number and string in the case labels.

Comment 20

[Ian Neal]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > >+  switch (Number(gProxyType.value))
> > > > Doesn't the pref object automatically return a Number these days?
> > > It doesn't seem so - the radio buttons have strings for values and the
> > > preference object stores them as strings - probably a bug but not in the scope
> > > of this bug
> > OK, but do you have to convert it to a number for the switch to work?
> > 
> Yes, or include both the number and string in the case labels.
> 
Logged bug 445049 on this issue.

Comment 21

[Ian Neal]

Comment on attachment 329312 [details] [diff] [review]
pref-proxies patch v0.2b

>Index: common/pref/pref-proxies.js
>===================================================================
>   DoEnabling();
>   
>   // The pref value 3 for network.proxy.type is unused to maintain
>   // backwards compatibility. Treat 3 equally to 0. See bug 115720.
>+  if (gProxyType.value == 3)
>+    gProxyType.value = 0;
It might be worth me putting this check before the DoEnabling();

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

Somebody with appropriate rights might want to add [Proxy] or something to the summary to make this bug more discoverable. And while you're at it, also consider adding bug 416274 to the depends list since a SM patch that is part of that bug has just been checked in.

Comment 23

[Ian Neal]

Attachment: pref-proxies patch v0.2c

Changes since v0.2b:
* Added missing changes to utilityOverlay.* - used disable rather than hidden for system proxy menu item due to issues on first popup with it appearing at bottom and then jumping to correct position
* Moved DoEnabling as I said in comment 21
* Made change for once patch on bug 445049 has landed (removed Number in switch)

Re-requesting sr due to number of additions

Comment 24

[[e-mail]]

(In reply to comment #23)
> * Added missing changes to utilityOverlay.* - used disable rather than hidden
> for system proxy menu item due to issues on first popup with it appearing at
> bottom and then jumping to correct position
I think that always disabled item looks ugly.
Would making it visible by default and hiding it in script work better?

Comment 25

[Ian Neal]

(In reply to comment #24)
> (In reply to comment #23)
> > * Added missing changes to utilityOverlay.* - used disable rather than hidden
> > for system proxy menu item due to issues on first popup with it appearing at
> > bottom and then jumping to correct position
> I think that always disabled item looks ugly.
> Would making it visible by default and hiding it in script work better?
> 
No, not really. On first popup the menu appears and then drops about 2-3cm - perhaps should look at hiding the menu item on load of the overlay?

Comment 26

[Ian Neal]

Attachment: pref-proxies patch v0.2d

Changes since v0.2c:
* Unhides menuitem through load event instead of popup.

Comment 27

[[e-mail]]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > * Added missing changes to utilityOverlay.* - used disable rather than hidden
> > > for system proxy menu item due to issues on first popup with it appearing at
> > > bottom and then jumping to correct position
> > I think that always disabled item looks ugly.
> > Would making it visible by default and hiding it in script work better?
> No, not really. On first popup the menu appears and then drops about 2-3cm -
Odd... maybe that's a regression? (given a standalone test case of course)

Comment 28

[[e-mail]]

Comment on attachment 331864 [details] [diff] [review]
pref-proxies patch v0.2c

>+  // Check for system proxy settings class and enable UI if present
>+  if ("@mozilla.org/system-proxy-settings;1" in Components.classes &&
>+      !proxyLocked)
>+    networkProxySystem.removeAttribute("disabled");
>+  else
>+    networkProxySystem.setAttribute("disabled", "true");
I tried this patch on my 1.9.0.x (CVS) build but replacing "disabled" with "hidden" and everything looked fine to me.

Comment 29

[[e-mail]]

Comment on attachment 331983 [details] [diff] [review]
pref-proxies patch v0.2d

That said, showing it on startup is not a problem, but remember to check that the element exists first - only the browser window has the context menu!

Comment 30

[Ian Neal]

(In reply to comment #27)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (In reply to comment #23)
> > > > * Added missing changes to utilityOverlay.* - used disable rather than hidden
> > > > for system proxy menu item due to issues on first popup with it appearing at
> > > > bottom and then jumping to correct position
> > > I think that always disabled item looks ugly.
> > > Would making it visible by default and hiding it in script work better?
> > No, not really. On first popup the menu appears and then drops about 2-3cm -
> Odd... maybe that's a regression? (given a standalone test case of course)
> 
I think this is due to the position of the window on the screen and how close it is to the bottom of the screen. If there is enough space then it pops downwards from the mouse cursor whereas if there isn't then it pops upwards from the mouse cursor. Now if you start hiding menuitems then it could jump the menu round given certain window positions relative to the bottom of the screen.

Comment 31

[Ian Neal]

Attachment: pref-proxies patch v0.2e

Changes since v0.2d:
* Added check for existence of "network-proxy-system" element

Carrying forward sr=

Comment 32

[[e-mail]]

(In reply to comment #30)
> (In reply to comment #27)
> > (In reply to comment #25)
> > > On first popup the menu appears and then drops about 2-3cm
> > Odd... maybe that's a regression? (given a standalone test case of course)
> I think this is due to the position of the window on the screen and how close
> it is to the bottom of the screen. If there is enough space then it pops
> downwards from the mouse cursor whereas if there isn't then it pops upwards
> from the mouse cursor. Now if you start hiding menuitems then it could jump the
> menu round given certain window positions relative to the bottom of the screen.
Wow, I see what you mean now - that's one annoying flicker.

Comment 33

[Ian Neal]

(In reply to comment #32)
> (In reply to comment #30)
> > (In reply to comment #27)
> > > (In reply to comment #25)
> > > > On first popup the menu appears and then drops about 2-3cm
> > > Odd... maybe that's a regression? (given a standalone test case of course)
> > I think this is due to the position of the window on the screen and how close
> > it is to the bottom of the screen. If there is enough space then it pops
> > downwards from the mouse cursor whereas if there isn't then it pops upwards
> > from the mouse cursor. Now if you start hiding menuitems then it could jump the
> > menu round given certain window positions relative to the bottom of the screen.
> Wow, I see what you mean now - that's one annoying flicker.
> 
Bug 449680 logged on this issue but shouldn't really be a blocker to this bug as I work around it.

Comment 34

[Steve Snyder]

Please see Bug 451135 for a related enhancement request.

Thank you.

Comment 35

[Karsten Düsterloh]

Comment on attachment 332132 [details] [diff] [review]
pref-proxies patch v0.2e

>+++ b/suite/common/pref/pref-proxies.js
> function Startup()
> {
>-  initElementVars();
>+  InitGlobals();
>+  gAutoURL = document.getElementById("network.proxy.autoconfig_url");
>+  gProxyType = document.getElementById("network.proxy.type");

"InitGlobals" is rather confusing if you init globals afterwards. ;-)
Better call it "InitCommonGlobals" or something like that.

>+  // The pref value 3 for network.proxy.type is unused to maintain
>+  // backwards compatibility. Treat 3 equally to 0. See bug 115720.
>+  if (gProxyType.value == 3)
>+    gProxyType.value = 0;

Please use constants.

> function DoEnabling()
...
>+    case 0:
>+    case 4:
>+    case 5:
...

Please use constants.

>+function Disable(aElementIds)
>+  for (var i = 0; i < aElementIds.length; i++)
>+  {
>+    var element = document.getElementById(aElementIds[i]);
>+    element.setAttribute("disabled", "true");

No need to define element if you don't check it anyway.
And you can drop the braces then, too.

>+function EnableUnlockedButton(aElement)
>+{
>+  var enable = gInstantApply ||
>+               (aElement.valueFromPreferences == aElement.value);
>+
>+  EnableElementById("autoReload", enable, false);

Uncessary empty line.

>+++ b/suite/common/pref/preferences.xul
>+          <treeitem id="proxiesItem" label="&proxies.label;"
>+                    prefpane="proxies_pane" helpTopic="advanced_pref_proxies"
>                     url="chrome://communicator/content/pref/pref-proxies.xul"/>

One attributue per line, please.


r=me with that.

Comment 36

[Ian Neal]

Pushed to comm-central with those changes - http://hg.mozilla.org/comm-central/rev/30bfbed1b0f5