50380 - Proxy: Option to set all servers to the same hostname

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, enhancement, P3)

Description

[Stefan Vassall]

Build 2000082513 (Linux)

A really useful feature which IE has but moz doesn't currently is an option to
allow all proxies to be set to the same server. Currently in moz you have to
fill all the boxes out (HTTP, FTP, SSL) when normally there just the same server.

Comment 1

[Asa Dotzler [:asa]]

a checkbox for "use the same proxy server for all protocols"? Is that what
you're talking about. Over to Networking.  

Comment 2

[Stefan Vassall]

Yes Asa, that's exactly what I mean! (Like IE!)

Comment 3

[Doron Rosenberg (IBM)]

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

Comment 4

[Darin Fisher]

How is it that this belongs to networking?  Shouldn't this be resolved by the
implementation of the preferences dialog?

Comment 5

[Darin Fisher]

-> preferences

Comment 6

[Gagan]

->neeti

Comment 7

[Cormac F]

Any chance of seeing this fixed soon? (Sorry for sounding so demanding!)

Using Mozilla over my college network, I have to re-enter all proxy settings 
quite frequently - everytime I download a new build, and often with my initial 
login to the network. With IE, I only have to do this once and click the select 
box. With moz, I have to cut,copy and paste five or six times.
Its an irritant that crops up frequently for a user such as myself, and surely 
should be quite simple to fix before 1.0? 

Comment 8

[Matthew T (active 1999-2002)]

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

Comment 9

[David Illsley]

Attachment: first try at a patch

This is a first shot at adding this feature to moz. It's is my first patch
ever, made with patchmaker on a cvs build. I've tried to keep to obvious code
convention, but I'm sure it can't be right first time. I'm more than willing to
do the work to get this small thing into mozilla.

David Illsley

Comment 10

[David Illsley]

Hi, I appreciate that currently there's the big push to get 1.0 out the door and
so this is low priority - its not my hihest priority either with exams
approaching, but I'd really like to get this on the trunk if its appropriate. It
currently hasn't had any review, or even UI design - I just copied MS IE, not
necesarily a good thing. I've read the relevant docs and they seem to suggest
asking for review etc in the bug. So here goes... can I get attachment 80378 [details] [diff] [review]
reviewed... There's a screenshot of the panel once applied at
http://www.illsley.org/pictures/prefpanel.jpg so you can see what the UI looks like.

Thanks,
David

Comment 11

[Hixie (not reading bugmail)]

David: For the UI, could you attach a screenshot? That will give our UI people a
chance to comment.

Comment 12

[David Illsley]

Attachment: Screenshot with patch applied

Screenshot per request

Comment 13

[Matthew T (active 1999-2002)]

First, it would be best if this was a push button, which changed all non-HTTP
fields but didn't disable them, rather than a checkbox. Reason being that if you
 use the same proxy for most protocols but need specific settings for one or
two, a checkbox would be useless but a button would still be helpful.

| | Colors & Effects| | ( ) _Manual proxy configuration              |
| | Helper Programs | |        _HTTP: [                     ]:[    ] |
| | Multimedia      | |               ( _Use For All Protocols )     |
| | Scripts&Windows | |         _FTP: [                     ]:[    ] |
| | Privacy         | |      _Gopher: [                     ]:[    ] |
| | History & Cache | |         SS_L: [                     ]:[    ] |
| | Web Forms       | |       _SOCKS: [                     ]:[    ] |
| | Appearance      | |               SOCKS _version: (*) v4  ( ) v5 |

Second, if you want this patch to be checked in, you *must* remove the group box
which surrounds the entire panel. It's silly to begin with, and if you don't
remove it the panel will no longer fit in the dialog in the Mac Classic theme.

Third, while you're hacking this panel, it would be nice if you could move the
automatic proxy configuration stuff above the manual proxy configuration stuff.
This is because in a radio group, it's best if the gaps between the radio
buttons are as similar as possible. But you don't have to do that as part of
this bug.

Thanks for working on this.

Comment 14

[David Illsley]

Attachment: new patch per mpt's UI design

New patch which changes to using a button to copy the settings to the other
fields, moves the pac settings above the manual ones- this does make it look
better, and removes the groupbox.

Will attach a screenshot asap.

Comment 15

[David Illsley]

Attachment: Screenshot using patch v2

Screenshot using patch v2.
Forgot to mention before. This patch is so much bigger because removal of the
groupbox changed the indenting for 90% of the file.

