967742 - Consolidate API to edit HomeConfig state

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Lucas Rocha (:lucasr)]

Representing the state with a list is great for the read-only cases but not very robust for updating the list (see discussion in bug 964375). I'm thinking something similar to SharedPreferences's Editor.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Consolidate API to edit HomeConfig state (r=margaret)

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 8382104 [details] [diff] [review]
Consolidate API to edit HomeConfig state (r=margaret)

I initially dismissed this as just a 'refactoring' but it turns out this more important than it looks.

We currently have ad-hoc code for changing HomeConfig's state in different ways e.g. set new default, uninstall, install, hide, etc. Most of this code is in HomeConfigInvalidator and PanelsPrePanelsPreferenceCategory right now. The changes are done in a mutable list with mutable PanelConfig elements. This is a bit too hand-wavy, we might end up saving broken state (see bug 969060 for example).

This patch is an overhaul of how we make changes to HomeConfig's state. The main goal here is to ensure that HomeConfig changes always lead to a valid/correct state. It uses more appropriate data structures to represent the HomeConfig state for editing. For instance, we don't need to traverse the list to update the default panel, uninstall panels, etc.

It also fixes bug 969060 now but it will probably save us from some pain in the future.

Here's an overview of the changes:
- PanelConfig is now immutable from outside HomeConfig code i.e. you can change it via HomeConfig.Editor. 
- HomeConfig.load() and HomeConfig.save() now return/take a State instance which is an immutable representation of HomeConfig's state.
- Changes to State can only be done via an Editor which can be created by calling edit() in a State instance.
- Editor has methods to install, uninstall, update, set a new default panel, and enable/disable panels. In the future, we'll have to add a method to move a panel to a new position (once we get the UI for doing so).
- For now, I'm just using a LinkedHashMap which is order-aware. This is enough for now but we'll probably have to use an extra data structure to hold the panel positions once we add support for reordering panels.
- I intentionally made Editor not thread-safe to simplify things and because, well, we don't need it to be thread-safe.

There's more information in the javadocs. Requesting feedback from liuche too as this patch changes a lot of things in our panel prefs code.

Comment 3

[Chenxia Liu [:liuche]]

Comment on attachment 8382104 [details] [diff] [review]
Consolidate API to edit HomeConfig state (r=margaret)

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

Great, this is much cleaner! The changes to the Custom/Panels preferences look good to me.

::: mobile/android/base/home/HomeConfig.java
@@ +741,5 @@
> +     * it. This means that adding, removing, or updating panels in an
> +     * {@code Editor} will never change the {@code State} which you
> +     * created the {@code Editor} from. Calling {@code commit()} or
> +     * {@code apply()} will cause the new {@code State} instance to be
> +     * created and saved.

Clarify that this new State is saved to the HomeConfig instance.

@@ +884,5 @@
> +                return;
> +            }
> +
> +            panelConfig.setIsDisabled(disabled);
> +            mEnabledCount += (disabled ? -1 : 1);

I'd consider moving this update-and-*crement to a private function just so they're always done together, but on the other hand, we don't do this too much, so it might just be unnecessary functions.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8382104 [details] [diff] [review]
Consolidate API to edit HomeConfig state (r=margaret)

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

Sorry it took a while to finish this review, I really like how this consolidates the logic in HomeConfig. I know you feel similarly, but it would be so great to have unit tests for things like this, since it's a bit tedious to look through all the logic to try to make sure there are no bugs :)

