942231 - Allow for reordering and removing home page tabs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Comment 1

[Lucas Rocha (:lucasr)]

I'll bootstrap the infrastructure to manage about:home pages. This includes both the backend bits (i.e. change about:home to be backed by a 'configuration' instead of fixed list of pages) and the UI to manage it.

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Remove unused member from HomePager (r=margaret)

https://bugzilla.mozilla.org/show_bug.cgi?id=946517

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Factor out HomePager's adapter into a separate file (r=margaret)

https://bugzilla.mozilla.org/show_bug.cgi?id=946517

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Use 'page' terminology in HomePager instead of 'tab' (r=margaret)

https://bugzilla.mozilla.org/show_bug.cgi?id=946517

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Keep showHomePager* method together in BrowserApp (r=margaret)

https://bugzilla.mozilla.org/show_bug.cgi?id=946517

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Define constant with default value for 'can load' hint (r=margaret)

https://bugzilla.mozilla.org/show_bug.cgi?id=946517

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Change HomePager to be backed by HomeConfig (r=margaret)

https://bugzilla.mozilla.org/show_bug.cgi?id=946517

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8344703 [details] [diff] [review]
Remove unused member from HomePager (r=margaret)

Prep work.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8344704 [details] [diff] [review]
Factor out HomePager's adapter into a separate file (r=margaret)

Prep work. The adapter will get a bit more involved. Let's get its code out of HomePager.java

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8344705 [details] [diff] [review]
Use 'page' terminology in HomePager instead of 'tab' (r=margaret)

Prep work. The HomePager code uses Tab/Page terminology inconsistently. HomePager code uses 'tabs' but each page is written using 'Page' (TopSitesPage, BookmarksPage, etc).  This patch consolidates everything to use 'Page'.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 8344706 [details] [diff] [review]
Keep showHomePager* method together in BrowserApp (r=margaret)

Prep work. Cosmetic change. I blame rnewman :-)

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 8344707 [details] [diff] [review]
Define constant with default value for 'can load' hint (r=margaret)

Prep work. We'll need the same default value available in the adapter.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8344708 [details] [diff] [review]
Change HomePager to be backed by HomeConfig (r=margaret)

Ok, here's the real meat. Before I start, a few fundamental principles behind the code implementation decisions here.

What this patch does
--------------------

It changes about:home so that we can:
- Load the list of pages from a configuration backend
- Refresh the contents of about:home if the configuration changes
- Define the default page from a configuration backend

Fundamental assumption
----------------------

We can't wait for Gecko to be up and running to figure out the content of about:home. HomePager should have everything it needs to define its content before Gecko is loaded.

Code Terminology
----------------

- Page: a swipeable page in about:home
- PageEntry: an config entry defining a page in about:home
- PageType: either builtin (our current 'native' pages) or addon (defined by a JS add-on)

Technical goals
---------------

The main goal with this approach is to decouple 'where', 'what', and 'how' the list of about:home pages is loaded:

'where' -> off-main thread, through a loader
'what' -> a list of PageEntries defining each about:home page
'how' -> the format used to load and save PageEntries

Besides the more obvious benefit of splitting the architecture into thinner layers (as opposed to a monolithic component that does everything), this approach will also allow us to more easily split development tasks. For example, someone can work on implementing the configuration storage while someone else implements the settings UI based on HomeConfig.

Components
----------

HomeConfig - The high-level about:home API and all related types (PageEntry, PageType, BuiltinPage, etc).

HomeConfigLoader - An AsyncTaskLoader bound to BrowserApp that loads the list of PageEntries off-main thread and caches the list in memory for future access. The greatest benefit of using an AsyncTaskLoader for this is that it handles the list mem-caching for us as well as transparently managing the config refresh routines. We'll only touch the disk to load the list of pages once on startup and whenever the configuration changes.  

HomeConfigBackend - The component responsible for loading and saving a list of PageEntries.

How it works
------------

