977980 - Add "Remove" as an option for dynamic Home panels

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Splitting this off from the Add Home panels bug.

Comment 1

[Chenxia Liu [:liuche]]

Attachment: Support "Remove" for dynamic panels

Comment 2

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Home panel context menu

Ian, do we want to support both "Hide" and "Remove" for non-built-in panels, or just "Remove"?

Comment 3

[Ian Barlow (:ibarlow)]

Just "remove" is fine.

Comment 4

[Chenxia Liu [:liuche]]

Attachment: WIP: Support "Remove" for dynamic panels

Rebased on top of "Add panels" patches.

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Patch: Display Hide and Remove for dynamic panels

This patch supports both "Hide" and "Remove".

Comment 6

[Chenxia Liu [:liuche]]

Attachment: (Part 2) Display only Remove for dynamic panels

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8385686 [details] [diff] [review]
(Part 2) Display only Remove for dynamic panels

Not sure if bug 978991 will be resolved soon - I'm inclined to do this as two separate patches and maybe just back one out if that's the case.

Comment 8

[Chenxia Liu [:liuche]]

Attachment: Patch: Display Remove for dynamic panels

Folded the patches to only display "Remove".

Comment 9

[:Margaret Leibovic]

Comment on attachment 8386363 [details] [diff] [review]
Patch: Display Remove for dynamic panels

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

Looking good, I just want to see an updated version before we land this.

::: mobile/android/base/preferences/PanelsPreference.java
@@ +15,5 @@
>  
>  public class PanelsPreference extends CustomListPreference {
>      protected String LOGTAG = "PanelsPreference";
>  
> +    private static final int INDEX_DISPLAY_BUTTON = 1;

I think you should add some comments to clarify that this button can be show/hide/remove depending on the type of panel and the state.

I actually feel like it might be a bit confusing to overload this button with so many meanings, but only having one button in the code is a good way to guarantee we only ever show one button in the UI.

@@ +28,2 @@
>          super(context, parentCategory);
> +        this.isRemovable = isRemovable;

I would prefer we stay consistent with the existing prefixing style in this file. Either that, or create a separate patch to change mIsHidden to isHidden. Although there are also prefixed protected fields in CustomListPreference, so maybe it would be easier to just add a prefix to the new field.

@@ +52,5 @@
>      @Override
> +    protected String[] createDialogItems() {
> +        if (isRemovable) {
> +            return new String[] { LABEL_SET_AS_DEFAULT, LABEL_REMOVE };
> +        } else {

You don't need an else statement since you're returning in the if statement.

@@ +85,5 @@
>                  break;
>  
> +            case INDEX_DISPLAY_BUTTON:
> +                if (isRemovable) {
> +                    mParentCategory.uninstall(this);

You should add some comments here about our assumptions about this display button, and the fact that if it is showing for a removable panel, it must be the "Remove" item.

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +70,1 @@
>          Resources res = getContext().getResources();

Don't need this res variable anymore.

Comment 10

[Chenxia Liu [:liuche]]

Attachment: Patch: Display Remove for dynamic panels

Comment 11

[:Margaret Leibovic]

Comment on attachment 8387362 [details] [diff] [review]
Patch: Display Remove for dynamic panels

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

r+ with comments addressed.

::: mobile/android/base/preferences/CustomListPreference.java
@@ +99,5 @@
>  
> +    private String[] getCachedDialogItems() {
> +        if (mDialogItems == null) {
> +            mDialogItems = createDialogItems();
> +        }

Why not still just set this in the constructor? You could still use this getter to wrap mDialogItems, but then it could still be final.

::: mobile/android/base/preferences/PanelsPreference.java
@@ +33,2 @@
>          super(context, parentCategory);
> +        this.mIsRemovable = isRemovable;

No need for the `this.`

::: mobile/android/base/preferences/PanelsPreferenceCategory.java
@@ +71,5 @@
>      }
>  
>      private void displayHomeConfig() {
>          for (PanelConfig panelConfig : mPanelConfigs) {
> +            boolean isRemovable = panelConfig.isDynamic();

Nit: final.

Comment 12

[Chenxia Liu [:liuche]]

(In reply to :Margaret Leibovic from comment #11)

> ::: mobile/android/base/preferences/CustomListPreference.java
> @@ +99,5 @@
> >  
> > +    private String[] getCachedDialogItems() {
> > +        if (mDialogItems == null) {
> > +            mDialogItems = createDialogItems();
> > +        }
> 
> Why not still just set this in the constructor? You could still use this
> getter to wrap mDialogItems, but then it could still be final.
> 
I wanted to enforce the setting of mDialogItems from createDialogItems in the superclass; setting these in the superclass constructor will result in all the string labels set by the subclass to be null, because those don't get assigned until the subclass constructor is called. Java also doesn't let you assign those string labels before calling the super constructor. :/

Addressed other nits.

Comment 13

[Chenxia Liu [:liuche]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/30d1f3ee8ad3