changing the "Hide tab bar with only one tab" pref is not immediate

VERIFIED FIXED

Status

SeaMonkey
Tabbed Browser
VERIFIED FIXED
16 years ago
10 years ago

People

(Reporter: sairuh (rarely reading bugmail), Assigned: Christopher Aillon (sabbatical, not receiving bugmail))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

16 years ago
seen using 2001.11.28.0x-comm bits on linux rh7.2, mac 10.1.1 and winnt.

0. make sure you have only one tab displayed in the browser window --or if you
have the tab bar closed [default state], make sure you don't have multiple tabs
displayed.
1. open the Preferences dialog and go to the Tabbed Browser panel.
2. toggle the state of the "Hide the tab bar when only one tab is open" checkbox
[turn it off if it's on, or vice versa].
3. click OK to save and dismiss the Prefs dlg.

expected:
if you had the tab bar open previously, the tab bar should go away after
dismissing the Prefs dlg. conversely, if the tab bar was closed, it should
appear after dismissing the Prefs dlg.

actual results:
after dismissing the Prefs dlg, there's no change wrt to the tab bar.

the changes only appear after the following workarounds:
* if the tab bar was previously open, i need to open another tab after
dismissing Prefs, close it, then the tab bar closes.
* if the tab bar was previously closed, i need to open another tab after
dismissing Prefs, close it, then the tab bar remains open.
(Reporter)

Comment 1

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

Comment 2

16 years ago
Reassigning to new component owner.
Assignee: hyatt → jaggernaut

Comment 3

16 years ago
-> future, helpwanted

It shouldn't be too hard to fix this, implement a pref listener similar to the
button pref listener, and make it open/close the bar as appropriate.

cc'ing some people who might want to take a stab at this, or know people who
might want to.
Keywords: helpwanted
Target Milestone: --- → Future
Taking.
Assignee: jaggernaut → caillon
Keywords: helpwanted
Created attachment 71061 [details] [diff] [review]
Proposed fix v1.0
Status: NEW → ASSIGNED
Keywords: patch, review
Target Milestone: Future → ---
Um... What if one uses <browser>, not <tabbrowser>?  That code looks like it
assumes that gBrowser.mTabContainer exists and the like...

Any way to move this code over into tabbrowser.xml?
Created attachment 71252 [details] [diff] [review]
Patch v2.0

/me mutters something about bug 50349...
Attachment #71061 - Attachment is obsolete: true
Comment on attachment 71252 [details] [diff] [review]
Patch v2.0

actually this patch sucks too.	It creates a new object on each access.  I'll
get a better one out.
Attachment #71252 - Attachment is obsolete: true
Created attachment 71277 [details] [diff] [review]
Proposed fix v3.0
Comment on attachment 71277 [details] [diff] [review]
Proposed fix v3.0

>+              this.observe = handler;

This line is not needed -- you set this.observe below....

r=bzbarsky with that change
Attachment #71277 - Flags: review+
Created attachment 71402 [details] [diff] [review]
Patch v1.1

Back to square 1 with a few tweaks.  Implements generic add/remove observer
functions as per jag.

jag, bz this should be ready to go! :)
Attachment #71277 - Attachment is obsolete: true
Comment on attachment 71402 [details] [diff] [review]
Patch v1.1

r=bzbarsky, assuming you've tested this and it works and all that
Attachment #71402 - Flags: review+

Comment 13

16 years ago
Comment on attachment 71402 [details] [diff] [review]
Patch v1.1

>Index: mozilla/xpfe/browser/resources/content/navigator.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/navigator.js,v
>retrieving revision 1.433
>diff -u -r1.433 navigator.js
>--- mozilla/xpfe/browser/resources/content/navigator.js	2002/02/20 04:06:57	1.433
>+++ mozilla/xpfe/browser/resources/content/navigator.js	2002/02/26 01:14:01
>@@ -207,18 +207,29 @@

// ...

>+var nsButtonPrefListener = new Function();
> nsButtonPrefListener.prototype =

I'm wondering if we should be using:

  var nsButtonPrefListener =
  {
    // ...
  }

  // ...

  window.gButtonPrefListener = nsButtonPrefListener;

Since you'll only have one copy of this object per window.

> {
>   domain: "browser.toolbars.showbutton",
>@@ -247,6 +258,22 @@
>   }
> }
> 
>+var nsTabStripPrefListener = new Function();
>+nsTabStripPrefListener.prototype =
>+{
>+  domain: "browser.tabs.autoHide",
>+  observe: function(subject, topic, prefName)
>+  {
>+    // verify that we're changing the tab browser strip auto hide pref
>+    if (topic != "nsPref:changed") return;
>+    if (prefName != this.domain) return;

Please put the return on the next line.

Do we need to check if |prefName != this.domain|, considering that that
particular listener is only registered for |this.domain|?
Created attachment 72248 [details] [diff] [review]
Patch v1.2

Move the const var foo = { /* ... */ } declarations to the beginning of the
file with other var declarations.  Fix up other issues.
Attachment #71402 - Attachment is obsolete: true
Comment on attachment 72248 [details] [diff] [review]
Patch v1.2

r=bzbarsky
Attachment #72248 - Flags: review+

Comment 16

16 years ago
Comment on attachment 72248 [details] [diff] [review]
Patch v1.2

>+    var show = pref.getBoolPref(prefName);
>+    if (show)
>+      button.setAttribute("hidden","false");
>+    else
>+      button.setAttribute("hidden", "true");

  button.hidden = !show;

>+
>+    // If all the buttons before the separator are hidden, also hide the
>+    // separator
>+    if(allLeftButtonsAreHidden())
>+      document.getElementById("home-bm-separator").setAttribute("hidden", "true");
>+    else
>+      document.getElementById("home-bm-separator").removeAttribute("hidden");

Or rather:

  var bookmarkSeparator = document.getElementById("home-bm-separator");
  bookmarkSeparator.hidden = allLeftButtonsAreHidden();

I know the above is not your doing, but since you're moving this code, please
fix these nits :-)

sr=jag
Attachment #72248 - Flags: superreview+
Created attachment 72552 [details] [diff] [review]
Patch v1.3

Fix jag's nits and address a case where starting Moz with the auto hide pref
set to true, then setting it to false will not initialize the tab listeners and
leave you with broken tabbed browsing.
Attachment #72248 - Attachment is obsolete: true
Created attachment 72554 [details] [diff] [review]
Patch v1.4

Reindents code as requested by jag ;)
Attachment #72552 - Attachment is obsolete: true

Comment 19

16 years ago
Comment on attachment 72554 [details] [diff] [review]
Patch v1.4

sr=jag
Attachment #72554 - Flags: superreview+
Comment on attachment 72554 [details] [diff] [review]
Patch v1.4

r=bzbarsky
Attachment #72554 - Flags: review+

Comment 21

16 years ago
Comment on attachment 72554 [details] [diff] [review]
Patch v1.4

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #72554 - Flags: approval+
checked in on the trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Reporter)

Comment 23

16 years ago
vrfy'd fixed using 2002.03.13 comm bits on linux rh7.2, win2k and mac 10.1.3.
Status: RESOLVED → VERIFIED

Comment 24

15 years ago
*** Bug 168421 has been marked as a duplicate of this bug. ***
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.