Splitting this off from the Add Home panels bug.

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

Just "remove" is fine.

Rebased on top of "Add panels" patches.

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

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.

Folded the patches to only display "Remove".

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

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