958185 - Rename "page" things to "panel" things in the home code

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

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.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Rename Page* terminology to Panel* in the home package (r=margaret)

Comment 2

[Lucas Rocha (:lucasr)]

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

Comment 4

[:Margaret Leibovic]

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.

Comment 5

[:Margaret Leibovic]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=702071597475

I also updated the CLOBBER file to deal with the renaming of resources.

Comment 6

[:Margaret Leibovic]

Green on try, so pushed to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/e6fc7735a592

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/e6fc7735a592

Comment 8

[Lucas Rocha (:lucasr)]

(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.

Comment 9

[:Margaret Leibovic]

(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.