Last Comment Bug 570970 - Move the Link Behaviour preferences from the tabs pane to a separate pane.
: Move the Link Behaviour preferences from the tabs pane to a separate pane.
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a3
Assigned To: Philip Chee
:
:
Mentors:
Depends on: 574609
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-09 08:16 PDT by Philip Chee
Modified: 2010-07-26 06:04 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 links pane. (19.29 KB, patch)
2010-06-09 08:53 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Patch v1.1 Access Keys (19.61 KB, patch)
2010-06-16 07:58 PDT, Philip Chee
iann_bugzilla: review+
neil: superreview+
iann_bugzilla: feedback+
Details | Diff | Splinter Review
Links Prefs as menulists. (31.38 KB, image/png)
2010-06-18 02:56 PDT, Philip Chee
iann_bugzilla: feedback-
Details
Prefpane Screenshot: Tweak description. (25.77 KB, image/png)
2010-06-22 02:30 PDT, Philip Chee
iann_bugzilla: review-
Details
[for checkin] Patch v1.2 r=IanN sr=Neil (19.25 KB, application/octet-stream)
2010-06-24 21:17 PDT, Philip Chee
philip.chee: review+
philip.chee: superreview+
Details

Description Philip Chee 2010-06-09 08:16:10 PDT
The current tabs preferences pane is overcrowded and in some themes is clipped at the bottom. We can make it less congested by moving the link behaviour preferences to a separate pane.
Comment 1 Philip Chee 2010-06-09 08:53:13 PDT
Created attachment 450131 [details] [diff] [review]
Patch v1.0 links pane.

>diff --git a/suite/common/pref/preferences.xul b/suite/common/pref/preferences.xul
>+          <treeitem id="linksItem"
>+                    label="&links.label;"
>+                    prefpane="links_pane"
>+                    helpTopic="navigator_pref_tabbed_browsing"

Note to self: file a followup help bug.
Comment 2 Ian Neal 2010-06-15 13:43:14 PDT
Comment on attachment 450131 [details] [diff] [review]
Patch v1.0 links pane.

In principle I am happy with this but I'd like some better accesskeys in the two resultant pref panes now there is more choice.
Comment 3 Philip Chee 2010-06-16 07:58:36 PDT
Created attachment 451589 [details] [diff] [review]
Patch v1.1 Access Keys

> Ian Neal      2010-06-15 13:43:14 PDT

> (From update of attachment 450131 [details] [diff] [review])
> In principle I am happy with this but I'd like some better accesskeys in the
> two resultant pref panes now there is more choice.

I changed about two access keys; the rest of the keys are pretty much optimized.

Tabbed Browsing->Open tabs instead of windows. These access keys come from platformCommunicatorOverlay.dtd and I don't want to change anything in case I break something.

Link Behaviour. The first group uses the first available consonant in (current|tab|window). The second group uses the first available vowel in (current|tab|window).
Comment 4 Ian Neal 2010-06-16 13:26:27 PDT
Comment on attachment 451589 [details] [diff] [review]
Patch v1.1 Access Keys

>+++ b/suite/locales/en-US/chrome/common/pref/pref-links.dtd
>@@ -0,0 +1,17 @@
>+<!ENTITY linksHeader.label "Link Behaviour">
>+<!ENTITY newWindow.label "Link open behavior">
>+<!ENTITY newWindowDescription.label "Open links meant to open a new window in">
>+<!ENTITY external.label "Links from other applications">
>+<!ENTITY externalDescription.label "Open links passed from other applications in">
>+
>+<!ENTITY openCurrent.label "The current tab/window">
>+<!ENTITY newWindowGroupCurrent.accesskey "c">
>+<!ENTITY externalGroupCurrent.accesskey "u">
>+
>+<!ENTITY openTab.label "A new tab in the current window">
>+<!ENTITY newWindowGroupTab.accesskey "t">
>+<!ENTITY externalGroupTab.accesskey "a">
Shouldn't this be "A"?
>+
>+<!ENTITY openWindow.label "A new window">
>+<!ENTITY newWindowGroupWindow.accesskey "w">
>+<!ENTITY externalGroupWindow.accesskey "o">
What about using "n" for one of these?

r=me with those addressed.
Comment 5 Robert Kaiser 2010-06-16 18:17:48 PDT
Comment on attachment 451589 [details] [diff] [review]
Patch v1.1 Access Keys

I'm not really fond of this, as I think that we actually should and could win space by merging the UI prefs for internal and external links, which I believe we will need to do when we make all tabs in all components the same - but then, we are not there yet...
Comment 6 Philip Chee 2010-06-17 01:28:21 PDT
Comment on attachment 451589 [details] [diff] [review]
Patch v1.1 Access Keys

> >+<!ENTITY openTab.label "A new tab in the current window">
> >+<!ENTITY newWindowGroupTab.accesskey "t">
> >+<!ENTITY externalGroupTab.accesskey "a">
> Shouldn't this be "A"?