::: mobile/android/base/home/HomeConfig.java
@@ +764,5 @@
> +
> +            initFromState(configState);
> +        }
> +
> +        private void initFromState(State configState) {

It would be nice to add some comments about how we use the initial configState to populate mConfigMap, and then use that data structure to keep track of the changes before committing the new state.

@@ +924,5 @@
> +            final String id = panelConfig.getId();
> +            if (!mConfigMap.containsKey(id)) {
> +                mConfigMap.put(id, panelConfig);
> +
> +                mEnabledCount++;

Maybe we should also perform a check here to make sure this panelConfig is enabled, and throw if its not. I am slightly worried about mEnabledCount somehow ending up being incorrect, since we're incrementing/decrementing it in multiple places, but in all the other places we're also explicitly setting (or checking) the enabled state of a panelConfig.

@@ +1024,5 @@
> +         * current thread.
> +         *
> +         * @return the resulting {@code State} instance.
> +         */
> +        public State commit() {

Can you elaborate on the difference between apply() and commit(), and when a consumer would choose to use one vs. the other?

::: mobile/android/base/home/HomePager.java
@@ +243,5 @@
>  
>          return super.dispatchTouchEvent(event);
>      }
>  
> +    private void updateUiFromPanelConfigs(HomeConfig.State configState) {

Nit: Rename this method to something like updateUiFromConfigState.

::: mobile/android/base/preferences/PanelsPreferenceCategory.java
@@ +106,5 @@
>  
>      /**
>       * Update HomeConfig off the main thread.
>       *
>       * @param panelConfigs Configuration to be saved

Not caused by you, but these docs are wrong. Also, is saveHomeConfig still used? I see this patch replacing lots of places where its used.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8382104 [details] [diff] [review]
Consolidate API to edit HomeConfig state (r=margaret)

Oops, I started my review before liuche posted hers, and it looks like I blew away her feedback+ flag.

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8382104 [details] [diff] [review]
Consolidate API to edit HomeConfig state (r=margaret)

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

::: mobile/android/base/home/HomeConfig.java
@@ +879,5 @@
> +        public void setDisabled(String panelId, boolean disabled) {
> +            ThreadUtils.assertOnThread(mOriginalThread);
> +
> +            final PanelConfig panelConfig = getPanelOrThrow(panelId);
> +            if (panelConfig.isDisabled() == disabled) {

Just a note - if we decide in bug 978991 that disabling is only for built-in panels, we might want to throw here if trying to disable a dynamic panel.

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Chenxia Liu [:liuche] from comment #3)
> Comment on attachment 8382104 [details] [diff] [review]
> Consolidate API to edit HomeConfig state (r=margaret)
> 
> Review of attachment 8382104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, this is much cleaner! The changes to the Custom/Panels preferences
> look good to me.
> 
> ::: mobile/android/base/home/HomeConfig.java
> @@ +741,5 @@
> > +     * it. This means that adding, removing, or updating panels in an
> > +     * {@code Editor} will never change the {@code State} which you
> > +     * created the {@code Editor} from. Calling {@code commit()} or
> > +     * {@code apply()} will cause the new {@code State} instance to be
> > +     * created and saved.
> 
> Clarify that this new State is saved to the HomeConfig instance.

Done.
 
> @@ +884,5 @@
> > +                return;
> > +            }
> > +
> > +            panelConfig.setIsDisabled(disabled);
> > +            mEnabledCount += (disabled ? -1 : 1);
> 
> I'd consider moving this update-and-*crement to a private function just so
> they're always done together, but on the other hand, we don't do this too
> much, so it might just be unnecessary functions.

Sounds good in terms of ensuring consistency. Done.

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #4)
> ::: mobile/android/base/home/HomeConfig.java
> @@ +764,5 @@
> > +
> > +            initFromState(configState);
> > +        }
> > +
> > +        private void initFromState(State configState) {
> 
> It would be nice to add some comments about how we use the initial
> configState to populate mConfigMap, and then use that data structure to keep
> track of the changes before committing the new state.

Done.
 
> @@ +924,5 @@
> > +            final String id = panelConfig.getId();
> > +            if (!mConfigMap.containsKey(id)) {
> > +                mConfigMap.put(id, panelConfig);
> > +
> > +                mEnabledCount++;
> 
> Maybe we should also perform a check here to make sure this panelConfig is
> enabled, and throw if its not. I am slightly worried about mEnabledCount
> somehow ending up being incorrect, since we're incrementing/decrementing it
> in multiple places, but in all the other places we're also explicitly
> setting (or checking) the enabled state of a panelConfig.

The install() method throws if the given PanelConfig is disabled just above.

> @@ +1024,5 @@
> > +         * current thread.
> > +         *
> > +         * @return the resulting {@code State} instance.
> > +         */
> > +        public State commit() {
> 
> Can you elaborate on the difference between apply() and commit(), and when a
> consumer would choose to use one vs. the other?

This is inspired by SharedPreferences' equivalent methods. In practice, apply() is used when you want to save the state off main thread (the Settings UI case) and commit() is when you're already off main thread and want to save the new state synchronously on the current thread (the HomeConfigInvalidator case). 

> ::: mobile/android/base/home/HomePager.java
> @@ +243,5 @@
> >  
> >          return super.dispatchTouchEvent(event);
> >      }
> >  
> > +    private void updateUiFromPanelConfigs(HomeConfig.State configState) {
> 
> Nit: Rename this method to something like updateUiFromConfigState.

Done.

> ::: mobile/android/base/preferences/PanelsPreferenceCategory.java
> @@ +106,5 @@
> >  
> >      /**
> >       * Update HomeConfig off the main thread.
> >       *
> >       * @param panelConfigs Configuration to be saved
> 
> Not caused by you, but these docs are wrong. Also, is saveHomeConfig still
> used? I see this patch replacing lots of places where its used.

Nice catch, this is method is unused now. Removed.

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #4)
> I know you feel similarly, but it would be so great to have unit tests for
> things like this, since it's a bit tedious to look through all the logic
> to try to make sure there are no bugs :)

Forgot to reply to this. Yes, I'm (slowly) working on unit tests (browser instrumentation tests) for the HomeConfig API. Filed bug 980979 to track this.

Comment 10

[Lucas Rocha (:lucasr)]

(In reply to Chenxia Liu [:liuche] from comment #6)
> Comment on attachment 8382104 [details] [diff] [review]
> Consolidate API to edit HomeConfig state (r=margaret)
> 
> Review of attachment 8382104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeConfig.java
> @@ +879,5 @@
> > +        public void setDisabled(String panelId, boolean disabled) {
> > +            ThreadUtils.assertOnThread(mOriginalThread);
> > +
> > +            final PanelConfig panelConfig = getPanelOrThrow(panelId);
> > +            if (panelConfig.isDisabled() == disabled) {
> 
> Just a note - if we decide in bug 978991 that disabling is only for built-in
> panels, we might want to throw here if trying to disable a dynamic panel.

I've just posted a reminder in bug 978991 to do this. Right now we do show 'Hide' on dynamic panels.

Comment 11

[Lucas Rocha (:lucasr)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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