Let's make a place where users can manage their Home page lists. The first part of this will allow them to reorder lists (bug 941312), and the next part will be to allow new lists to be created from here as well. Designs will be posted shortly.

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.

Darnit, I forgot I filed this bug. I placed these mockups in two other related bugs today, but here they are again in case this is a better home for them.

(In reply to Ian Barlow (:ibarlow) from comment #2) > Created attachment 8345401 [details] > Mockups > > Darnit, I forgot I filed this bug. I placed these mockups in two other > related bugs today, but here they are again in case this is a better home > for them. I forgive you. There are a lot of bugs floating around here :) Also, Lucas, I know you're going on PTO soon. We should focus on trying to land bug 942231, but if you haven't had time to get to this bug, somebody else can also pick it up to work on the UI bits in parallel.

(In reply to :Margaret Leibovic from comment #3) > (In reply to Ian Barlow (:ibarlow) from comment #2) > > Created attachment 8345401 [details] > > Mockups > > > > Darnit, I forgot I filed this bug. I placed these mockups in two other > > related bugs today, but here they are again in case this is a better home > > for them. > > I forgive you. There are a lot of bugs floating around here :) > > Also, Lucas, I know you're going on PTO soon. We should focus on trying to > land bug 942231, but if you haven't had time to get to this bug, somebody > else can also pick it up to work on the UI bits in parallel. Yep, once bug 942231 lands, there will definitely be room for some parallel work around the settings UI and the config storage backend.

This doesn't do anything useful yet, but it does create the settings page. For now I just stuck "Home page lists" below "Search settings" in the "Customize" page. I didn't see a spec for where it should actually go, so I'll need to follow up with ibarlow about that. This is my first time hacking on this preferences code, and there are a lot of different XML files, so I want to make sure I'm not missing something :)

Comment on attachment 8348504 [details] [diff] [review] (Part 1 - WIP) Create "Home page lists" settings page Review of attachment 8348504 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! Glad you added stuff to the tablet layout. Make sure to update the robocop tests as well to include these new items. Also, seeing the string "Home page lists" for the first time, I found it a little weird, but iirc there's already been a lot of discussion about about strings and naming. ::: mobile/android/base/resources/layout/preference_list.xml @@ +11,5 @@ > + android:paddingLeft="15dp" > + android:paddingRight="?android:attr/scrollbarSize"> > + > + <TextView android:id="@+android:id/title" > + android:layout_width="wrap_content" Nit: align subsequent android:* lines with the first android:id.

Search settings and Lists will be very similar, so refactoring for sharing code.

Updated to be on top of bug 958189 and bug 958179.

This ballooned a lot when adding support for the disabled state of panels.

This patch adds persisting of disabled state, which I split out of Part 3 because that was getting huge. Needless to say, disabled state doesn't work completely without this patch.

Lucas, just a note: I wasn't able to use the HomeConfigLoader because Preferences don't have access to Activity or Fragment (and we don't use Fragments in any case on the older devices). I would be happy to make these calls async/off-the-UI-thread if you think they need to be. I also added the code to handle disabled state, but I have to hunt down a problem where the HomePager caches the panel index, which causes problems with displaying the Home Panels. I've also noticed a problem where the TabStrip doesn't display a particular panel that has just been disabled/removed, but you can still scroll to it...

Comment on attachment 8360172 [details] [diff] [review] Part 3: Add Lists Review of attachment 8360172 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a good start. You need to avoid calling load() and save() in the main thread. Using something like UIAsyncTask should do it. You'll have to update the backend to save the disabled state for each PanelConfig. It should be analogous to the default bits. ::: mobile/android/base/home/HomeConfig.java @@ +159,5 @@ > + public void setIsDisabled(boolean isDisabled) { > + if (isDisabled) { > + mFlags.add(Flags.DISABLED_PANEL); > + } else { > + mFlags.remove(Flags.DISABLED_PANEL); nit: fix indentation ::: mobile/android/base/locales/en-US/android_strings.dtd @@ +84,5 @@ > <!ENTITY pref_developer_remotedebugging "Remote debugging"> > <!ENTITY pref_developer_remotedebugging_docs "Learn more"> > <!ENTITY pref_remember_signons "Remember passwords"> > > +<!ENTITY pref_category_home_panels "Home page panels"> I think we've decided to use just 'Home' for any user-visible communication around about:home. Please check with ibarlow. With that in mind, this should probably be 'Home panels' instead. @@ +86,5 @@ > <!ENTITY pref_remember_signons "Remember passwords"> > > +<!ENTITY pref_category_home_panels "Home page panels"> > +<!ENTITY pref_category_panels "Panels"> > +<!ENTITY pref_panels_tip "Change which list appears by default on your Firefox home page by tapping on it. You can also change the order of these lists by dragging them up and down."> which list -> which panel Double-check the wording with ibarlow. For instance, I think we want to go with 'Firefox home' instead. @@ +157,5 @@ > <!ENTITY pref_vendor_feedback "Give feedback"> > <!ENTITY pref_search_set_default "Set as default"> > <!ENTITY pref_search_default "Default"> > <!ENTITY pref_search_remove "Remove"> > +<!ENTITY pref_search_last_item_msg "You can\'t remove your last search engine."> Unrelated? ::: mobile/android/base/preferences/CustomListCategory.java @@ +68,5 @@ > */ > public void setDefault(CustomListPreference item) { > + if (mDefaultReference != null) { > + mDefaultReference.setIsDefault(false); > + } nit: add empty line here. ::: mobile/android/base/preferences/PanelsPreference.java @@ +70,5 @@ > + if (isDefault) { > + setSummary(LABEL_IS_DEFAULT); > + if (mIsHidden) { > + Log.e(LOGTAG, "unhiding the panel for setting default."); > + // Unhide the panel if it's being set as the default. The comment is a bit redundant given the log call above. ::: mobile/android/base/preferences/PanelsPreferenceCategory.java @@ +21,5 @@ > + public static final String LOGTAG = "PanelsPrefCategory"; > + > + private static final int LOADER_ID_HOME_CONFIG = 0; > + > + protected final List<PanelConfig> panels = new ArrayList<PanelConfig>(); panels = mPanelConfigs (for style consistency with the backend code) @@ +25,5 @@ > + protected final List<PanelConfig> panels = new ArrayList<PanelConfig>(); > + protected HomeConfig mHomeConfig; > + > + // These seemingly redundant constructors are mandated by the Android system, else it fails to > + // inflate this object. This is very common in Android code, this comment is not strictly necessary. @@ +40,5 @@ > + > + public PanelsPreferenceCategory(Context context) { > + super(context); > + initConfig(context); > + } nit: We usually have these constructors in the opposite order, from simple to more complex. (Context) -> (Context, AttributeSet) -> (Context, AttributeSet, int) @@ +84,5 @@ > + > + @Override > + public void onChange() { > + panels.clear(); > + removeAll(); Shouldn't these calls be part of loadFromConfig itself? You always want to cleanup the current rows before loading a new set of PanelConfigs, no? @@ +86,5 @@ > + public void onChange() { > + panels.clear(); > + removeAll(); > + > + loadFromConfig(); IIUC, this is going to cause a full refresh of the list of panels every time you save the new config, right? Is that intentional? loadFromConfig() should never be called in main thread btw. @@ +95,5 @@ > + super.setDefault(pref); > + > + final String mId = pref.getKey(); > + > + for (PanelConfig entry : panels) { entry -> panelConfig @@ +103,5 @@ > + entry.setIsDefault(false); > + } > + } > + > + mHomeConfig.save(panels); This has to happen off main thread. @@ +112,5 @@ > + super.uninstall(pref); > + final String mId = pref.getKey(); > + > + PanelConfig toRemove = null; > + for (PanelConfig entry : panels) { entry -> panelConfig @@ +120,5 @@ > + } > + } > + panels.remove(toRemove); > + > + mHomeConfig.save(panels); Should be called off main thread. @@ +139,5 @@ > + break; > + } > + } > + > + mHomeConfig.save(panels); Ditto. @@ +149,5 @@ > + private void setDefaultForHiding(PanelsPreference pref, boolean toHide) { > + if (toHide) { > + // Set a default if there is an enabled panel left. > + if (pref == mDefaultReference) { > + setFallbackDefault(); What's this 'fallback default' about? ::: mobile/android/base/preferences/SearchEnginePreference.java @@ +76,5 @@ > if (mParentCategory.getPreferenceCount() == 1) { > ThreadUtils.postToUiThread(new Runnable() { > @Override > public void run() { > + Toast.makeText(getContext(), R.string.pref_search_last_item_msg, Toast.LENGTH_SHORT).show(); Unrelated? Change this in a separate patch?

Ian, some screenshots of the current state I have for Home Panels settings. A few things: - I dropped the "Restore defaults" because from the product page, my understanding is that we won't allow removal for built-in panels, just disabling. - Disabling panels: The text for disabling/enabling is "Hide" and "Show," and they toggle. All the panels can be disabled, and the first one to be enabled again will be set as the default. - Setting default: Any panel can be set as the default, including disabled ones (at which point they will be enabled). - Some feedback on strings, now that the Renaming Revolution has calmed down a little - we're changing "Home Lists" to "Home Panels." Specifically the "tip" string (but also the dialog, I guess). Probably just renaming "pages" to "panels," I assume.

(In reply to Chenxia Liu [:liuche] from comment #18) > Ian, some screenshots of the current state I have for Home Panels settings. Woohoo! > A few things: > > - I dropped the "Restore defaults" because from the product page, my > understanding is that we won't allow removal for built-in panels, just > disabling. That seems reasonable. > - Disabling panels: The text for disabling/enabling is "Hide" and "Show," > and they toggle. All the panels can be disabled, and the first one to be > enabled again will be set as the default. What do we do when they are all disabled? Show a blank page? > - Setting default: Any panel can be set as the default, including disabled > ones (at which point they will be enabled). Perfect. > - Some feedback on strings, now that the Renaming Revolution has calmed down > a little - we're changing "Home Lists" to "Home Panels." Specifically the > "tip" string (but also the dialog, I guess). Probably just renaming "pages" > to "panels," I assume. Sure, here is some tweaked text you can use: "Change which panel appears by default on your Firefox Homepage by tapping on it. You can also change the order of these panels by dragging them up and down." I also think we can change the name of the section itself to "Home", for simplicity's sake.

Ian, for consistency, Search and Home preference titles should match in the Customize screen - I have a slight preference for just "Home" and "Search", though I can see how there could be some confusion about "Search" being a verb (though this should be mitigated by "Home" clearly not being a verb).

Un-bitrot.

Comment on attachment 8358564 [details] [diff] [review] Part 2: Consolidate some strings v2 This does some renaming of string names to make them more general, for use with the Home Panels settings, but since I didn't actually modify the strings themselves, I didn't rename the dtd resources. (Let me know if that's something I should do, for consistency, or just leave them the way they are now.)

Comment on attachment 8358564 [details] [diff] [review] Part 2: Consolidate some strings v2 Review of attachment 8358564 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/strings.xml.in @@ +164,5 @@ > <string name="pref_vendor_feedback">&pref_vendor_feedback;</string> > > + <string name="pref_dialog_set_default">&pref_search_set_default;</string> > + <string name="pref_dialog_remove">&pref_search_remove;</string> > + <string name="pref_default">&pref_search_default;</string> Drive-by: I feel like you should also update these android_strings.dtd, so that localizers can know they're used more generally than just in the search settings.

(In reply to Chenxia Liu [:liuche] from comment #23) > Comment on attachment 8358564 [details] [diff] [review] > Part 2: Consolidate some strings v2 > > This does some renaming of string names to make them more general, for use > with the Home Panels settings, but since I didn't actually modify the > strings themselves, I didn't rename the dtd resources. (Let me know if > that's something I should do, for consistency, or just leave them the way > they are now.) Haha, made my last comment before seeing this.

(In reply to Chenxia Liu [:liuche] from comment #20) > Created attachment 8360671 [details] > Screenshot: Home settings > > Ian, for consistency, Search and Home preference titles should match in the > Customize screen - I have a slight preference for just "Home" and "Search", > though I can see how there could be some confusion about "Search" being a > verb (though this should be mitigated by "Home" clearly not being a verb). I agree. Maybe we should re-order them too, and place Home first. So it would be Home Search Tabs Import from Android (also moved this down as it's kind of the oddball of the group)

Comment on attachment 8360711 [details] [diff] [review] Part 1: Refactor Search settings v3 Review of attachment 8360711 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I do wonder why we need these postToUiThread() calls for things that are supposed to be called from the main thread anyway. Not entirely sold on the CustomList* class names but I can't think of a better alternative :-) I'm a bit concerned about how we're going to implement drag-n-drop behaviour on top of PreferenceCategory. Makes me wonder if we'll end up having to implement this with a ListView somehow. ::: mobile/android/base/preferences/CustomListPreference.java @@ +21,5 @@ > + * This preference con display a dialog when clicked, and also supports > + * being set as a default item within the preference list category. > + */ > + > +public abstract class CustomListPreference extends Preference implements View.OnLongClickListener { I'd prefer us to encapsulate things like OnLongClickListener in a private inner class. Doing that in the top-level class means you're making onLongClick() part of CustomListPreference's public API which doesn't seem to be the intent here? @@ +135,5 @@ > + /** > + * (Optional) Configure the AlertDialog builder. > + */ > + protected void configureDialogBuilder(AlertDialog.Builder builder) { > + return; No need to call return here. Maybe configureDialogBuilder() should be an abstract method? Or do you actually want it to be optionally overridden? @@ +149,5 @@ > + protected void configureShownDialog() { > + // If this is already the default list item, disable the button for setting this as the default. > + final TextView defaultButton = (TextView) mDialog.getListView().getChildAt(INDEX_SET_DEFAULT_BUTTON); > + if (mIsDefault) { > + defaultButton.setEnabled(false); nit: add empty line here. @@ +160,5 @@ > + /** > + * Hide the dialog we previously created, if any. > + */ > + public void hideDialog() { > + ThreadUtils.postToUiThread(new Runnable() { Same here: is hideDialog() called off main thread? ::: mobile/android/base/preferences/SearchEnginePreference.java @@ +50,5 @@ > */ > @Override > protected void onBindView(View view) { > super.onBindView(view); > + Unrelated? @@ +76,5 @@ > + public void showDialog() { > + // If this is the last engine, then we are the default, and none of the options > + // on this menu can do anything. > + if (mParentCategory.getPreferenceCount() == 1) { > + ThreadUtils.postToUiThread(new Runnable() { Isn't showDialog() called from the UI thread? Why do you need to postToUiThread()? @@ +94,5 @@ > + // Copy the icon from this object to the prompt we produce. We lazily create the drawable, > + // as the user may not ever actually tap this object. > + if (mPromptIcon == null && mIconBitmap != null) { > + mPromptIcon = new BitmapDrawable(getContext().getResources(), mFaviconView.getBitmap()); > + } nit: add empty line here. @@ +103,5 @@ > + protected void onDialogIndexClicked(int index) { > + switch (index) { > + case INDEX_SET_DEFAULT_BUTTON: > + mParentCategory.setDefault(this); > + break; nit: empty line between each 'case'.

Comment on attachment 8358564 [details] [diff] [review] Part 2: Consolidate some strings v2 Review of attachment 8358564 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: mobile/android/base/strings.xml.in @@ +164,5 @@ > <string name="pref_vendor_feedback">&pref_vendor_feedback;</string> > > + <string name="pref_dialog_set_default">&pref_search_set_default;</string> > + <string name="pref_dialog_remove">&pref_search_remove;</string> > + <string name="pref_default">&pref_search_default;</string> What margaret said :-)

Just to respond to a few things right here (the non-code bits). > > @@ +149,5 @@ > > + private void setDefaultForHiding(PanelsPreference pref, boolean toHide) { > > + if (toHide) { > > + // Set a default if there is an enabled panel left. > > + if (pref == mDefaultReference) { > > + setFallbackDefault(); > > What's this 'fallback default' about? This is called if you try to hide/disable the default - if there are still enabled panel items, we should set one of them as a default. > > I'm a bit concerned about how we're going to implement drag-n-drop behaviour > on top of PreferenceCategory. Makes me wonder if we'll end up having to > implement this with a ListView somehow. I started looking at various drag-and-drop code snippets, and you're right, all of those are for ListViews. There is also the problem with older APIs, because API 11 includes DragListeners, but 8-10 don't have that.

(In reply to Ian Barlow (:ibarlow) from comment #19) > > > - Disabling panels: The text for disabling/enabling is "Hide" and "Show," > > and they toggle. All the panels can be disabled, and the first one to be > > enabled again will be set as the default. > > What do we do when they are all disabled? Show a blank page? Yep! The current implementation of Home Pages does this.

Carrying over r+ from previous review. (In reply to Lucas Rocha (:lucasr) from comment #27) > Comment on attachment 8360711 [details] [diff] [review] > Part 1: Refactor Search settings v3 > > Review of attachment 8360711 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/preferences/CustomListPreference.java > @@ +21,5 @@ > > + * This preference con display a dialog when clicked, and also supports > > + * being set as a default item within the preference list category. > > + */ > > + > > +public abstract class CustomListPreference extends Preference implements View.OnLongClickListener { > > I'd prefer us to encapsulate things like OnLongClickListener in a private > inner class. Doing that in the top-level class means you're making > onLongClick() part of CustomListPreference's public API which doesn't seem > to be the intent here? Thanks for pointing this out - this is part of a hack to allow for long-clicks on the Preference, which I had forgotten to update - this needs to be public so that GeckoPreferences can call this on pre-ICS devices, which I've added to this patch. > > @@ +135,5 @@ > > + /** > > + * (Optional) Configure the AlertDialog builder. > > + */ > > + protected void configureDialogBuilder(AlertDialog.Builder builder) { > > + return; > > No need to call return here. Maybe configureDialogBuilder() should be an > abstract method? Or do you actually want it to be optionally overridden? > I wanted this to explicitly be optional, because this is for any additional configuration of the Dialog. SearchEngine uses this to set an icon in the Dialog. > > Isn't showDialog() called from the UI thread? Why do you need to > postToUiThread()? Good call, I checked the code paths and you're correct. Fixed.

Renamed strings, carrying over r+.

I combined the Lists and Disabled state patches. I also extracted the "Panels tip" since these patches won't support dragging and dropping (which I split into bug 959917). I also removed the onChange handling per earlier discussion, and pulled the HomeConfig code into UiAsyncTasks.

Panels tip for managing panels and reordering by dragging and dropping.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=c504064be02c

Comment on attachment 8361425 [details] [diff] [review] Part 3: Add Lists + tests Review of attachment 8361425 [details] [diff] [review]: ----------------------------------------------------------------- Look nice but needs some thread-safety fixes and other tweaks. ::: mobile/android/base/home/HomePager.java @@ +300,5 @@ > + // Only keep enabled panels. > + final List<PanelConfig> enabledPanels = new ArrayList<PanelConfig>(); > + > + for (PanelConfig panel : panelConfigs) { > + if (!panel.isDisabled()) { I wonder if we should rename DISABLED to HIDDEN so that it matches the UI terminology? I realize this code is shared with the search stuff. File a follow-up if you think this is something we should do. @@ +311,3 @@ > > + // Hide the tab strip if the new configuration contains no panels. > + final int count = enabledPanels.size(); nit: remove empty line here. ::: mobile/android/base/preferences/PanelsPreference.java @@ +58,5 @@ > + protected String[] getDialogStrings() { > + Resources res = getContext().getResources(); > + // XXX: Don't provide the "Remove" string for now, because we only support built-in > + // panels, which can only be disabled. > + return new String[] { res.getString(R.string.pref_dialog_set_default), Why not having a cached SET_AS_DEFAULT_LABEL too? ::: mobile/android/base/preferences/PanelsPreferenceCategory.java @@ +21,5 @@ > + > +public class PanelsPreferenceCategory extends CustomListCategory { > + public static final String LOGTAG = "PanelsPrefCategory"; > + > + private static final int LOADER_ID_HOME_CONFIG = 0; Unused? @@ +55,5 @@ > + > + /** > + * Load the Home Panels config and populate the preferences screen and maintain local state. > + */ > + private void loadFromConfig() { Maybe loadHomeConfig() instead? @@ +58,5 @@ > + */ > + private void loadFromConfig() { > + // Clear the local cached version of panels. > + mPanelConfigs.clear(); > + removeAll(); Are these lines still needed given that loadFromConfig() is guaranteed to only be called once when the UI is first shown? @@ +69,5 @@ > + return mHomeConfig.load(); > + } > + > + @Override > + public void onPostExecute(List<PanelConfig> panelConfigs) { It's probably a good idea to safeguard against the case where the preference UI is destroyed before the UIAsyncTask returns its results. Does PreferenceCategory provide any API to track its destruction? You can keep a reference of the UIAsyncTask and cancel it if the PreferenceCategory gets destroyed before it finishes. @@ +70,5 @@ > + } > + > + @Override > + public void onPostExecute(List<PanelConfig> panelConfigs) { > + for (PanelConfig panel: panelEntries) { Does that even build? In any case, for clarity: panel -> panelConfig panelEntries -> panelConfigs @@ +118,5 @@ > + super.uninstall(pref); > + final String mId = pref.getKey(); > + > + PanelConfig toRemove = null; > + for (PanelConfig panel : mPanelConfigs) { For consistency: panel -> panelConfig @@ +145,5 @@ > + break; > + } > + } > + > + savePanelConfigs(mPanelConfigs); It's not safe to use mPanelConfigs directly in savePanelConfigs() as both the list itself and its PanelConfig instances might change while HomeConfig saves the list off main thread e.g. set a new default panel while HomeConfig.save() is running in the background thread. This is likely to lead us to save a list of PanelConfigs in an invalid state. One possible way to make this code thread-safe is to clone the list and its PanelConfig instances to be used in the HomeConfig.save() call. Or something else along these lines. @@ +148,5 @@ > + > + savePanelConfigs(mPanelConfigs); > + } > + > + private void savePanelConfigs(final List<PanelConfig> panelConfigs) { Maybe saveHomeConfig() instead? I'd move saveHomeConfig() and loadHomeConfig() to stay together in the class.

This patch separates out the save calls for disabling and setting defaults so only one call to save() is necessary. The deep-copy of the panels list is in the next patch.

I tried using the parcels to write/read the Panels, but there seems to be a problem with creating a panel from a Parcel - the fields are null, even though the PanelConfig isn't null. Here's an explicit-copy alternative.

I tried this patch originally, to use the built-in methods for creating PanelConfig.

Comment on attachment 8362449 [details] [diff] [review] Alternative: Part 3b: Parcel read/write Log excerpt - maybe the problem is in trying to write to Parcel from the Settings activity? 01-20 02:20:57.651: E/DatabaseUtils(2955): Writing exception to parcel 01-20 02:20:57.651: E/DatabaseUtils(2955): java.lang.SecurityException: Permission Denial: get/set setting for user asks to run as user -2 but is calling from user 0; this requires android.permission.INTERACT_ACROSS_USERS_FULL 01-20 02:20:57.651: E/DatabaseUtils(2955): at com.android.server.am.ActivityManagerService.handleIncomingUser(ActivityManagerService.java:13082) 01-20 02:20:57.651: E/DatabaseUtils(2955): at android.app.ActivityManager.handleIncomingUser(ActivityManager.java:2038) 01-20 02:20:57.651: E/DatabaseUtils(2955): at com.android.providers.settings.SettingsProvider.callFromPackage(SettingsProvider.java:577) 01-20 02:20:57.651: E/DatabaseUtils(2955): at android.content.ContentProvider$Transport.call(ContentProvider.java:279) 01-20 02:20:57.651: E/DatabaseUtils(2955): at android.content.ContentProviderNative.onTransact(ContentProviderNative.java:273) 01-20 02:20:57.651: E/DatabaseUtils(2955): at android.os.Binder.execTransact(Binder.java:388) 01-20 02:20:57.651: E/DatabaseUtils(2955): at dalvik.system.NativeStart.run(Native Method) 01-20 02:20:57.651: W/ActivityManager(2955): Permission Denial: get/set setting for user asks to run as user -2 but is calling from user 0; this requires android.permission.INTERACT_ACROSS_USERS_FULL

Comment on attachment 8362447 [details] [diff] [review] Part 3b: Copy panels list for update v1 Review of attachment 8362447 [details] [diff] [review]: ----------------------------------------------------------------- Yep. ::: mobile/android/base/preferences/PanelsPreferenceCategory.java @@ +123,5 @@ > + } > + final PanelConfig pc = new PanelConfig(panelConfig.getType(), > + panelConfig.getTitle(), > + panelConfig.getId(), > + flagsSet); This code should actually be in a PanelConfig constructor that takes another PanelConfig as an argument. This way you can validate the cloned instance (see patch in bug 959777). @@ +182,5 @@ > } > } > mPanelConfigs.remove(toRemove); > > + saveHomeConfig(deepCopyConfigList(mPanelConfigs)); Maybe saveHomeConfig() should simply do the deep copy of mPanelConfigs everytime you call it? This is to avoid callers passing mPanelConfigs directly altogether.

Comment on attachment 8362449 [details] [diff] [review] Alternative: Part 3b: Parcel read/write Review of attachment 8362449 [details] [diff] [review]: ----------------------------------------------------------------- No need to get fancy like this :-) The other approach is simpler and easier to understand.

Comment on attachment 8362444 [details] [diff] [review] Part 3: Add Lists + tests v2 Review of attachment 8362444 [details] [diff] [review]: ----------------------------------------------------------------- Nice. This should be good go with the cloning code from the other patch and the suggested changes here. ::: mobile/android/base/home/HomeConfigPrefsBackend.java @@ +161,5 @@ > } > > + if (PanelConfig.isDisabled()) { > + jsonPanelConfig.put(JSON_KEY_DISABLED, IS_DISABLED); > + } This code is moving to a PanelConfig constructor (see bug 959777). You'll need a rebase. ::: mobile/android/base/home/HomePager.java @@ +299,5 @@ > > + // Only keep enabled panels. > + final List<PanelConfig> enabledPanels = new ArrayList<PanelConfig>(); > + > + for (PanelConfig panel : panelConfigs) { panel -> panelConfig ::: mobile/android/base/preferences/PanelsPreferenceCategory.java @@ +25,5 @@ > + > + protected HomeConfig mHomeConfig; > + protected final List<PanelConfig> mPanelConfigs = new ArrayList<PanelConfig>(); > + > + protected UiAsyncTask<Void, Void, List<PanelConfig>> loadTask = null; loadTask = mLoadTask saveTask = mSaveTask Strictly speaking, no need to initialize them. @@ +58,5 @@ > + /** > + * Load the Home Panels config and populate the preferences screen and maintain local state. > + */ > + private void loadHomeConfig() { > + final PanelsPreferenceCategory thisParentCategory = this; You could simply use PanelsPreferenceCategory.this instead. @@ +66,5 @@ > + return mHomeConfig.load(); > + } > + > + @Override > + public void onPostExecute(List<PanelConfig> panelConfigs) { I'd move this code in onPostExecute() into a separate method. @@ +136,5 @@ > + mId = mDefaultReference.getKey(); > + } > + > + for (PanelConfig panelConfig : mPanelConfigs) { > + if (panelConfig.getId().equals(mId)) { TextUtils.equals() @@ +154,5 @@ > + > + final String mId = pref.getKey(); > + PanelConfig toRemove = null; > + for (PanelConfig panelConfig : mPanelConfigs) { > + if (panelConfig.getId().equals(mId)) { Use the safer TextUtils.equals() instead. @@ +177,5 @@ > + ensureDefaultForHide(pref, toHide); > + > + final String mId = pref.getKey(); > + for (PanelConfig panelConfig : mPanelConfigs) { > + if (panelConfig.getId().equals(mId)) { Ditto for TextUtils.equals() ::: mobile/android/base/preferences/SearchEnginePreference.java @@ +67,5 @@ > */ > @Override > protected String[] getDialogStrings() { > Resources res = getContext().getResources(); > + return new String[] { LABEL_SET_AS_DEFAULT, The LABEL_SET_AS_DEFAULT change should (ideally) go in a separate patch btw :-)

Moved the LABEL_SET_AS_DEFAULT to this patch.

Addressed comments and moved deep copy cod to HomeConfig.

This would be folded into Part 3.

(In reply to Chenxia Liu [:liuche] from comment #46) > Created attachment 8363319 [details] [diff] [review] > Part 3: Add Lists + tests v3 > > Addressed comments and moved deep copy cod to HomeConfig. By "moved deep copy code" I actually mean I moved the DISABLED_PANEL flag to HomeConfig (since deep copy is on patch 3b).

Comment on attachment 8363321 [details] [diff] [review] Part 3b: Copy panels list for update v2 Review of attachment 8363321 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Needs some tweaking. ::: mobile/android/base/home/HomeConfig.java @@ +165,5 @@ > > validate(); > } > > + public PanelConfig(PanelConfig mPanelConfig) { mPanelConfig -> panelConfig Also, I'd try to set the members on the same order than the other constructors. This means mFlags should be set last (after mViews). @@ +166,5 @@ > validate(); > } > > + public PanelConfig(PanelConfig mPanelConfig) { > + EnumSet<Flags> flagsSet = EnumSet.noneOf(Flags.class); You can simply clone the enumset from the given PanelConfig instance like: mFlags = panelConfig.mFlags.clone(); @@ +176,5 @@ > + if (mPanelConfig.isDisabled()) { > + flagsSet.add(Flags.DISABLED_PANEL); > + } > + > + mType = mPanelConfig.getType(); The nice thing about doing this in a constructor is that you don't need to use the public API for cloning the PanelConfig instance. So, you can do: mType = panelConfig.mType; mTitle = panelConfig.mTitle; ... and so on. @@ +180,5 @@ > + mType = mPanelConfig.getType(); > + mTitle = mPanelConfig.getTitle(); > + mId = mPanelConfig.getId(); > + mFlags = flagsSet; > + mLayoutType = getLayoutType(); nit: add empty line here. @@ +210,5 @@ > > validate(); > } > > + unrelated? @@ +472,5 @@ > > validate(); > } > > + public ViewConfig(ViewConfig mViewConfig) { mViewConfig -> viewConfig @@ +473,5 @@ > validate(); > } > > + public ViewConfig(ViewConfig mViewConfig) { > + mType = mViewConfig.getType(); Same here. You can simply do: mType = viewConfig.mType; mDatasetId = viewConfig.mDatasetId; @@ +474,5 @@ > } > > + public ViewConfig(ViewConfig mViewConfig) { > + mType = mViewConfig.getType(); > + mDataSetId = mViewConfig.getDataSetId(); mDataSetId -> mDatasetId ::: mobile/android/base/preferences/PanelsPreferenceCategory.java @@ +123,5 @@ > @Override > public void setDefault(CustomListPreference pref) { > super.setDefault(pref); > updateConfigDefault(); > + saveHomeConfig(makeConfigListDeepCopy()); As I suggested in my previous review, I'd probably move the makeConfigListDeepCopy() call into saveHomeConfig() to force a consistent safe behaviour i.e. don't rely on the caller doing the right thing.

(In reply to Lucas Rocha (:lucasr) from comment #49) > Comment on attachment 8363321 [details] [diff] [review] > Part 3b: Copy panels list for update v2 > > ::: mobile/android/base/home/HomeConfig.java > @@ +165,5 @@ > > > > validate(); > > } > > > > + public PanelConfig(PanelConfig mPanelConfig) { > > mPanelConfig -> panelConfig > > Also, I'd try to set the members on the same order than the other > constructors. This means mFlags should be set last (after mViews). > Oh naming...When would you use m* versus just the lowercased object? When it's a member of a class? As for ordering, I guess I was looking at the *one* constructor where the ordering is different, and doesn't set mFlags last. Should I change that to match in this patch? > > The nice thing about doing this in a constructor is that you don't need to > use the public API for cloning the PanelConfig instance. So, you can do: > > mType = panelConfig.mType; > mTitle = panelConfig.mTitle; > ... and so on. > Thanks! I didn't realize that. > ::: mobile/android/base/preferences/PanelsPreferenceCategory.java > @@ +123,5 @@ > > @Override > > public void setDefault(CustomListPreference pref) { > > super.setDefault(pref); > > updateConfigDefault(); > > + saveHomeConfig(makeConfigListDeepCopy()); > > As I suggested in my previous review, I'd probably move the > makeConfigListDeepCopy() call into saveHomeConfig() to force a consistent > safe behaviour i.e. don't rely on the caller doing the right thing. Done.

Comment on attachment 8363723 [details] [diff] [review] Part 3b: Copy panels list for update v3 Review of attachment 8363723 [details] [diff] [review]: ----------------------------------------------------------------- Looks nice with the suggested changes. ::: mobile/android/base/home/HomeConfig.java @@ +193,5 @@ > List<ViewConfig> views, EnumSet<Flags> flags) { > mType = type; > mTitle = title; > mId = id; > + mLayoutType = layoutType; no need for empty line in this case. ::: mobile/android/base/preferences/PanelsPreferenceCategory.java @@ +105,4 @@ > mSaveTask = new UiAsyncTask<Void, Void, Void>(ThreadUtils.getBackgroundHandler()) { > @Override > public Void doInBackground(Void... params) { > + final List<PanelConfig> panelConfigs = makeConfigListDeepCopy(); This has to happen in the main thread for safety. So, at the top of saveHomeConfig(): final List<PanelConfig> panelConfigs = makeConfigListDeepCopy();

Rebased on m-c.

...with changes suggested in the review.

https://hg.mozilla.org/integration/fx-team/rev/c2b850565e10 https://hg.mozilla.org/integration/fx-team/rev/d1d52d7eed98 https://hg.mozilla.org/integration/fx-team/rev/385ed622a7da

https://hg.mozilla.org/mozilla-central/rev/c2b850565e10 https://hg.mozilla.org/mozilla-central/rev/d1d52d7eed98 https://hg.mozilla.org/mozilla-central/rev/385ed622a7da

TC added in Moztrap: https://moztrap.mozilla.org/manage/case/13912/ The Home Settings is present (last 2 screenshots represent the developed behavior - missing the _ to add new ones from first mockups) The are 2 main categories in Home Settings : (nightly, aurora, beta) Panels (displays all home panels) (nightly) Content settings (Show site Suggestions, Automatic updates) Device: Alcatel One Touch OS: Android 4.1.2 Firefox 33.0a1, 32.0a2, 31.0b4