223908 - Proxy: add proxy mode selector (via right-click) to Online-Offline button

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Leon Sha]

Similar to Bug 88217.
We can add "Net config" selector beside "offline/online" button.
This will be a sync button of proxy settings.
We can pop up a menu to select the proxy type.
It is easy for users to switch from one proxy type to another, especially for
labtob users.

Comment 1

[Leon Sha]

Assing to sun browser team

Comment 2

[Leon Sha]

Attachment: snapshot

Comment 3

[Leon Sha]

Attachment: patch for trunk

Add a popupmenu to status bar.

Comment 4

[timeless]

Comment on attachment 134743 [details] [diff] [review]
patch for trunk

No.

Consider making the online status icon indicate online(direct), online(PAC),
online(proxied), offline.

To switch to one of the proxy states one would right click the online indicator
and get a popup menu which lists the available states (direct, PAC, proxied,
offline).

Comment 5

[[e-mail]]

Comment on attachment 134743 [details] [diff] [review]
patch for trunk

We have <statusbarpanel type="menu"> these days.

Comment 6

[Leon Sha]

Attachment: Patch with timeless's comment.

Comment 7

[Leon Sha]

Comment on attachment 135896 [details] [diff] [review]
Patch with timeless's comment.

Timeless, can you help to review?
Thans a lot.

Comment 8

[Stefan Borggraefe]

Comment on attachment 135896 [details] [diff] [review]
Patch with timeless's comment.

Timeless, Neil and I discussed this patch on IRC. Timeless asked me to minus
this patch and add some comments to it. So here we go. :-)

>+    <statusbarpanel class="statusbarpanel-iconic" id="offline-status" contex="networkProperties"/>

This should be context=

Also this should get an extra attribute "proxy" so the new online states will
be themable like this:

#offline-status[proxy="direct"]
#offline-status[proxy="manual"]
#offline-status[proxy="auto"]
#offline-status[offline="true"]

>+function setNetworkStatus()
>+{
>+  var networkStatus = document.getElementById("networkProperties");
>+  if ( !networkStatus ) 

Please remove the blanks in the condition, so this is consistent with the rest
of this function at least. ;-)