When about:home is first displayed, the initLoader() call in HomePager.show() will trigger HomeConfigLoader to use the given HomeConfigBackend to load the list of PageEntries off-main thread and deliver it to the HomePager adapter. The HomePager is updated with the list of PageEntries and the initial page is defined by the PageEntry set as DEFAULT_PAGE. From then on, subsequent calls of initLoader() in HomePager.show() will simply use the cached list of PageEntries and deliver them immediatelly to the HomePager adapter. If the HomeConfigBackend reports a change in the configuration (i.e. OnChangeListener is called), the HomeConfigLoader will automatically re-load the list of PageEntries and deliver them to the HomePager adapter (in case it's visible).

Pages backed by add-ons will be instantiated as an 'AddonPage' fragment (which does nothing right now). AddonPage instances get a PageEntry as an argument. PageEntry should have all the information needed to fetch data from that add-on. For the simplest add-on with a single list, it will simply use the add-on id (from the PageEntry) to access the data from our 'lists' ContentProvider. We can always extend PageEntry to provide more context/information to AddonPage. This is just the bare minimum.  

Notes on page layouts
---------------------

Right now, there's no definition of 'layout' in PageEntry. By 'layout' I mean things like 'grid', 'list', etc. For now, AddonPage is just a ListView.

Notes on config invalidation
----------------------------

Because we can't wait for Gecko to be up and running, the configuration will also serve as a cache of things like 'title' which depends on the current locale and version of the add-on. For instance, if you switch to a different locale or the add-on is updated to a new version, we'll have to 'refresh' the configuration to get, say, the title string for the newly selected locale as well as any layout changes from the new add-on version.

The bottom line here is that we'll have to come up with a robust invalidation routine that is triggered whenever the add-on is updated and you switch to another locale.

Notes on backwards-compatibility
--------------------------------

This is step zero torwards the new dynamic about:home bits. Given that we want to land the new stuff in Nightly without breaking everything, we'll have to take gradual steps in terms of steeing about:home into a new direction. This patch essentially replaces the 'guts' of about:home without changing (barely) anything in HomePager's public API.

More specifically, this patch keeps the 'Page' enum and adds a small set of (internal) APIs to convert the old world to the new HomeConfig-based world. We can't remove the Page enum just yet because we need it to access the Reading List page from the Reader Mode UI. In fact, this specific use case will have to be re-designed once we make about:home configurable. Right now, we assume Reading List will always be in about:home and that will not necessarily be the case anymore. I'll file a follow-up to track this. Once this use case is sorted out (in a different UI maybe?), we can remove Page enum entirely and update the tests accordingly.

As I mentioned before, the intent of this patch is just to bootstrap the needed infrastructure for making HomePager configurable without breaking anything in Nightly. Let's solve one problem at a time :-)

Notes on settings UI, HomeConfigBackend, and HomeConfig
-------------------------------------------------------

I expect the Settings UI to:
- Get the list of PageEntries using HomeConfigLoader
- Get the list of 'list add-ons' using a Gecko message or something
- Blend these two to provide a way to add/remove/reorder pages as well as defining the default one
- Each change in the setting UI, triggers a HomeConfig.save() call off-main thread.

The backend should be able to report changes to the HomePager somehow. For instance, if we end up storing the HomePager configuration in, say, a SharedPreferences key, the backend should probably set an OnSharedPreferenceChangeListener under the hood. Or just listen to a special Gecko message.

Comment 14

[:Margaret Leibovic]

Comment on attachment 8344705 [details] [diff] [review]
Use 'page' terminology in HomePager instead of 'tab' (r=margaret)

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

Nice, I was about to suggest this after reading the last patch! :)

Comment 15

[:Margaret Leibovic]

Comment on attachment 8344706 [details] [diff] [review]
Keep showHomePager* method together in BrowserApp (r=margaret)

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

Nice catch ;)

Comment 16

[:Margaret Leibovic]

Comment on attachment 8344708 [details] [diff] [review]
Change HomePager to be backed by HomeConfig (r=margaret)

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

Thanks for the detailed comment about this patch! Everything you said sounds reasonable to me, and I agree it's best to work in incremental chunks here.

I'm getting review fatigue, so I need to take a break and look through this more later, but I wanted to leave the comments I came up with so far.

