Closed Bug 593157 Opened 12 years ago Closed 12 years ago
Use a pref to keep track of Panorama "first run" experience
Right now I have "first run" set to go off every time you open a new window and you don't have any groups. We need a better rule. Store a "we've shown firstrun" flag in Prefs; if it's not there, assume we haven't yet.
Comment on attachment 471648 [details] [diff] [review] Proposed patch Canceling review. We will commit this to tabcandy-central and get it reviewed and landed as part of bug 578512.
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/9eb8d4396885 Committed to tabcandy-central.
Depends on: 578512
Comment on attachment 471648 [details] [diff] [review] Proposed patch We are no longer doing a t-c to m-c merge. Requesting feedback so we can review it.
Comment on attachment 471648 [details] [diff] [review] Proposed patch looks great.
I'm going to write a test for this before landing.
Comment on attachment 471648 [details] [diff] [review] Proposed patch Random thought: what if someone sets the pref to false manually, after they already have a number of groups? As written, we will create a new group and pull all of their tabs into it, but leave the other groups around (as long as they have titles). I wouldn't say this scenario warrants a lot of effort, but maybe we could do something simple to address it.
Priority: -- → P3
Patch for checkin, with a test... BUT the try tree is closed so I can't send it for a sanity test. Will push to try whenever I can.
Attachment #471648 - Attachment is obsolete: true
We need to ensure that we create a new group and put all orphan tab items in if no group data is available even after the first run. Pushed to try -> 35bb5ccd05d6
Thanks Raymond. I didn't like how we split up the grouping code and the first-run welcome infoitem code, so I pulled it all out into a utility function, UI.reset(), which perhaps some of our tests can benefit from as well. Also, in the unlikely scenario it was firstTime but there's group info, it would display the infoitem but not do the grouping, so I fixed that.
Pushed to try: dca4fb9b939b
Comment on attachment 476985 [details] [diff] [review] Patch v3 Looks good, but let's destroy any existing groups in reset. Also, we can kill _reset to avoid confusion; it's out of date now anyway.
Attachment #476985 - Flags: feedback?(ian) → feedback+
Re-requesting review as the patch has changed sufficiently from the previously r+'ed patch.
Patch v3, test-interaction-wise exactly the same, passed try. Patch v4 also passes locally (as does patch v3). Just waiting on review, then.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 4.0 → Firefox 4.0b7
Thanks Dolske! :D
verified with recent nightly trunk builds
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.