Closed Bug 1145858 Opened 5 years ago Closed 4 years ago

Rename FirstrunPane to be less confusing

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: liuche, Assigned: ahunt, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 file, 1 obsolete file)

The hierarchy of firstrun is confusing: FirstrunPane, FirstrunPager, FirstrunPanel

FirstrunPane: this is a container for the pager and the whole first run experience and is for animation purposes. The name of this class should reflect that. Perhaps FirstrunContainer, or AnimatedContainer, or something along those lines.

FirstrunPager is the ViewPager that contains the first run pages, but for consistency with HomePager/HomePanel, these pages are called FirstrunPanel.

There should also be some class docs clarifying the relationship between these.
Just your nagging conscience here. This refactor work should be secondary to any other bugs that need higher priority. Also remember landing this before other patches makes uplift the other patches harder.
Aaaahh I also realize that there is a firstrun/FirstrunPanel as well as a static inner class FirstrunPanel! The static inner class should be renamed something like PanelConfig.
Attached patch renameFirstRunPaneReviewCopy (obsolete) — Splinter Review
Attachment #8604326 - Flags: review?(liuche)
Comment on attachment 8604326 [details] [diff] [review]
renameFirstRunPaneReviewCopy

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

Needs a little more work.

For refactoring, you should always use 'hg rename' to rename files (or hg copy, if it's relevant), which will preserve file history for hg annotate.

Per comment #0, can you also add a class comment describing the relationship between these files? Probably to FirstrunAnimationContainer.

Also need to address comment #2.
Attachment #8604326 - Flags: review?(liuche) → review-
This isn't a user-facing issue, but I do think it makes working with this part of the code base more confusing. We could keep it open and I'll try to find time to work on it, or we could WONTFIX it and I'll reopen if it bothers me too much. (Or I'll WONTFIX it if I don't get to it in the next two weeks.)
Assignee: ally → nobody
Mentor: liuche, ally
Whiteboard: [lang=java][good first bug]
Assignee: nobody → andrzej
Bug 1145858 - Rename FirstrunPane to be less confusing
Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26889/diff/1-2/
Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26889/diff/2-3/
Attachment #8694821 - Flags: review?(liuche)
Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

https://reviewboard.mozilla.org/r/26889/#review24379

Looks good, thank you for the class comments! One last thing - be sure to propagate the rename changes to moz.build too, otherwise you'll get build errors.
Attachment #8694821 - Flags: review?(liuche)
Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26889/diff/2-3/
Attachment #8694821 - Flags: review?(liuche)
Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

https://reviewboard.mozilla.org/r/26889/#review24489

Great! Do you have permissions to land this on fx-team? If not, flag 'checkin-needed' in the the keywords, or I can land this for you.
Attachment #8694821 - Flags: review?(liuche) → review+
Also, make sure to put the reviewer r=liuche in the commit message (or look at https://treeherder.mozilla.org/#/jobs?repo=fx-team to see what the commit messages are like).
Attachment #8604326 - Attachment is obsolete: true
Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26889/diff/3-4/
Attachment #8694821 - Attachment description: MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing → MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche
Keywords: checkin-needed
(checkin not needed yet - doing a try build first)
Keywords: checkin-needed
Hi, this caused problems during checkin,

patching file mobile/android/base/firstrun/FirstrunAnimationContainer.java
Hunk #1 FAILED at 13
Hunk #2 FAILED at 64
2 out of 2 hunks FAILED -- saving rejects to file mobile/android/base/firstrun/FirstrunAnimationContainer.java.rej
unable to find 'mobile/android/base/firstrun/FirstrunPager.java' for patching
2 out of 2 hunks FAILED -- saving rejects to file mobile/android/base/firstrun/FirstrunPager.java.rej
unable to find 'mobile/android/base/firstrun/FirstrunPanel.java' for patching
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/firstrun/FirstrunPanel.java.rej
mobile/android/base/firstrun/FirstrunPane.java not tracked!
patch failed to apply
abort: fix up the merge and run hg transplant --continue

can you take a look, thanks!
Flags: needinfo?(ahunt)
Comment on attachment 8694821 [details]
MozReview Request: Bug 1145858 - Rename FirstrunPane to be less confusing. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26889/diff/4-5/
(In reply to Carsten Book [:Tomcat] from comment #16)
> Hi, this caused problems during checkin,
> 
<snip>
> can you take a look, thanks!

Fixed now - we had some path changes recently which is what caused the patch to not apply!
Flags: needinfo?(ahunt)
https://hg.mozilla.org/mozilla-central/rev/96c407f2815f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
You need to log in before you can comment on or make changes to this bug.