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

VERIFIED FIXED

Status

()

defect
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: leon.sha, Assigned: leon.sha)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 10 obsolete attachments)

62.89 KB, image/png
Details
14.81 KB, patch
neil
: review+
Details | Diff | Splinter Review
1.10 KB, patch
Details | Diff | Splinter Review
1.26 KB, patch
neil
: review+
mscott
: superreview+
Details | Diff | Splinter Review
Assignee

Description

16 years ago
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.
Assignee

Comment 1

16 years ago
Assing to sun browser team
Assignee: darin → browser-china
Assignee

Comment 2

16 years ago
Posted image snapshot
Assignee

Comment 3

16 years ago
Posted patch patch for trunk (obsolete) — Splinter Review
Add a popupmenu to status bar.

Comment 4

16 years ago
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).
Attachment #134743 - Flags: review-
Comment on attachment 134743 [details] [diff] [review]
patch for trunk

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

Comment 6

16 years ago
Attachment #134743 - Attachment is obsolete: true
Assignee

Comment 7

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

Timeless, can you help to review?
Thans a lot.
Attachment #135896 - Flags: review?(timeless)

Comment 8

16 years ago
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.
Attachment #135896 - Flags: review?(timeless) → review-
Assignee

Comment 9

16 years ago
>- 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?
Assignee: browser-china → leon.sha
Assignee

Comment 10

16 years ago
Posted patch proposed patch (obsolete) — Splinter Review
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.
Attachment #135896 - Attachment is obsolete: true
Assignee

Updated

16 years ago
Attachment #136335 - Flags: review?(borggraefe)
Assignee

Comment 11

16 years ago
Posted patch patch_v3 (obsolete) — Splinter Review
Remove some tabs in patch_v2.
Still no images for themes.
Attachment #136335 - Attachment is obsolete: true
Assignee

Comment 12

16 years ago
Comment on attachment 136337 [details] [diff] [review]
patch_v3

Stefan, can you help to review this patch?
Thanks a lot.
Attachment #136337 - Flags: review?(borggraefe)

Comment 13

16 years ago
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...".
Attachment #136337 - Flags: review?(borggraefe) → review?(timeless)

Updated

16 years ago
Attachment #136335 - Flags: review?(borggraefe)
Assignee

Comment 14

16 years ago
Posted patch patch_v3.1 (obsolete) — Splinter Review
>>+    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.
Assignee

Updated

16 years ago
Attachment #136337 - Attachment is obsolete: true
Assignee

Updated

16 years ago
Attachment #136396 - Flags: review?(timeless)

Comment 15

16 years ago
Comment on attachment 136337 [details] [diff] [review]
patch_v3

Removing review request from obsolete patch.
Attachment #136337 - Flags: review?(timeless)
Assignee

Comment 16

16 years ago
Posted patch Add handler for lock. (obsolete) — Splinter Review
Assignee

Updated

16 years ago
Attachment #136396 - Attachment is obsolete: true
Assignee

Updated

16 years ago
Attachment #136396 - Flags: review?(timeless)
Assignee

Updated

16 years ago
Attachment #137098 - Flags: review?(timeless)
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)?
Assignee

Comment 18

16 years ago
Posted patch New patch (obsolete) — Splinter Review
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.
Attachment #137098 - Attachment is obsolete: true
Assignee

Updated

16 years ago
Attachment #137649 - Flags: review?(neil.parkwaycc.co.uk)
Assignee

Updated

16 years ago
Attachment #137098 - Flags: review?(timeless)
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).
Attachment #137649 - Flags: review?(neil.parkwaycc.co.uk) → review-
Assignee

Comment 20

16 years ago
Posted patch Patch with neil's comments. (obsolete) — Splinter Review
>+  <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?
Assignee

Updated

16 years ago
Attachment #137649 - Attachment is obsolete: true
Assignee

Updated

16 years ago
Attachment #137691 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #137691 - Flags: review?(neil.parkwaycc.co.uk) → review-
Assignee

Comment 22

16 years ago
Posted patch Patch again. (obsolete) — Splinter Review
I have tried neil's comments.
It realy doesn't work. If you usr setAttribute("checked", "true"), the other
button will not be unchecked.
Attachment #137691 - Attachment is obsolete: true
Assignee

Updated

16 years ago
Attachment #137864 - Flags: review?(neil.parkwaycc.co.uk)
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").
Attachment #137864 - Flags: review?(neil.parkwaycc.co.uk) → review-
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.
Attachment #137691 - Flags: review- → review+
Assignee

Comment 25

16 years ago
I filed a new bug that radio buttons don't uncheck on a context menu.
Now using onpopupshown as workaround.
Attachment #137864 - Attachment is obsolete: true
Assignee

Updated

16 years ago
Attachment #138095 - Flags: superreview?(jst)
Comment on attachment 138095 [details] [diff] [review]
Patch with neil's comments

sr=jst
Attachment #138095 - Flags: superreview?(jst) → superreview+

Updated

16 years ago
Attachment #138095 - Flags: review+

Comment 27

16 years ago
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

16 years ago
./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

16 years ago
just a one-sided take from building tbird, which is broken due to the checkins
here on how to fix it.
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

16 years ago
I agree, now that you've mentioned it. I'm just still too new at making changes
in the cvs. Thanks.
As per Neil's suggestion.
Attachment #138252 - Attachment is obsolete: true

Comment 33

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

Comment 34

16 years ago
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

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

Comment 36

16 years ago
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 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.
Attachment #138559 - Flags: review+

Comment 38

16 years ago
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...
Attachment #138559 - Flags: superreview?(mscott)
Attachment #138559 - Flags: review?(neil.parkwaycc.co.uk)

Comment 39

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

thanks for the fix
Attachment #138559 - Flags: superreview?(mscott) → superreview+

Comment 40

16 years ago
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!
Attachment #138559 - Flags: review?(neil.parkwaycc.co.uk)
checked in attachment 138559 [details] [diff] [review]
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Andreas pointed out to me that the error only happens in some windows...

Comment 43

16 years ago
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.
Status: RESOLVED → VERIFIED
Summary: Add "Net config" selector → Proxy: add proxy mode selector (via right-click) to Online-Offline button
Whiteboard: checkwin, checklinux

Comment 44

16 years ago
*** Bug 112421 has been marked as a duplicate of this bug. ***
*** Bug 232085 has been marked as a duplicate of this bug. ***

Comment 46

15 years ago
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.
Whiteboard: checkwin, checklinux
(that's bug 243624 it seems, for those who want to know)
You need to log in before you can comment on or make changes to this bug.