949172 - Implement proper HomeConfigBackend to load and save HomePager configuration

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Lucas Rocha (:lucasr)]

Right now, we're just using a temporary static backend.

A few things to keep in mind:
- We shouldn't rely on the backend being a singleton
- External changes to the configuration source (be it a JSON file, a SharedPref, or whatever) should always trigger the attached OnChangeListener.
- Performance is really important. HomePager can't take too long to show up on startup.  Let's save as little as possible so that loading the data is as fast as possible. This also includes the time to parse the configuration in whatever format we choose.
- Whatever format we choose, the HomePager configuration should be stored per profile, not globally.

Comment 1

[Chenxia Liu [:liuche]]

I was looking at the product page, and I just wanted to mention that we also want to support "disabled" panels, which is not in the visual mocks.

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Implement backend to load and save HomePager configuration (r=margaret)

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8357897 [details] [diff] [review]
Implement backend to load and save HomePager configuration (r=margaret)

Tested this with local hacks (as there's no settings UI yet). Works fine.

Went with a JSON string stored in SharedPreferences. I considered storing it in a json file in the profile directory but using SharedPreferences gives all the thread-safety from the SharedPreferences API as well as key monitoring for free. We still need to implement proper per-profile shared preferences though (bug 940575).

I noticed that HomePager is not handling live config changes properly while testing this patch. This is a separate issue that I'll investigate in bug 958171.

The backend simply falls back to our current built-in configuration (history, top sites, bookmarks, reading list) if the configuration is undefined. This way we can simply hide the settings UI before the merge if we decide to skip this feature in 29.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8357897 [details] [diff] [review]
Implement backend to load and save HomePager configuration (r=margaret)

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

Nice, this is more straightforward than I thought it would be.

Just to make sure we're all on the same page, my understanding here is that we'll always load a default config until the user goes into the settings UI to change their preferences. And when that happens, the settings UI code will call save with that new list of page entries. And HomeConfig can register a change listener to update appropriately when these changes happen (I imagine that's what you'll do in bug 958171). Is that right?

For simplicity, I feel like it would be nice if we could just have a default shared preference with the default config, but I also like the solution here because it means we won't need to do any I/O unless the user changes the default preference, which is nice for first-run performance (we will need to deal with this for distributions that include some config file or whatever, but that's not something to worry about right now).

Also, I like that this removes the dependency on ListManager/PanelManager. As we discussed earlier today, the settings code can just ask PanelManager for the available panels and then change the set of panels in the config if the user chooses to do that.

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +180,5 @@
> +        editor.putString(PREFS_KEY, jsonString);
> +        editor.commit();
> +    }
> +
> +    public void setOnChangeListener(OnChangeListener listener) {

Nit: @Override, since this is implementing the interface.

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8357897 [details] [diff] [review]
Implement backend to load and save HomePager configuration (r=margaret)

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

This looks good to me too! I'll apply this under my patches for bug 942875 so I can use these changes.

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +30,5 @@
> +import java.util.EnumSet;
> +import java.util.List;
> +
> +class HomeConfigPrefsBackend implements HomeConfigBackend {
> +    private static final String LOGTAG = "GeckoHomeConfigPrefsBackend";

I think this is too long for the Android logger - tag length is limited to 23 characters.

http://developer.android.com/reference/android/util/Log.html#isLoggable%28java.lang.String,%20int%29

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #4)
> Comment on attachment 8357897 [details] [diff] [review]
> Implement backend to load and save HomePager configuration (r=margaret)
> 
> Review of attachment 8357897 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, this is more straightforward than I thought it would be.
> 
> Just to make sure we're all on the same page, my understanding here is that
> we'll always load a default config until the user goes into the settings UI
> to change their preferences. And when that happens, the settings UI code
> will call save with that new list of page entries.

Exactly.

> And HomeConfig can register a change listener to update appropriately when
> these changes happen (I imagine that's what you'll do in bug 958171). Is that right?

HomePager and HomeConfigLoader are already hooked up to live-update when the config changes. See:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeConfigLoader.java#38

When the config changes (i.e. the backend's OnChangeListener is triggered), HomeConfigLoader is automatically reloaded to fetch the latest list of panel entries (from a SharedPrefences key in this case).

Bug 958171 is more about live config changes leading to broken UI state.

> For simplicity, I feel like it would be nice if we could just have a default
> shared preference with the default config, but I also like the solution here
> because it means we won't need to do any I/O unless the user changes the
> default preference, which is nice for first-run performance (we will need to
> deal with this for distributions that include some config file or whatever,
> but that's not something to worry about right now).

Yeah, that's the intent: avoiding any unnecessary I/0 until the user changes the configuration. And it's also a clean way to keep the feature disabled if we decide to skip 29. 

> Also, I like that this removes the dependency on ListManager/PanelManager.
> As we discussed earlier today, the settings code can just ask PanelManager
> for the available panels and then change the set of panels in the config if
> the user chooses to do that.

Precisely ;-)

> ::: mobile/android/base/home/HomeConfigPrefsBackend.java
> @@ +180,5 @@
> > +        editor.putString(PREFS_KEY, jsonString);
> > +        editor.commit();
> > +    }
> > +
> > +    public void setOnChangeListener(OnChangeListener listener) {
> 
> Nit: @Override, since this is implementing the interface.

Nice catch, done.

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Chenxia Liu [:liuche] from comment #5)
> Comment on attachment 8357897 [details] [diff] [review]
> Implement backend to load and save HomePager configuration (r=margaret)
> 
> Review of attachment 8357897 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me too! I'll apply this under my patches for bug 942875
> so I can use these changes.
> 
> ::: mobile/android/base/home/HomeConfigPrefsBackend.java
> @@ +30,5 @@
> > +import java.util.EnumSet;
> > +import java.util.List;
> > +
> > +class HomeConfigPrefsBackend implements HomeConfigBackend {
> > +    private static final String LOGTAG = "GeckoHomeConfigPrefsBackend";
> 
> I think this is too long for the Android logger - tag length is limited to
> 23 characters.
> 
> http://developer.android.com/reference/android/util/Log.
> html#isLoggable%28java.lang.String,%20int%29

Renamed to GeckoHomeConfigBackend, thanks.

Comment 8

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/fec4987cbee9

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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