What do you think of now?

Comment 16

[timeless]

depending on the behavior of no proxy for, i'd almost like to have a <hr> for 
it.

Comment 17

[Vidar Haarr (not reading bugmail)]

"No proxy for" is attached to the manual configuration, and thus gets disabled
when selecting "no proxy" or "automatic configuration".

Why can't I specify sites that should not use proxy, even if I'm using automatic
proxy configuration ? that makes no sense.

My proposal is: move it a bit down, and slap on a <hr>, or put it in a
<fieldset>, and make it enabled in Automatic and Manual state, and disabled in
"No proxy" state.

Comment 18

[David Illsley]

Currently no proxy for is only concerned with manual settings - as such, adding
an hr seems unnecessary as it should live with the rest of the manual settings.
Its also very clear with automatic selected that no proxy for doesn't work as is
is disabled.

As for changing it so you can override the pac file, that is very much a backend
issue which doesn't at the moment work, and this was to be a simple UI patch to
make the panel a bit more useable. I don't have the knowledge or the time to
make backend changes to support this.

David

Comment 19

[benc]

Adding no proxy to pac is probably a bad idea.

No proxy is easily (and more robustly) configured in a PAC file, so adding code
to do it again would be counter-intuitive.

Comment 20

[timeless]

benc: proxy configurations in networks are often served from special servers 
and automatically updated.  it wouldn't be inconceivable for individual users 
to need to add a few no proxy for entries that their admin didn't specify. 
however it would be disastrous for them to modify the js file because when the 
proxy setting changes, they're stuck.

Comment 21

[benc]

timless: lets move this to a new bug so I can use my newly-delegated authority
in PAC and future it.

This is a remote admin feature. You don't get to ammend the network policy. I'll
give examples of other bad features that sound like this if you create the bug. :)

re patch: I think the general world would be reluctant to re-order the mode
selections. Can you draw a box around each mode or get the UE people to help here?

Comment 22

[David Illsley]

Attachment: Another v2 patch with elements moved around

Here's a patch which removes the groupbox (required for this to work according
to mpt), adds a Use for all protocols button and doesn't reorder the elements.
I can't see a way of putting a box around which doesn't look stupid so that's
not there.

It'd be nice if this could be put in in some form as a useability improvement.
For people who regularly change proxies this is going to be useful and popular.


For what its worth I prefer patch attachment 83491 [details] [diff] [review] but its entirely up to the
experts.

David

Comment 23

[David Illsley]

Attachment: The inevitably requested screenshot

Comment 24

[benc]

To increase the usability, we need to know what the real issues are.

After having to test two releases on 4 platforms (yes, that's functional testing
of 8 proxy pref panes), I think the two bigest items are:

1- Easier data entry for manual (this bug addressed that directly).
2- Better support for PAC URLs (longer display line, maybe a PAC history stored
in a menu.
3- Possible deletion of the "reload" button, which may occur everytime you close
prefs.

If you are interested in making these (or other changes), lets create a new bug
that is a "post mozilla 1.0" bug for discussion. Existing bugs should be focused
on specific problems <-> solutions.

Comment 25

[neeti]

-->david

Comment 26

[David Illsley]

neeti: thanks, I think :-)

benc: there is a patch here (attachment 84346 [details] [diff] [review]) which addresses your point 1
which is what this bug is about. It meets the UI spec from mpt without
reordering the elements or otherwise screwing up the layout. I'm using it at the
moment on linux and it seems to work. Where do I go from here? Who is the
yay/nay contact?

As for 2,3 I agree, but definately a different bug.

Thanks,
David

Comment 27

[Matthias Versen [:Matti]]

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

Comment 28

[Matthias Versen [:Matti]]

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

Comment 29

[David Illsley]

Well the distinct lack of a coherent decision making process means that I
dropped this a couple of months ago - I also no longer use a proxy :-). The
patches are here and probably still work so anyone else is more than welcome to
try and drive this through

David

Assigning to [e-mail]

Comment 30

[benc]

+nsbeta1

neeti: this bug was up for review, and it is a good incremental improvement in
our proxy configs. What is the next step? 

Comment 31

[Darin Fisher]