::: mobile/android/base/BrowserApp.java
@@ +1652,5 @@
>      }
>  
> +    private void showHomePagerWithAnimator(PropertyAnimator animator) {
> +        // Passing null here means the default page will be defined
> +        // by the HomePager's configuration.

Helpful comment!

::: mobile/android/base/Tab.java
@@ +93,5 @@
>          mBaseDomain = "";
>          mUserSearch = "";
>          mExternal = external;
>          mParentId = parentId;
> +        mAboutHomePage = null;

Am I correct that we just need this for that reader mode toolbar button? It feels like Tab shouldn't need to know anything about about:home.

::: mobile/android/base/home/AddonPage.java
@@ +26,5 @@
> +
> +/**
> + * Fragment that displays lists backed by add-ons.
> + */
> +public class AddonPage extends HomeFragment {

Naming bikeshed: I don't know that we should use the term "add-on" here, since this page doesn't need to know that its data comes from an add-on. The current plan is to just pull the data out of some content provider (bug 941318).

In bug 862805, I named my version of this `CustomListPage`, but I don't really like that name. Maybe we could just call it `ListPage`, since it should just be some generic container that can load some list. Later down the road, I could see us using this for things like our own reading list, since most of this logic is exactly the same as ReadingListPage.

What do other people think?

Comment 17

[Ian Barlow (:ibarlow)]

Attachment: Settings mockups

Hey folks, quick mockup for your reference. 

The mockup on the left shows the initial "reordering" screen for this bug, and the one on the right shows the additional "add list" functionality which will be added in bug 942878.

Comment 18

[:Margaret Leibovic]

Comment on attachment 8345295 [details]
Settings mockups

I think this mock-up belongs in bug 942875.

Let's keep this bug about the plumbing infrastructure needed to change these pages around, and use bug 942875 for the UI.

Comment 19

[:Margaret Leibovic]

Comment on attachment 8344708 [details] [diff] [review]
Change HomePager to be backed by HomeConfig (r=margaret)

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

Overall this approach looks good. However, I don't want us to include the idea of "add-ons" in our home page code, so I'd like us to iterate on this a bit before landing it.

::: mobile/android/base/home/AddonPage.java
@@ +31,5 @@
> +    // Cursor loader ID for add-on list
> +    private static final int LOADER_ID_ADDON = 0;
> +
> +    // The add-on id associated with this page
> +    private PageEntry mPageEntry;

I think we should be careful to keep the idea of add-ons separate from pages. In the conversation in bug 862805, we decided that add-ons should be able to register new lists, but users are the ones who can manage those lists in pages. For v1, there will be a 1-1 list-page mapping, but this might not always be the case, so we should be careful not to bake that assumption into the home pager logic.

::: mobile/android/base/home/HomeAdapter.java
@@ +151,5 @@
> +        private final PageEntry mPageEntry;
> +
> +        PageInfo(PageEntry pageEntry) {
> +            mId = pageEntry.getType() + "-" + pageEntry.getId();
> +            mPageEntry = pageEntry;

Why do we need both PageInfo and PageEntry? It feels like PageInfo is an unnecessary extra layer of abstraction.

::: mobile/android/base/home/HomeConfig.java
@@ +17,5 @@
> +
> +final class HomeConfig {
> +    public static enum PageType implements Parcelable {
> +        BUILTIN("builtin"),
> +        ADDON("addon");

Instead of specifying "built-in" and "add-on" types here, then creating a separate BuiltinPage enum, could we specify those built-in pages as their own page types in here? I'd like to minimize the amount of logic we have that needs to special-case these new third-party lists. It seems like we could combine PageType with BuiltinPage, and then just create a new generic list type that will be instantiated with the new generic list page class.

Comment 20

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #19)
> Comment on attachment 8344708 [details] [diff] [review]
> Change HomePager to be backed by HomeConfig (r=margaret)
> 
> Review of attachment 8344708 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall this approach looks good. However, I don't want us to include the
> idea of "add-ons" in our home page code, so I'd like us to iterate on this a
> bit before landing it.
> 
> ::: mobile/android/base/home/AddonPage.java
> @@ +31,5 @@
> > +    // Cursor loader ID for add-on list
> > +    private static final int LOADER_ID_ADDON = 0;
> > +
> > +    // The add-on id associated with this page
> > +    private PageEntry mPageEntry;
> 
> I think we should be careful to keep the idea of add-ons separate from
> pages. In the conversation in bug 862805, we decided that add-ons should be
> able to register new lists, but users are the ones who can manage those
> lists in pages. For v1, there will be a 1-1 list-page mapping, but this
> might not always be the case, so we should be careful not to bake that
> assumption into the home pager logic.

Yeah, this is just a confusing leftover comment, sorry. I didn't mean to imply there's a 1-1 mapping between add-ons and pages. But we will need an add-on ID to 'scope' the data fetching to the lists that the add-ons 'owns'. Also, as I said in comment #13, there's nothing about the layouts and their respective data sources in PageEntry yet. This is something should be sorted out in a follow-up bug.

Just as an extra clarification on how I envision this API to work, let's take the example given by rnewman in bug 862805 comment 18. For the example page below:

Provider = Pocket
Title = "Pocket"
Addon-ID = abcde
Page-ID = pocketpage
Panels:
  Recommended: type = "squares", list = "com.getpocket.recommended", limit = 4
  My Queue: type = "thumbnail-list", list = "com.getpocket.queue" 

The (hypothetical) PageEntry instance representing the page above would look like:

PageEntry
  PageType type: PageType.ADDON
  String id: abcde
  String title: "Pocket"
  List<Panels> panels:
     PagePanel
        type: PanelType.SQUARE
        list: "com.getpocket.recommended"
     PagePanel
        type: PanelType.THUMBNAIL_LIST
        list: "com.getpocket.queue"

Then PageEntry would have an API roughly like this:
  int getPanelCount();
  PagePanel getPanel(int index);

So that in AddonPage you could do something like:

final int panelCount = pageEntry.getPanelCount();
for (int i = 0; i < panelCount; i++) {
    PagePanel panel = pageEntry.getPanel(i);
    // Build AddonPage views and bind their respective data accordingly
}

In terms of ContentProvider URIs, I expect us to have something like:

content://org.mozilla.fennec.db.lists/?id=ADDON_ID&listId=LIST_ID&profile=PROFILE_ID&limit=LIMIT

More concretely, in the case above, we'd query the content provider with something like:

content://org.mozilla.fennec.db.lists/?id=abcde&listId=com.getpocket.recommended&profile=PROFILE_ID&limit=4

> ::: mobile/android/base/home/HomeAdapter.java
> @@ +151,5 @@
> > +        private final PageEntry mPageEntry;
> > +
> > +        PageInfo(PageEntry pageEntry) {
> > +            mId = pageEntry.getType() + "-" + pageEntry.getId();
> > +            mPageEntry = pageEntry;
> 
> Why do we need both PageInfo and PageEntry? It feels like PageInfo is an
> unnecessary extra layer of abstraction.

PageEntry is meant to be UI agnostic. It wouldn't make sense to have things like fragment class and arguments in PageEntry. Also, the PageInfo ID is UI-specific as it's used to identify the fragment in HomePager, no the PageEntry itself.

> ::: mobile/android/base/home/HomeConfig.java
> @@ +17,5 @@
> > +
> > +final class HomeConfig {
> > +    public static enum PageType implements Parcelable {
> > +        BUILTIN("builtin"),
> > +        ADDON("addon");
> 
> Instead of specifying "built-in" and "add-on" types here, then creating a
> separate BuiltinPage enum, could we specify those built-in pages as their
> own page types in here? I'd like to minimize the amount of logic we have
> that needs to special-case these new third-party lists. It seems like we
> could combine PageType with BuiltinPage, and then just create a new generic
> list type that will be instantiated with the new generic list page class.

Interesting, I feel that keeping things clearly separate is less prone to confusion :-) Let me try doing what you suggest here and see how it goes. I'll report back.

Comment 21

[Lucas Rocha (:lucasr)]

Attachment: Change HomePager to be backed by HomeConfig (r=margaret)

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 8346054 [details] [diff] [review]
Change HomePager to be backed by HomeConfig (r=margaret)

Ok, here's what's new:
- Went with the single enum for all page types as suggested.
- Added some extra validation checks on new PageEntry instances (see PageEntry's constructor).
- Renamed Addon* to List*.

Comment 23

[:Margaret Leibovic]

Comment on attachment 8346054 [details] [diff] [review]
Change HomePager to be backed by HomeConfig (r=margaret)

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

Ship it! ... if it's green on try :)

Overall, this achieves the goal of putting the plumbing in place to start making more progress on configurable home pages. Let's make sure to file bugs on next steps if they aren't already filed (and mark them as shovel-ready!). A few things we need to do:
* Remove the dependencies on HomePager.Page (that reading list button and robocop tests), so that we can kill it off
* Implement the HomeConfig backend
* ... anything else?

It would also be nice to add some code documentation for these new classes, so that people new to the code understand their respective roles and any assumptions they should be aware of.

::: mobile/android/base/home/HomeConfig.java
@@ +20,5 @@
> +        TOP_SITES("top_sites", TopSitesPage.class),
> +        BOOKMARKS("bookmarks", BookmarksPage.class),
> +        HISTORY("history", HistoryPage.class),
> +        READING_LIST("reading_list", ReadingListPage.class),
> +        LIST("list", ListPage.class);

I really like this a lot more :)

::: mobile/android/base/home/HomePager.java
@@ +321,5 @@
> +        }
> +
> +        @Override
> +        public void onLoadFinished(Loader<List<PageEntry>> loader, List<PageEntry> pageEntries) {
> +            if (mLoaded) {

Is this covering the case where the home page gets hidden before the config loader actually finishes? We should add a comment explaining what this check is for.

Comment 24

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #23)
> Comment on attachment 8346054 [details] [diff] [review]
> Change HomePager to be backed by HomeConfig (r=margaret)
> 
> Review of attachment 8346054 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ship it! ... if it's green on try :)
> 
> Overall, this achieves the goal of putting the plumbing in place to start
> making more progress on configurable home pages. Let's make sure to file
> bugs on next steps if they aren't already filed (and mark them as
> shovel-ready!). A few things we need to do:
> * Remove the dependencies on HomePager.Page (that reading list button and
> robocop tests), so that we can kill it off
> * Implement the HomeConfig backend
> * ... anything else?

Filed a few follow-up bugs depending on this one.  

> It would also be nice to add some code documentation for these new classes,
> so that people new to the code understand their respective roles and any
> assumptions they should be aware of.

Ok, I'll sprinkle some javadocs here and there :-)

> ::: mobile/android/base/home/HomeConfig.java
> @@ +20,5 @@
> > +        TOP_SITES("top_sites", TopSitesPage.class),
> > +        BOOKMARKS("bookmarks", BookmarksPage.class),
> > +        HISTORY("history", HistoryPage.class),
> > +        READING_LIST("reading_list", ReadingListPage.class),
> > +        LIST("list", ListPage.class);
> 
> I really like this a lot more :)
> 
> ::: mobile/android/base/home/HomePager.java
> @@ +321,5 @@
> > +        }
> > +
> > +        @Override
> > +        public void onLoadFinished(Loader<List<PageEntry>> loader, List<PageEntry> pageEntries) {
> > +            if (mLoaded) {
> 
> Is this covering the case where the home page gets hidden before the config
> loader actually finishes? We should add a comment explaining what this check
> is for.

We only care about the adapter if the HomePager is up and visible in the activity (even if the activity itself is in background). I added a comment explaining this.

Comment 25

[Lucas Rocha (:lucasr)]

Ok, got it green on try. Some tests were in 'pending' state forever so there's a slight possibility of them failing. I'm not sure how much I'll still have to work on this today so I decided to push the patches (without an extra review, sorry margaret) and see how it goes.

The changes I've made in the last patch:
- Remove the custom HomeAdapter.getItemPosition(). It was causing problems when destroying the activity while the HomePager was visibility. The HomeAdapter was trying to remove the same fragments twice (from different code paths).
- Proper handling of tab strip visibility. We only show the strip when we have the list of pages in place. This is only noticeable on startup (when we don't have the list of about:home pages in memory yet).

https://hg.mozilla.org/integration/fx-team/rev/f1769583f43b
https://hg.mozilla.org/integration/fx-team/rev/c4e1151b6a7e
https://hg.mozilla.org/integration/fx-team/rev/f279d53c2d4d
https://hg.mozilla.org/integration/fx-team/rev/c521a1683852
https://hg.mozilla.org/integration/fx-team/rev/7744ed51864c
https://hg.mozilla.org/integration/fx-team/rev/64376a7a08df

Comment 26

[Carsten Book [:Tomcat]]

(In reply to Lucas Rocha (:lucasr) from comment #25)

Hi Lucas,

> https://hg.mozilla.org/integration/fx-team/rev/f1769583f43b
> https://hg.mozilla.org/integration/fx-team/rev/c4e1151b6a7e
> https://hg.mozilla.org/integration/fx-team/rev/f279d53c2d4d
> https://hg.mozilla.org/integration/fx-team/rev/c521a1683852
> https://hg.mozilla.org/integration/fx-team/rev/7744ed51864c
> https://hg.mozilla.org/integration/fx-team/rev/64376a7a08df

had to backout this changes because of bustage in android builds like https://tbpl.mozilla.org/php/getParsedLog.php?id=31863842&tree=Fx-Team

Comment 27

[Lucas Rocha (:lucasr)]

This needs a clobber apparently. Touched CLOBBER file and pushed again:

https://hg.mozilla.org/integration/fx-team/rev/5968d1dd2b88
https://hg.mozilla.org/integration/fx-team/rev/1c7f69890209
https://hg.mozilla.org/integration/fx-team/rev/4780678d387f
https://hg.mozilla.org/integration/fx-team/rev/ec4673726b46
https://hg.mozilla.org/integration/fx-team/rev/ce3dd14c1840
https://hg.mozilla.org/integration/fx-team/rev/fe13b62902c8

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5968d1dd2b88
https://hg.mozilla.org/mozilla-central/rev/1c7f69890209
https://hg.mozilla.org/mozilla-central/rev/4780678d387f
https://hg.mozilla.org/mozilla-central/rev/ec4673726b46
https://hg.mozilla.org/mozilla-central/rev/ce3dd14c1840
https://hg.mozilla.org/mozilla-central/rev/fe13b62902c8

Comment 30

[Richard Newman [:rnewman]]

I think this regressed locale switching; home page tab labels no longer change when the locale switches. Please remember to test!