Closed
Bug 958185
Opened 10 years ago
Closed 10 years ago
Rename "page" things to "panel" things in the home code
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 29
People
(Reporter: Margaret, Assigned: lucasr)
References
Details
(Whiteboard: shovel-ready)
Attachments
(1 file)
88.19 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
To make our code less confusing, we should rename the things we currently call "pages" to "panels". This includes existing code, e.g. BookmarksPage -> BookmarksPanel. A patch for this will probably touch a lot of code, but it should be straightforward.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → lucasr.at.mozilla
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8358411 [details] [diff] [review] Rename Page* terminology to Panel* in the home package (r=margaret) Here's what the patch does: In HomePager: TopSitesPage -> TopSitesPanel BookmarksPage -> BookmarksPanel HistoryPage -> HistoryPanel MostRecentPage -> MostRecentPanel LastTabsPage -> LastTabsPanel ReadingListPage -> ReadingListPanel ListPage -> ListPanel ...and all associated layouts e.g. home_top_sites_page -> home_top_sites_panel. In HomeConfig: PageEntry -> PanelConfig PageType -> PanelType
Attachment #8358411 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8358411 [details] [diff] [review] Rename Page* terminology to Panel* in the home package (r=margaret) Review of attachment 8358411 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. One possible point of confusion is that we still use some "page" terminology in HomePager, since we're extending a ViewPager, but I think that's fine. However, that could be a good thing to explain in some documentation when we get around to writing that up :) I can try to land this before the weekend so that we can base all new patches on top of it, but we should do a try push, since we have some test changes. ::: mobile/android/base/home/HomeAdapter.java @@ +142,4 @@ > } > } > > + private final class PanelInfo { Oh... namespace collision with my patch in bug 958192. Maybe we really should just use PanelConfig in that patch instead of making a new data type to represent panel data coming from the JS side. ::: mobile/android/base/home/HomeConfig.java @@ +101,1 @@ > this(type, title, type.toString(), flags); I think we should file a bug to get rid of these constructors that don't take an id themselves, since it's dangerous that we're assuming each panel will have a unique id, yet we're just using the type as an id here. ::: mobile/android/base/tests/components/AboutHomeComponent.java @@ +61,5 @@ > private ViewPager getHomePagerView() { > return (ViewPager) mSolo.getView(R.id.home_pager); > } > > + public AboutHomeComponent assertCurrentPage(final PanelType expectedPage) { As a follow-up, we should rename this method assertCurrentPanel.
Attachment #8358411 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Comment 5•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=702071597475 I also updated the CLOBBER file to deal with the renaming of resources.
Reporter | ||
Comment 6•10 years ago
|
||
Green on try, so pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/e6fc7735a592
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e6fc7735a592
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to :Margaret Leibovic from comment #4) > Comment on attachment 8358411 [details] [diff] [review] > Rename Page* terminology to Panel* in the home package (r=margaret) > > Review of attachment 8358411 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. One possible point of confusion is that we still use some "page" > terminology in HomePager, since we're extending a ViewPager, but I think > that's fine. However, that could be a good thing to explain in some > documentation when we get around to writing that up :) Yeah, I tried to keep 'page' terminology on the parts of the code that are dealing with view-level stuff in ViewPager. > ::: mobile/android/base/home/HomeAdapter.java > @@ +142,4 @@ > > } > > } > > > > + private final class PanelInfo { > > Oh... namespace collision with my patch in bug 958192. Maybe we really > should just use PanelConfig in that patch instead of making a new data type > to represent panel data coming from the JS side. Indeed. I can rename this PanelInfo here to be something else, just to avoid confusion. As for reusing PanelConfig in PanelManager, we'll probably need a distinction between what's stored in HomeConfig and the objects that represent the available panels in the Settings UI. PanelInfo instances (in the context of PanelManager) will likely contain extra information about the panels that are not meant to (and shouldn't really) be stored in the HomePager configuration e.g. favicon, longer description, thumbnails (?), author credits, etc. Maybe PanelInfo will be a kinda of super-set of PanelConfig or something. > ::: mobile/android/base/home/HomeConfig.java > @@ +101,1 @@ > > this(type, title, type.toString(), flags); > > I think we should file a bug to get rid of these constructors that don't > take an id themselves, since it's dangerous that we're assuming each panel > will have a unique id, yet we're just using the type as an id here. Yeah, this is a bit too hand-wavy. Bug 958175 could cover this I guess? > ::: mobile/android/base/tests/components/AboutHomeComponent.java > @@ +61,5 @@ > > private ViewPager getHomePagerView() { > > return (ViewPager) mSolo.getView(R.id.home_pager); > > } > > > > + public AboutHomeComponent assertCurrentPage(final PanelType expectedPage) { > > As a follow-up, we should rename this method assertCurrentPanel. Ah, missed this one. Filed bug 959219 to track this.
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #8) > > ::: mobile/android/base/home/HomeAdapter.java > > @@ +142,4 @@ > > > } > > > } > > > > > > + private final class PanelInfo { > > > > Oh... namespace collision with my patch in bug 958192. Maybe we really > > should just use PanelConfig in that patch instead of making a new data type > > to represent panel data coming from the JS side. > > Indeed. I can rename this PanelInfo here to be something else, just to avoid > confusion. > > As for reusing PanelConfig in PanelManager, we'll probably need a > distinction between what's stored in HomeConfig and the objects that > represent the available panels in the Settings UI. PanelInfo instances (in > the context of PanelManager) will likely contain extra information about the > panels that are not meant to (and shouldn't really) be stored in the > HomePager configuration e.g. favicon, longer description, thumbnails (?), > author credits, etc. Maybe PanelInfo will be a kinda of super-set of > PanelConfig or something. This makes sense. I think we should name the PanelInfo in HomeAdapter something else then, maybe even just Panel? I can file a follow-up about that. > > ::: mobile/android/base/home/HomeConfig.java > > @@ +101,1 @@ > > > this(type, title, type.toString(), flags); > > > > I think we should file a bug to get rid of these constructors that don't > > take an id themselves, since it's dangerous that we're assuming each panel > > will have a unique id, yet we're just using the type as an id here. > > Yeah, this is a bit too hand-wavy. Bug 958175 could cover this I guess? Sure, I can handle this issue in that bug.
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•