644704 - Stop using XUL persistence for saving the current view

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

It causes various problems particularly when we dynamically build the category list so we're just going to have to move to using the prefs system instead.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

I considered just saving the entire history state but really that isn't what we want (we never want to restore to a search or details view f.e.) so this just persists the selected category to a pref and restores it early in startup.

We don't need to call maybeHideSearch because we never start with the search view and it defaults to disabled. Also moved the initialisation parts of updateState out to initialize where it makes more sense to have them.

Don't think there is much point hiding this behind a kill-switch. Backout should be the easiest way to revert it.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Tweaks the name of the pref slightly. I want to use extensions.ui.* for all the UI related prefs.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 521619 [details] [diff] [review]
patch rev 2

Looks good.

>+  // Allow passing in a view through the window arguments
>+  if ("arguments" in window && window.arguments.length > 0) {
>+    if ("view" in window.arguments[0])
>+      view = window.arguments[0].view;
>+  }

Nit: collapse into one if statement.

>+    try {
>+      this.node.value = Services.prefs.getCharPref(PREF_UI_LASTCATEGORY);
>+    }
>+    catch (e) { }

Style nit: Remove newline before the catch.

>+
>+    // If there was no last view or no existing category matched the last view
>+    // then the list will default to selecting the search category and we never
>+    // want to show that as the first view so switch to the default category
>+    if (this.node.selectedItem == this._search)
>+      this.node.value = VIEW_DEFAULT;

Feels like this really needs a test. r- so I get to see that.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #2)
> I want to use extensions.ui.* for all the UI related prefs.

Agreed.

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 3

> >+
> >+    // If there was no last view or no existing category matched the last view
> >+    // then the list will default to selecting the search category and we never
> >+    // want to show that as the first view so switch to the default category
> >+    if (this.node.selectedItem == this._search)
> >+      this.node.value = VIEW_DEFAULT;
> 
> Feels like this really needs a test. r- so I get to see that.

I've added a test that closing from the search view opens the default view but I can't test that a missing view goes back to default without fixing bug 595848 so I'll have to add a test for that there.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 523635 [details] [diff] [review]
patch rev 3

>+  // Allow passing in a view through the window arguments
>+  if ("arguments" in window && window.arguments.length > 0 &&
>+      "view" in window.arguments[0])
>+    view = window.arguments[0].view;

Tiny style nit: Multi-line if-condition should have curly brackets.

(In reply to comment #5)
> I've added a test that closing from the search view opens the default view but
> I can't test that a missing view goes back to default without fixing bug 595848
> so I'll have to add a test for that there.

Ah, yes.

Comment 7

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/fbe7830f27c0

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110430 Firefox/6.0a1