Closed Bug 644704 Opened 10 years ago Closed 10 years ago

Stop using XUL persistence for saving the current view

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla6

People

(Reporter: mossop, Assigned: mossop)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → dtownsend
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Attachment #521603 - Flags: review?(bmcbride)
Blocks: 641421
Attached patch patch rev 2 (obsolete) — Splinter Review
Tweaks the name of the pref slightly. I want to use extensions.ui.* for all the UI related prefs.
Attachment #521603 - Attachment is obsolete: true
Attachment #521603 - Flags: review?(bmcbride)
Attachment #521619 - Flags: review?(bmcbride)
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.
Attachment #521619 - Flags: review?(bmcbride) → review-
(In reply to comment #2)
> I want to use extensions.ui.* for all the UI related prefs.

Agreed.
Blocks: 647265
Attached patch patch rev 3Splinter Review
> >+
> >+    // 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.
Attachment #521619 - Attachment is obsolete: true
Attachment #523635 - Flags: review?(bmcbride)
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.
Attachment #523635 - Flags: review?(bmcbride) → review+
Landed: http://hg.mozilla.org/mozilla-central/rev/fbe7830f27c0
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110430 Firefox/6.0a1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.