> >+<!ENTITY openWindow.label "A new window">
> >+<!ENTITY newWindowGroupWindow.accesskey "w">
> >+<!ENTITY externalGroupWindow.accesskey "o">
> What about using "n" for one of these?

> r=me with those addressed.

See Comment 3.

Normally this would be optimal. However the way I reverse engineered the current access keys is that the key words are (current|tab|window) so the first group uses the first available consonant from these words. And the second group selects the first available vowel.

> What about using "n" for one of these?
The "n" ends up on _n_ew and not wi_n_dow.
Comment 7 neil@parkwaycc.co.uk 2010-06-17 05:49:40 PDT
Why not just change the radiobuttons into a menulist?
+- Link open behaviour -----------------------------+
| Open links meant to open a new window in          |
|    [  A new window                     | V ]      |
| Open links passed from other applications in      |
|    [  A new tab in the current window  | V ]      |
+---------------------------------------------------+
Comment 8 Philip Chee 2010-06-17 06:18:53 PDT
Hmm. Good Idea. This means I don't have to split the pref-tabs and I can get rid of all those pesky access keys.
Comment 9 Philip Chee 2010-06-18 02:48:28 PDT
> Why not just change the radiobuttons into a menulist?
Actually this doesn't seem to save any significant vertical space.
Comment 10 Philip Chee 2010-06-18 02:56:42 PDT
Created attachment 452223 [details]
Links Prefs as menulists.

>> Why not just change the radiobuttons into a menulist?
> Actually this doesn't seem to save any significant vertical space.