>+  else
>+  {
>+    ioService.offline = false;
>+    try {

Let the else and the { be on the same line for consistency.

>+  <popup id="networkProperties" type="menu">
>+      <menuitem type="radio" name="status" value="3" label="&offline.label;" oncommand="setNetworkStatus()"/>
>+      <menuitem type="radio" name="status" value="0" label="&direct.label;" oncommand="setNetworkStatus()"/>
>+      <menuitem type="radio" name="status" value="1" label="&manual.label;" oncommand="setNetworkStatus()"/>
>+      <menuitem type="radio" name="status" value="2" label="&pac.label;" oncommand="setNetworkStatus()"/>
>+  </popup>

Please indent with 2 spaces so this is consistent with the rest of the file.
Please add access keys.

>+<!ENTITY offline.label                                  "offline">  
>+<!ENTITY direct.label                                   "online(direct)">  
>+<!ENTITY manual.label                                   "online(manual)">  
>+<!ENTITY pac.label                                      "online(pac)">  

Please remove the blanks at the and of the lines. Also I think the texts could
be more user-friendly. What about:

Offline
Online (Use direct connection)
Online (Use manually configured proxies)
Online (Use automatically configured proxies)


Some more ideas you might address in this or in follow-up bugs

- the actual theming

- it would be nice if the manual/automatic menu item is greyed out when no
proxy for this option is configured. Since currently you can select a
not-configured proxy via the "Proxies" panel in Preferences, this requires some
more work I guess.

- add a menu item to open the "Proxies" panel in Preferences to the menu:

Offline
Online (Use direct connection)
Online (Use manually configured proxies)
Online (Use automatically configured proxies)
---------------------------------------------
Proxy configuration...

- make a submenu out of "File|Work Offline" so the same menu items are
available there.

Comment 9

[Leon Sha]

>- make a submenu out of "File|Work Offline" so the same menu items are
>available there.

I think it should be synchronous with left click of status bar.
If we add such a sub menu, should we remove the left click. Or we just let the
"File|Work Offline" menu items be synchronous with right click and remain the
left click?

Comment 10

[Leon Sha]

Attachment: proposed patch

I don't know how to prepare the theme image.
Should I draw it by myself, or I just need to provide the requirement to
someone.

Comment 11

[Leon Sha]

Attachment: patch_v3

Remove some tabs in patch_v2.
Still no images for themes.

Comment 12

[Leon Sha]

Comment on attachment 136337 [details] [diff] [review]
patch_v3

Stefan, can you help to review this patch?
Thanks a lot.

Comment 13

[Stefan Borggraefe]

Comment on attachment 136337 [details] [diff] [review]
patch_v3

Please understand: I'm no real reviewer. I just added some comments to your
last patch and minused it because timeless asked me to do so.  I tested your
new patch once again, so I'm going to add some comments. But you need to get a
review from a real reviewer in the end. Because of this I'm setting r? for
timeless for this patch, because he may have some more comments/other ideas
about this patch and he is a real reviewer.

> I don't know how to prepare the theme image.
> Should I draw it by myself, 

This would be great, if you have the artistic skills. Just attach them to the
bug.

> or I just need to provide the requirement to
> someone.

I don't know how paints new pictures for classic/modern nowadays.

>+function enableProxyMenu()
>+{
>+  var i = 0;
>+  var element;

Variable "element" is never used. I think it is better to declare the counter
"i" directly in the for statement.

>+  var proxyManualConfigured = "false";

I'm no native English speaker, too, but I think proxyManuallyConfigured would
be grammatically correct here.

>+
>+  //Checking for proxy manual settings.
>+  for (i = 0; i < kProxyManual.length; i++) {
>+    if(GetStringPref(kProxyManual[i]) != "") {

Nit: Please add a blank after the if keyword.

>+      proxyManualConfigured = "true";
>+      break;
>+    }
>+  }
>+  enableElement("network_Manual", proxyManualConfigured)
>+
>+  //Checking for proxy PAC settings.
>+  var proxyPacConfigured = "false";
>+  if(GetStringPref("network.proxy.autoconfig_url") != "")

Nit: Here, too. ;-)

>+    proxyPacConfigured = "true";
>+
>+  enableElement("network_Pac", proxyPacConfigured)
>+}

While this works, you can still reach the disabled menu items via the "Proxies"
pane in Preferences. :-(

>+
>+function enableElement(name, value)
>+{
>+  var element = document.getElementById(name);
>+  if (!element) return;
>+
>+  if (value == "false")
>+    element.setAttribute("disabled", "true");
>+  else
>+    element.removeAttribute("disabled");
>+}
>+
> 
> function setOfflineUI(offline)

Nit: One blank line is enough.

>+  <broadcaster id="network_Proxy_Settings"
>+               label="&proxy.label;"
>+               accesskey="&proxy.accesskey;"
>+               oncommand="goPreferences('advanced_pref_proxies',
>+                                        'chrome://communicator/content/pref/pref-proxies.xul',
>+                                        'advanced_pref_proxies');"/>

This doesn't select the Proxies item pane in the Preferences tree. The first
parameter has to be "advancedItem" in order to get this to work. Furthermore
the treeitem has not the id advanced_pref_proxies.

>+<!ENTITY  networkStatus.label                 "Network Status">
>+<!ENTITY  networkStatus.accesskey             "n">

This accesskey conflicts with the one for the "New" submenu.

>+<!ENTITY offline.label                                  "offline">
>+<!ENTITY offline.accesskey                              "o">
>+<!ENTITY direct.label                                   "online(proxy: no)">
>+<!ENTITY direct.accesskey                               "n">
>+<!ENTITY manual.label                                   "online(proxy: manual)">
>+<!ENTITY manual.accesskey                               "m">
>+<!ENTITY pac.label                                      "online(proxy: pac)">
>+<!ENTITY pac.accesskey                                  "p">

The words of the menu items should all start with capital letters. PAC should
be all in capital letters, because it's an acronym. But I would object against
using the term "PAC" here anyway, because it's used nowhere else in Mozilla and
I believe an average user might not know what it means.
There should be a blank between "online" and the opening parenthesis.

>+<!ENTITY proxy.label                                      "Proxy Configuration">
>+<!ENTITY proxy.accesskey                                  "C">

This menu item should have an ellipsis: "Proxy Configuration...".

Comment 14

[Leon Sha]

Attachment: patch_v3.1

>>+    proxyPacConfigured = "true";
>>>+
>>>+  enableElement("network_Pac", proxyPacConfigured)
>>>+}
>
>
>While this works, you can still reach the disabled menu items via the
"Proxies"
>pane in Preferences. 

It will conflict with the actions now.
For example, now if you select direct connection in "Proxies" panel in
preferences, all the proxy settings in manual configuration will be disabled.
If you also disabled the radio button of manual configuration, we can not set
manual configuration anymore.

Comment 15

[Stefan Borggraefe]

Comment on attachment 136337 [details] [diff] [review]
patch_v3

Removing review request from obsolete patch.

Comment 16

[Leon Sha]

Attachment: Add handler for lock.

Comment 17

[[e-mail]]

Comment on attachment 137098 [details] [diff] [review]
Add handler for lock.

>+    <popup id="networkProperties" type="menu"/>
Does type="menu" do anything? Also, doesn't this belong in the popupset, rather
than in the statusbar?

>+    <broadcaster id="network_Offline"/>
>+    <broadcaster id="network_Direct"/>
>+    <broadcaster id="network_Manual"/>
>+    <broadcaster id="network_Pac"/>
>+    <broadcaster id="network_Proxy_Settings"/>
If possible, these should be <command>s instead. Oh I remember, checked
settings don't work for commands in a popup. Sigh... you could of course just
tweak the checked setting on the menuitem when the popup is opened...

>+  var proxyLocked = prefBranch.prefIsLocked("network.proxy.type");
>+
>+  if (proxyLocked) {
>+      for (var i = 0; i < kOnlineStatus.length; i++)
>+        kOnlineStatus[i].setAttribute("disabled", "true");
>+  }
>+  else {
>+      for (var i = 0; i < kOnlineStatus.length; i++)
>+        kOnlineStatus[i].removeAttribute("disabled");
>+  }
You only need to do the direct setting, because you do manual and auto below
anyway. Actually, you should update the menuitems on the onpopupshowing of the
menupopup, rather than when the prefs change. Of course you still need to
update the tooltip when the proxy type changes.

>+  pref.addObserver("network.proxy", offlineObserver, false);
Looks like an undeclared variable to me.

>+<!ENTITY direct.label                                   "Online (proxy: no)">
(proxy: none) perhaps? Or maybe (proxy: off)?

Comment 18

[Leon Sha]

Attachment: New patch

I have done the following changes,

1. Leave the situation of online/offline as before.
    The online/offline status can only be controlled by "file-->work offline"
and left click the icon.
2. Only when you are online, you can right click to pop up the proxy menu.
    The reason is "online/offline" status is the connection status, while proxy
setting is the status when online.
    It should be better to separate these two situations to make the setting
clear.
    Also it will make the code more clear.
3. Use tooltips to replace the themes
4. Use onpopupshow to replace the observer of prefs changing.

Comment 19

[[e-mail]]

Comment on attachment 137649 [details] [diff] [review]
New patch

>+  <popupset>
>+    <popup id="networkProperties"/>
>+  </popupset>
Would you mind grouping this with the other popups?

>+function setNetworkStatus(type)
>+{
>+  var element = document.getElementById(type);
>+  if (!element) return;
>+
>+  var networkProxyType = element.getAttribute("value");
This is a bit involved - why not pass the network proxy type in as the
parameter?

>+  prefService = prefService.getService();
>+  prefService = prefService.QueryInterface(Components.interfaces.nsIPrefService);
These should all use
prefService.getService(Components.interfaces.nsIPrefService);

>+  var proxyManuallyConfigured = "false";
>+      proxyManuallyConfigured = "true";
>+  var proxyAutoConfigured = "false";
>+    proxyAutoConfigured = "true";
Please use boolean variables, not strings.

>+  for (var i = 0; i < networkProxyStatus.length; i++)
>+    networkProxyStatus[i].removeAttribute("checked");
This shouldn't be necessary, just set the group attribute on the menuitems so
that they will automatically uncheck each other.

>+function setProxyTypeUI()
You do all sorts of unnecessary work when you are offline. Don't bother calling
the function if you are offline. Or if you can't, then at least check for
offline earlier.

>+  var networkProxyType;
You could declare this in the block that you use it.

>-onlineTooltip=You are online. Click the icon to go offline.
>+onlineTooltip0=You are online (proxy: none). Click the icon to go offline. Right click for details.
>+onlineTooltip1=You are online (proxy: manual). Click the icon to go offline. Right click for details.
>+onlineTooltip2=You are online (proxy: auto). Click the icon to go offline. Right click for details.
I don't like the right click message... it will only work in the browser, not
in the other windows (unless you add the popupset to all of the other windows
too, of course, but it only seems to be useful in the browser).

Comment 20

[Leon Sha]

Attachment: Patch with neil's comments.

>+  <popupset>
>+    <popup id="networkProperties"/>
>+  </popupset>
>Would you mind grouping this with the other popups?
I don't know where is the correct place.
I'd like to put them in navigator.xul or utilityoverlay.xul, so it will be
clear. But I do not find other popupsets.

>+  for (var i = 0; i < networkProxyStatus.length; i++)
>+    networkProxyStatus[i].removeAttribute("checked");
>This shouldn't be necessary, just set the group attribute on the menuitems so
>that they will automatically uncheck each other.
I have tried this. The result is if you just click one radio, the other radio
buttons in the same group will be unchecked. If you set the attribute by
"setAttribute", the other radio buttons will not unchecked. I read the code of
textZoom, they have done the same thing as I do.

>I don't like the right click message... it will only work in the browser, not
>in the other windows (unless you add the popupset to all of the other windows
>too, of course, but it only seems to be useful in the browser).
So is that OK to just leave the right click message in the browser?

Comment 21

[[e-mail]]

Comment on attachment 137691 [details] [diff] [review]
Patch with neil's comments.

>+  for (var i = 0; i < networkProxyStatus.length; i++)
>+    networkProxyStatus[i].removeAttribute("checked");
If you change name="status" to group="status" you don't need this.

>+              name="status"
>+              value="0"
>+              name="status"
>+              value="1"
>+              name="status"
>+              value="2"
I don't think you need value attributes any more.

>+onlineTooltip0=You are online (proxy: none). Click the icon to go offline. Right click for details.
>+onlineTooltip1=You are online (proxy: manual). Click the icon to go offline. Right click for details.
>+onlineTooltip2=You are online (proxy: auto). Click the icon to go offline. Right click for details.
I still don't think it should say "Right click for details" here.

Comment 22

[Leon Sha]

Attachment: Patch again.

I have tried neil's comments.
It realy doesn't work. If you usr setAttribute("checked", "true"), the other
button will not be unchecked.

Comment 23

[[e-mail]]

Comment on attachment 137864 [details] [diff] [review]
Patch again.

Sorry to mess you around on this, you are right to use name="status", then you
really don't need to do the removeAttribute("checked").

Comment 24

[[e-mail]]

Comment on attachment 137691 [details] [diff] [review]
Patch with neil's comments.

>+  for (var i = 0; i < networkProxyStatus.length; i++)
>+    networkProxyStatus[i].removeAttribute("checked");
>+              value="0"
>+              value="1"
>+              value="2"

These lines are unnecessary, but apart from that, r=me.

Comment 25

[Leon Sha]

Attachment: Patch with neil's comments

I filed a new bug that radio buttons don't uncheck on a context menu.
Now using onpopupshown as workaround.

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 138095 [details] [diff] [review]
Patch with neil's comments

sr=jst

Comment 27

[Mickey Stein]

The 30-Dec checkin for this bug somehow moves into thunderbird territory causing
a regression where the toolbar is deformed, and this message is displayed at the
bottom of the screen:

label="&direct.label"
---------------------------

which is from utilityOverlay.* (xul or dtd)

-- backing out these checkins by running these commands 'fixes' the tbird
regression:




cvs update -j1.99 -j1.98
mozilla/xpfe/components/prefwindow/resources/content/preftree.xul
cvs update -j1.5 -j1.4
mozilla/xpfe/communicator/resources/locale/en-US/utilityOverlay.properties
cvs update -j1.26 -j1.25
mozilla/xpfe/communicator/resources/locale/en-US/utilityOverlay.dtd
cvs update -j1.43 -j1.42
mozilla/xpfe/communicator/resources/content/utilityOverlay.xul
cvs update -j1.78 -j1.77
mozilla/xpfe/communicator/resources/content/utilityOverlay.js
cvs update -j1.410 -j1.409 mozilla/xpfe/browser/resources/content/navigator.xul

Comment 28

[Mickey Stein]

./mail/base/locale/utilityOverlay.dtd dated 4-4, is being inserted into the
en-US.jar file. It should be the utilityOverlay.dtd you patched 30-Dec. 

Adding this  (the *+ part) 

*+    locale/en-US/communicator/utilityOverlay.dtd                (resources/loc
ale/en-US/utilityOverlay.dtd)

to the utilityOverlay.dtd line in 

~./mozilla/xpfe/communicator/jar.mn

seems to do the trick.

(or removing utilityOverlay.dtd from mail/base/jar.mn), 

but I don't know the larger picture of this patch being designated "browser", so
some variant of the above should give the right idea.

Comment 29

[Mickey Stein]

Attachment: fixes utilityOverlay.dtd -> en-US.jar problems

just a one-sided take from building tbird, which is broken due to the checkins
here on how to fix it.

Comment 30

[[e-mail]]

Comment on attachment 138252 [details] [diff] [review]
fixes utilityOverlay.dtd -> en-US.jar problems

Surely the way to fix this is to repeat the changes to xpfe's
utilityOverlay.dtd for mail's version.

Comment 31

[Mickey Stein]

I agree, now that you've mentioned it. I'm just still too new at making changes
in the cvs. Thanks.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: apply utilityOverlay.dtd changes to tbird fork

As per Neil's suggestion.

Comment 33

[Scott MacGregor]

I am back online from vacation. I checked in the utilityOverlay.dtd patch dmose
posted for tbird.

Comment 34

[Scott MacGregor]

anyone else get a JS error on startup with this patch:

Error: prefBranch.addObserver is not a function
Source File: chrome://communicator/content/utilityOverlay.js
Line: 513

Comment 35

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

indeed. prefBranch needs to be QI'd to nsIPrefBranchInternal before addObserver
is called.

Comment 36

[Andreas Kunz]

Attachment: patch for the problem mentioned in comment #34 and #35

Patch makes what biesi said in comment #35 (patch dictated by him).
There is a second place where this is missing in function utilityOnUnload. The
patch fixes this also.

Comment 37

[[e-mail]]

Comment on attachment 138559 [details] [diff] [review]
patch for the problem mentioned in comment #34 and #35

I've got no objection to this, but then for some reason I don't see the error
either way, probably some weird XPConnect wrapper caching thingy.

Comment 38

[Andreas Kunz]

Comment on attachment 138559 [details] [diff] [review]
patch for the problem mentioned in comment #34 and #35

Requesting r= from Neil, because he r'ed the original patch and is so
wonderfully responsive	and sr= from mscott because he was the first one to
report the problem...

Comment 39

[Scott MacGregor]

Comment on attachment 138559 [details] [diff] [review]
patch for the problem mentioned in comment #34 and #35

thanks for the fix

Comment 40

[Andreas Kunz]

Comment on attachment 138559 [details] [diff] [review]
patch for the problem mentioned in comment #34 and #35

Oh, Neil gave r= before I requested it... now THAT is responsive!

Comment 41

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

checked in attachment 138559 [details] [diff] [review]

Comment 42

[[e-mail]]

Andreas pointed out to me that the error only happens in some windows...

Comment 43

[benc]

VERIFIED/fixed: Mac OS X, Mozilla 1.7a trunk (20040111)

I've confirmed the selector appears when online, but not offline.

I've confirimed that the selector does change the mode (even while working on a
loadset!).

I've confirmed it correctly sets to all 3 modes.

I've confirmed that Proxy Configuration opens to the config panel.

NOTE:
I've changed the title so that it is more accurate to the implemented behavior.
"Net configs" should be used to discuss a feature where networks live in
sub-profiles in a single profile.

If anyone can verify for Linux and Win, please remove the "checkLinux and "check
Win" in the whiteboard.

Comment 44

[benc]

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

Comment 45

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

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

Comment 46

[benc]

V: all plats.

I've been testing proxy up and down the branch and using this all day long.

I've filed a bug w/ some minor enhancement requests. I'm not sure who should be
cc'd, so I'm commenting here.

Comment 47

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

(that's bug 243624 it seems, for those who want to know)