Closed Bug 870011 Opened 8 years ago Closed 7 years ago

Restore Default requires a restart on UX nightly

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jaws, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M6])

Attachments

(1 file, 2 obsolete files)

The "Restore Default" functionality in the new Australis customization currently requires a restart to work correctly. It should restore the toolbars and panel menu to their default state without requiring this restart.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
No longer blocks: 869104
Whiteboard: [Australis:M5]
Just a heads up that this will also mean clearing the currentset of all toolbars, and persisting that value. We're still using currentset for now, as we want it to be relatively painless to switch back and forth between builds until this thing gets shipped.
Hey Mike, any headway on this?
Flags: needinfo?(mdeboer)
Unfortunately not, I'd like to discuss this with you tomorrow as as possible!
Flags: needinfo?(mdeboer)
Attached patch Restore Default implementation (obsolete) — Splinter Review
Attachment #753015 - Flags: review?(bmcbride)
Comment on attachment 753015 [details] [diff] [review]
Restore Default implementation

Review of attachment 753015 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1410,5 @@
>      LOG("State reset");
> +    // HACK: when all buttons are restored to their defaults, the Customize view
> +    // is out of whack. Exiting beforehand makes that behavior not noticable.
> +    for (let [window,] of gBuildWindows) {
> +      window.gCustomizeMode.exit();

Driveby - if this is really necessary, I think we should do it in CustomizeMode.jsm, but I'm curious *why* this is necessary.

Why does it get out of whack? Can you describe this out-of-whackness? :)
Attachment #753015 - Flags: review?(bmcbride) → review?(mconley)
(In reply to Mike Conley (:mconley) from comment #5)
> 
> Driveby - if this is really necessary, I think we should do it in
> CustomizeMode.jsm, but I'm curious *why* this is necessary.
> 
> Why does it get out of whack? Can you describe this out-of-whackness? :)

I'll ask you on IRC about the move to CustomizeMode.jsm.

Out-of-whackness: when the toolbar buttons are restored/ rebuilt, the buttons in customize mode are not draggable anymore - the drag 'n drop mode/ attributes don't apply anymore because the nodes are replaced by new ones. The state of the bookshelf button is not maintained, so clicking it after a restore will result in opening an empty popup. So here I went for the quick 'n dirty solution; just exit customize mode and when the user enters it again the UI is in the proper state.

An alternative solution would be to re-initialize the customize mode when the toolbars and panels are rebuilt. This way we do not need to exit the mode. To implement this, I would like to have a bit more information to figure out what the best way forward is.
Whiteboard: [Australis:M5] → [Australis:M6]
Comment on attachment 753015 [details] [diff] [review]
Restore Default implementation

Cancelling review request since, as mentioned in IRC, we want restore default state to work without having to exit (and/or subsequently re-enter) customization mode.

The problem Mike is running into is that we need to unwrap all of the widgets before rebuilding the areas. We may also need to de-populate and re-populate the palettes.
Attachment #753015 - Flags: review?(mconley)
Attached patch Restore Default implementation (obsolete) — Splinter Review
Attachment #753015 - Attachment is obsolete: true
Attachment #753718 - Flags: review?(mconley)
Comment on attachment 753718 [details] [diff] [review]
Restore Default implementation

Review of attachment 753718 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Mike! r=me with the following fixes.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1428,5 @@
> +    // Restore the state for each area to its defaults
> +    for (let [area, defaultPlacements] of gDefaultPlacements) {
> +      this.restoreStateForArea(area);
> +    }
> +    

Trailing whitespace here and on next line.

@@ +1435,5 @@
> +    for (let [areaId, areaNodes] of gBuildAreas) {
> +      let placements = gPlacements.get(areaId);
> +      for (let areaNode of areaNodes) {
> +        this.buildArea(areaId, placements, areaNode);
> +        areaNode.setAttribute("currentset", placements.join(","));

Hm - we just remove this attribute again in CustomizeMode.reset after calling this, so we probably don't need to set the attribute.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +482,5 @@
> +      toolbar.removeAttribute("currentset");
> +      LOG("[RESET] Removing currentset of " + toolbar.id);
> +      // Persist the currentset attribute directly on hardcoded toolbars.
> +      document.persist(toolbar.id, "currentset");
> +    }

After this, set the hidden state of the reset button via:

this.resetButton.hidden = CustomizableUI.inDefaultState;
Attachment #753718 - Flags: review?(mconley)
Attachment #753718 - Flags: review?(benjamin)
Attachment #753718 - Flags: review+
Carrying over r=mconley
Attachment #753718 - Attachment is obsolete: true
Attachment #753823 - Flags: review+
Landed in UX as https://hg.mozilla.org/projects/ux/rev/bab65fae9ff6
Whiteboard: [Australis:M6] → [Australis:M6][fixed-in-ux]
Duplicate of this bug: 858363
https://hg.mozilla.org/mozilla-central/rev/bab65fae9ff6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M6][fixed-in-ux] → [Australis:M6]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.