I think moving the links to a separate prefpane as in patch v1.1 is still a good idea as this frees up some space for newer tabbrowser prefs e.g. those features Misak is working on. Neil? IanN?
Comment 11 neil@parkwaycc.co.uk 2010-06-18 05:10:27 PDT
(In reply to comment #10)
> I think moving the links to a separate prefpane as in patch v1.1 is still a
> good idea as this frees up some space for newer tabbrowser prefs e.g. those
> features Misak is working on. Neil? IanN?
Hmm, there are a number of tab-related prefs in browser-prefs.js which we don't expose yet. It would also give us a chance to add the diversion restriction.
Comment 12 Jens Hatlak (:InvisibleSmiley) 2010-06-18 05:26:56 PDT
(In reply to comment #11)
> It would also give us a chance to add the diversion restriction.

xref Bug 378213
Comment 13 Ian Neal 2010-06-18 06:23:36 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > I think moving the links to a separate prefpane as in patch v1.1 is still a
> > good idea as this frees up some space for newer tabbrowser prefs e.g. those
> > features Misak is working on. Neil? IanN?
> Hmm, there are a number of tab-related prefs in browser-prefs.js which we don't
> expose yet. It would also give us a chance to add the diversion restriction.

Indeed, is it worth rolling up into this patch?
Comment 14 Ian Neal 2010-06-18 06:27:00 PDT
Comment on attachment 452223 [details]
Links Prefs as menulists.

I prefer a separate pref panel
Comment 15 Ian Neal 2010-06-18 06:30:30 PDT
Comment on attachment 451589 [details] [diff] [review]
Patch v1.1 Access Keys

Having had another look at the wording, I think with a few tweaks could simplify things. "other applications" being repeated for example.
Comment 16 Philip Chee 2010-06-18 10:29:15 PDT
> Having had another look at the wording, I think with a few tweaks could
> simplify things. "other applications" being repeated for example.

Suggestions for wording?

+<!ENTITY newWindow.label "Link open behavior">
+<!ENTITY newWindowDescription.label "Open links meant to open a new window in">
+<!ENTITY external.label "Links from other applications">
+<!ENTITY externalDescription.label "Open links passed from other applications in">

How about: "Open external links in" ?
Comment 17 Jens Hatlak (:InvisibleSmiley) 2010-06-18 12:13:44 PDT
(In reply to comment #16)
> +<!ENTITY newWindow.label "Link open behavior">
> (...)
> +<!ENTITY external.label "Links from other applications">
> 
> How about: "Open external links in" ?

Or to make it more in line with the other box: "External link open behavior"?
Comment 18 Philip Chee 2010-06-18 12:50:20 PDT
>> How about: "Open external links in" ?
> Or to make it more in line with the other box: "External link open behavior"?

I'm talking about this entity:
+<!ENTITY externalDescription.label "Open links passed from other applications in">
Comment 19 Jens Hatlak (:InvisibleSmiley) 2010-06-18 12:59:27 PDT
(In reply to comment #18)
> >> How about: "Open external links in" ?
> > Or to make it more in line with the other box: "External link open behavior"?
> 
> I'm talking about this entity:
> +<!ENTITY externalDescription.label "Open links passed from other applications
> in">

Ah, I see. Ian's comment left it open (on purpose?) whether to change the label or description. I'm still unsure which is better to be changed: "external links" is a bit too vague for my taste but as long as there is a reference to "other applications" in either the label or description I guess it's OK.

BTW: You have 2x "Behaviour" (BE) in your patch, and in this bug's summary. ;-)
Comment 20 Philip Chee 2010-06-22 02:30:19 PDT
Created attachment 453028 [details]
Prefpane Screenshot: Tweak description.

> Having had another look at the wording, I think with a few tweaks could
> simplify things. "other applications" being repeated for example.

---------------------------------------
From: http://kb.mozillazine.org/Browser.link.open_external

1 Open links from external programs in the current window.
2 Open links from external programs in a new window. (Default in SeaMonkey)
3 Open links from external programs in new tabs in an existing window. (Default in Firefox)

---------------------------------------
From: Firefox 3.0

mozilla/browser/components/preferences/tabs.js

* browser.link.open_external
* - determines where pages opened by external applications are opened:
*     0 opens such links in the default window,
*     1 opens such links in the most recent window or tab,
*     2 opens such links in a new window,
*     3 opens such links in a new tab

<!ENTITY newPagesIn.label             "New pages should be opened in:">
---------------------------------------
In the attached screenshot, I changed the description in the second group from:

Open links passed from other applications in
to
New Pages sent from other programs should open in
Comment 21 Ian Neal 2010-06-22 17:31:58 PDT
Comment on attachment 451589 [details] [diff] [review]
Patch v1.1 Access Keys

I've just been thinking ahead to if/when bug 378213 gets fixed and what is the best layout to allow for that. For the moment I am happy to go with the current patch. What do you think about adding all four options or would that be too confusing?
Comment 22 Ian Neal 2010-06-22 17:38:28 PDT
Comment on attachment 453028 [details]
Prefpane Screenshot: Tweak description.

I was thinking of changing the groupbox caption. For example for the external links one, to "When links are passed from other applications open in" and removing the description. Though, as I said, if we're looking at how the layout will change when other prefs are added, it might be better to leave those as they are.
Comment 23 Philip Chee 2010-06-23 04:00:45 PDT
Comment on attachment 451589 [details] [diff] [review]
Patch v1.1 Access Keys

> I've just been thinking ahead to if/when bug 378213 gets fixed and what is the
> best layout to allow for that. For the moment I am happy to go with the current
> patch.

Hmm.

> // 0: no restrictions - divert everything
> // 1: don't divert window.open at all
> // 2: don't divert window.open with features
> pref("browser.link.open_newwindow.restriction", 0);

Looking at:

http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsContentTreeOwner.cpp#877

> What do you think about adding all four options or would that be too
> confusing?

I think this needs some thought as to the UI. So I prefer that the restriction preference be done in a separate patch. Lets get the UI in this patch into the tree first.
Comment 24 neil@parkwaycc.co.uk 2010-06-23 04:24:12 PDT
Comment on attachment 451589 [details] [diff] [review]
Patch v1.1 Access Keys

>-            
>+
Please try to avoid random whitespace changes. This one is OK though.

>-   </prefpane>
>+  </prefpane>
As is this.

>+                    helpTopic="navigator_pref_tabbed_browsing"
File a bug on a new help topic?

>new file mode 100644
This isn't a new file, it's a copy of pref-tabs.dtd with some lines deleted.
(pref-links.xul probably looks too different to pref-tabs.xul though.)

>+<!ENTITY links.label "Link Behaviour">
Needs to be US spelling, unfortunately :-(
Comment 25 neil@parkwaycc.co.uk 2010-06-23 04:26:48 PDT
(In reply to comment #20)
> From: Firefox 3.0
> 
> mozilla/browser/components/preferences/tabs.js
> 
> * browser.link.open_external
> * - determines where pages opened by external applications are opened:
> *     0 opens such links in the default window,
> *     1 opens such links in the most recent window or tab,
> *     2 opens such links in a new window,
> *     3 opens such links in a new tab
Actually 0 makes zero sense here, as this is the pref used when the API is passed a zero value. 1 is the default default ;-)
Comment 26 Philip Chee 2010-06-24 21:17:43 PDT
Created attachment 453962 [details]
[for checkin] Patch v1.2 r=IanN sr=Neil

> (From update of attachment 451589 [details] [diff] [review])
> >-            
> >+
> Please try to avoid random whitespace changes. This one is OK though.
Fixed.

> >-   </prefpane>
> >+  </prefpane>
> As is this.
Fixed.

> >+                    helpTopic="navigator_pref_tabbed_browsing"
> File a bug on a new help topic?
Will do.

> >new file mode 100644
> This isn't a new file, it's a copy of pref-tabs.dtd with some lines deleted.
> (pref-links.xul probably looks too different to pref-tabs.xul though.)
Now using hg cp

> >+<!ENTITY links.label "Link Behaviour">
> Needs to be US spelling, unfortunately :-(
Fixed.
Comment 27 Philip Chee 2010-06-24 21:35:22 PDT
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/9fb2b78003f3
Comment 28 Giacomo Magnini 2010-06-25 02:34:34 PDT
Help changes are needed: here or in another bug?

Note You need to log in before you can comment on or make changes to this bug.