Closed Bug 599029 Opened 10 years ago Closed 10 years ago

Restore Default Set doesn't remove toolbar items from Add-on bar

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 4.0b8
Tracking Status
blocking2.0 --- final+

People

(Reporter: WonderCsabo, Assigned: dao)

References

Details

(Whiteboard: [addon bar])

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100923 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100923 Firefox/4.0b7pre

If I put a toolbar button (eg. "Print") to Add-on bar, "Restore Default Set" doesn't remove it at all.

Reproducible: Always

Steps to Reproduce:
1. Open Customization palette.
2. Drag a toolbar item to Add-on bar. Click on Done.
3. Open Customization palette again. Click on Restore Defatult Set. Click on Done.
Actual Results:  
The toolbar item wont be removed from Add-on bar.

Expected Results:  
The toolbar item should be removed from Add-on bar.
blocking2.0: --- → ?
Depends on: 574688
Version: unspecified → Trunk
Blocks: 574688
No longer depends on: 574688
Confirmed the STR in comment 0 with Mozilla/5.0 (Windows NT 5.1; rv:2.0b7pre) Gecko/20100923 Firefox/4.0b7pre
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 599102
blocking2.0: ? → final+
Assignee: nobody → dietrich
Attached patch patch (obsolete) — Splinter Review
Attachment #479801 - Flags: review?(mano)
Comment on attachment 479801 [details] [diff] [review]
patch

>diff --git a/toolkit/content/customizeToolbar.js b/toolkit/content/customizeToolbar.js
>--- a/toolkit/content/customizeToolbar.js
>+++ b/toolkit/content/customizeToolbar.js
>@@ -621,31 +621,22 @@ function addNewToolbar()
>  * Restore the default set of buttons to fixed toolbars,
>  * remove all custom toolbars, and rebuild the palette.
>  */
> function restoreDefaultSet()
> {
>   // Unwrap the items on the toolbar.
>   unwrapToolbarItems();
> 
>-  // Remove all of the customized toolbars.
>-  var child = gToolbox.lastChild;
>-  while (child) {
>-    if (child.hasAttribute("customindex")) {
>-      var thisChild = child;
>-      child = child.previousSibling;
>-      thisChild.currentSet = "__empty";
>-      gToolbox.removeChild(thisChild);
>-    } else {
>-      child = child.previousSibling;
>-    }
>-  }
>-
>-  // Restore the defaultset for fixed toolbars.
>+  // Remove all of the customized toolbars, and
>+  // restore the defaultset for fixed toolbars.
>   forEachCustomizableToolbar(function (toolbar) {
>+    if (toolbar.hasAttribute("customindex"))
>+      gToolbox.removeChild(toolbar);

>+    toolbar.currentSet = "__empty";

Hrm, why is that not within the |if (toolbar.hasAttribute("customindex"))| block as it was before?
Good question. The only way the add-on bar would get reset is if i set currentSet = "__empty". So I changed this code to apply that to all customizable toolbars instead of just those with "customindex" attribute set. Which seems to work. I don't know why the add-on bar doesn't have a customindex value.
> I don't know why the add-on bar doesn't have a customindex value.
Because it is not a user created toolbar?
Comment on attachment 479801 [details] [diff] [review]
patch

Hrm, why aren't you setting defaultset to "" on the addonbar (in browser.xul)?
(In reply to comment #7)
> Comment on attachment 479801 [details] [diff] [review]
> patch
> 
> Hrm, why aren't you setting defaultset to "" on the addonbar (in browser.xul)?

in addition to, or instead of what i'm doing now? i don't understand what you're asking.
Whiteboard: [needs comment mano]
Instead, AFAICT not having the defaultset attribute set to "" is the cause for this bug.
I tried it, and it doesn't seem to result in any change at all.
Whiteboard: [needs comment mano] → [needs review mano]
Comment on attachment 479801 [details] [diff] [review]
patch

>   forEachCustomizableToolbar(function (toolbar) {
>+    if (toolbar.hasAttribute("customindex"))
>+      gToolbox.removeChild(toolbar);
>+    toolbar.currentSet = "__empty";

Setting currentSet on a removed toolbar isn't going to work.

>     var defaultSet = toolbar.getAttribute("defaultset");
>     if (defaultSet)
>       toolbar.currentSet = defaultSet;

Setting currentSet to __empty before setting it to defaultSet seems to be unnecessary work.
I'd suggest this instead:

toolbar.currentSet = toolbar.getAttribute("defaultset") || "__empty";
if (toolbar.hasAttribute("customindex"))
  gToolbox.removeChild(toolbar);
Attachment #479801 - Flags: review-
Attached patch patch (obsolete) — Splinter Review
I was a little bit wrong! It shouldn't be just empty it should be __empty.
Assignee: dietrich → mano
Attachment #479801 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #488655 - Flags: review?
Attachment #479801 - Flags: review?(mano)
It probably / hopefully doesn't matter in practice, but the "correct" default set would actually be "status-bar".
Comment on attachment 488655 [details] [diff] [review]
patch

As far as I understand it, we don't treat static elements as part of the "set".
Attachment #488655 - Flags: review? → review?(dietrich)
(In reply to comment #14)
> As far as I understand it, we don't treat static elements as part of the "set".

We certainly do for toolbars in the navigator-toolbox.
Comment on attachment 488655 [details] [diff] [review]
patch

Hm, I think that as long as the statusbar compat hack is there it should be treated as a default.
Attachment #488655 - Flags: review?(dietrich) → review-
Whiteboard: [needs review mano] → [needs new patch]
Whiteboard: [needs new patch] → [needs new patch][addon bar]
Attached patch patchSplinter Review
Assignee: mano → dao
Attachment #488655 - Attachment is obsolete: true
Attachment #493364 - Flags: review?(dietrich)
Whiteboard: [needs new patch][addon bar] → [addon bar]
Component: General → Toolbars
QA Contact: general → toolbars
http://hg.mozilla.org/mozilla-central/rev/22344f2229cc
Severity: major → minor
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
You need to log in before you can comment on or make changes to this bug.