593157 - Use a pref to keep track of Panorama "first run" experience

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

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 1

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Proposed patch

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

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.

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/9eb8d4396885

Committed to tabcandy-central.

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

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 5

[Ian Gilman [:iangilman]]

Comment on attachment 471648 [details] [diff] [review]
Proposed patch

looks great.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

I'm going to write a test for this before landing.

Comment 7

[Ian Gilman [:iangilman]]

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.

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

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.

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: Proposed patch v2

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

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v3

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.

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

Pushed to try: dca4fb9b939b

Comment 12

[Ian Gilman [:iangilman]]

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.

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v4: made changes suggested by Ian

Re-requesting review as the patch has changed sufficiently from the previously r+'ed patch.

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

Patch v3, test-interaction-wise exactly the same, passed try. Patch v4 also passes locally (as does patch v3). Just waiting on review, then.

Comment 15

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/c9bf3375b202

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

Thanks Dolske! :D

Comment 17

[Tracy Walker [:tracy]]

verified with recent nightly trunk builds