cc neeti (see comment #30)

Comment 32

[timeless]

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

Comment 33

[Samir Gehani]

adt: nsbeta1-

Comment 34

[Matthias Versen [:Matti]]

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

Comment 35

[Jerry]

what is the status of this bug?

from my personal view, I like the screenshot of 
http://bugzilla.mozilla.org/attachment.cgi?id=83335&action=view.

Comment 36

[Vidar Haarr (not reading bugmail)]

I like attachment 84348 [details] alot better. See comment #13 for reasons.
Anyway.. nobody is working on this at the moment. See comment #29.

Comment 37

[Jerry]

I take it

Comment 38

[Jerry]

Attachment: patch

based on patch 84346

Comment 39

[Jerry]

Comment on attachment 135236 [details] [diff] [review]
patch 

neil, can you review it?

Comment 40

[[e-mail]]

Comment on attachment 135236 [details] [diff] [review]
patch 

>+            <spacer />
Nit: this is XML, you don't need a space before /

>+            <hbox align="center">
>+              <button id="reuseProxy" label="&reuseProxy.label;" accesskey="&reuseProxy.accesskey;" oncommand="DoProxyCopy();"
>+                    flex="0"/>
You don't need flex="0"

>+              <spacer flex="1"/>
You don't need this spacer

>+    var http = document.getElementById("networkProxyHTTP");
>+    var ftp = document.getElementById("networkProxyFTP");
>+    var gopher = document.getElementById("networkProxyGopher");
>+    var socks = document.getElementById("networkProxySOCKS");
>+    var ssl = document.getElementById("networkProxySSL");
>+    var hostnames = [ ssl, ftp, gopher, socks ];
>+    if (http.value)
This if should go before the variables.

>+       for (var i=0;i<hostnames.length;i++)
Nit: Spaces around = and < operators and after ;

>+           if (!hostnames[i].value)
I think you should always overwrite.

>+    var portNums = [ sslPort, ftpPort, gopherPort, socksPort ];
The socks port should not be the same as any other port.

>+<!ENTITY  reuseProxy.label              "Use For All Protocols">
Hmmm... "Copy to all protocols", perhaps?

Comment 41

[Jerry]

Attachment: patch v2

thanks for neil's review.
I modified the patch according to neil's suggestion.
in js, I skip copy to socks, since socks proxy setting is quite different from
other proxy settings.

I still keep one spacer to locate the button correctly.
I saw many usage of spacer in mozilla.
http://lxr.mozilla.org/seamonkey/search?string=spacer

neil, can you review it again?

Comment 42

[[e-mail]]

Comment on attachment 135877 [details] [diff] [review]
patch v2

Better, but still not quite right...

>+            <spacer/>
Sorry for the confusion, I didn't mean for you to delete this, I'm glad you
figured it out.

>+              <button id="reuseProxy" label="&reuseProxy.label;" accesskey="&reuseProxy.accesskey;" oncommand="DoProxyCopy();" />
I think you should disable the button if the proxy setting isn't manual.

>+  if (http.value && httpPort.value) {
The thing is, the value defaults to "0" - you need to use &&
parseInt(httpPort.value) to make sure the value is valid before you copy it.

>+    var hostNames = [ ssl, ftp, gopher ];
>+    var portNums = [ sslPort, ftpPort, gopherPort ];
>+    for (var i = 0; i < portNums.length; i++) {
>+      portNums[i].value = httpPort.value;
>+      hostNames[i].value = http.value;
>+    }
This is hardly worth a loop, you might as well copy each item manually.
Alternatively, put the document.getElementById into the loop, so that your
arrays only contain the elemnent id strings, and also declare them const.

You might also want to copy httpPort.value and http.value to temporary
variables.

>+<!ENTITY  reuseProxy.label              "Copy to all protocols except socks">
Use these settings for SSL, FTP and Gopher
>+<!ENTITY  reuseProxy.accesskey          "c">
Make sure your access key matches the letter you want to underline. This
actually underlined the "c" of protocols.

Comment 43

[Jerry]

Attachment: patch v3

a updated version.

Comment 44

[[e-mail]]

Comment on attachment 135971 [details] [diff] [review]
patch v3

>+<!ENTITY  reuseProxy.label              "Use these settings for SSL, FTP and Gopher">
>+<!ENTITY  reuseProxy.accesskey          "e">
r=me, although I'd prefer to use U here.

Comment 45

[Alex Bishop]

> Use these settings for SSL, FTP and Gopher

Given the position of the button, I think 'Use HTTP settings for SSL, FTP and
Gopher' would be less ambiguous.

Comment 46

[Jerry]

check in on 12/23, fixed.

Comment 47

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

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