Splitting this functionality into its own separate bug from the general settings bug.

Morphing this to support basic reordering via options in the long-tap menu. For the v1, we want to support basic reordering. The follow-up "nice-to-have" bug for drag and drop is now bug 974983.

Chenxia, can you own this, or are you busy with other stuff?

I started the patches for this, and it seems a little awkward to clear the Map and reinsert everything, just so we can set the order of the HomeConfig panels. Can you suggest a better way? For the mConfigMap in HomeConfig, I'm wondering if we should change it from a Map to LinkedHashMap; since ordering matters, it shouldn't really be just a Map.

Comment on attachment 8389043 [details] [diff] [review] Part 1: Support reordering in HomeConfig Review of attachment 8389043 [details] [diff] [review]: ----------------------------------------------------------------- LinkedHashMap was good enough for an initial Editor implementation because we never moved elements around. We'll need to use an extra data structure to track panel positions in order to implement this properly. Here's a suggestion: 1. Add a separate mConfigOrder (ArrayLink<String>) Editor member that keeps the list of panel IDs in the expected order. 2. Update mConfigOrder whenever you install, remove, or move a panel. 3. Use HashMap instead of LinkedHashMap as we won't rely on the order of the Map anymore (and LinkedHashMap has a bigger memory footprint) 4. Update the Editor's iterator to return elements in the order defined in mConfigOrder. 5. Add a moveTo(String panelId, int index) which simply updates mConfigOrder.

Comment on attachment 8389241 [details] [diff] [review] Part 1: Explicitly handle PanelConfig ordering in HomeConfig.Editor Review of attachment 8389241 [details] [diff] [review]: ----------------------------------------------------------------- Yep, that's the idea :-) ::: mobile/android/base/home/HomeConfig.java @@ +843,5 @@ > + * > + * @param deepCopy true to make deep-copied objects > + * @return ordered List of PanelConfigs > + */ > + private List<PanelConfig> makeOrderedDeepCopy(boolean deepCopy) { 'makeOrderedCopy' is more accurate I think. @@ +1065,2 @@ > final State newConfigState = > + new State(mHomeConfig, orderedPanelConfigs); nit: No need to break the line anymore. @@ +1080,5 @@ > public Iterator<PanelConfig> iterator() { > ThreadUtils.assertOnThread(mOriginalThread); > > + final List<PanelConfig> orderedPanelConfigs = makeOrderedDeepCopy(false); > + return orderedPanelConfigs.iterator(); It would probably be more efficient to have a custom Iterator that follows the order defined in mConfigOrder but fetches items from mConfigMap. Something like: private class EditorIterator implements Iterator<PanelConfig>() { private final Iterator<String> mOrderIterator; public EditorIterator() { mOrderIterator = mConfigOrder.iterator(); } @Override public boolean hasNext() { return mOrderIterator.hasNext(); } @Override public PanelConfig next() { final String panelId = mOrderIterator.next(); return mConfigMap.get(panelId); } @Override public void remove() { throw new UnsupportedOperationException("Can't remove from an Editor iterator"); } }

This involves more back-and-forth with the parent Category than I'd really like, but this is a function of the limitations of the Preferences classes, and we'll ideally be taking that out when we add support of drag and drop.

Ian, any string or other suggestions? This is the screenshot for the bottom-most Panel item, so the "Move down" button is disabled.

Comment on attachment 8389039 [details] [diff] [review] Part 0: Remove unused imports Review of attachment 8389039 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

Comment on attachment 8389494 [details] [diff] [review] Part 1: Explicitly handle PanelConfig ordering in HomeConfig.Editor Review of attachment 8389494 [details] [diff] [review]: ----------------------------------------------------------------- Great. ::: mobile/android/base/home/HomeConfig.java @@ +1061,5 @@ > public State commit() { > ThreadUtils.assertOnThread(mOriginalThread); > > + final List<PanelConfig> orderedPanelConfigs = makeOrderedCopy(false); > + final State newConfigState = new State(mHomeConfig, orderedPanelConfigs); Call makeOrderedCopy directly here? final State newConfigState = new State(mHomeConfig, makeOrderedCopy(false));

Comment on attachment 8389495 [details] [diff] [review] Part 2: Support "moveTo" for HomeConfig.Editor Review of attachment 8389495 [details] [diff] [review]: ----------------------------------------------------------------- Yep.

(In reply to Chenxia Liu [:liuche] from comment #11) > Created attachment 8389498 [details] > Screenshot: Context menu with Up/Down buttons > > Ian, any string or other suggestions? This is the screenshot for the > bottom-most Panel item, so the "Move down" button is disabled. Wrong screenshot? :-)

Comment on attachment 8389498 [details] Screenshot: Context menu with Up/Down buttons omg, definitely the wrong screenshot...›

Addressed nit, carrying over r+.

Correct screenshot.

This is the first level of the nested "Change order" dialog.

Second dialog.

Ian, how does that look? The ordering is now a nested dialog. A few screenshots, and also an apk if you want to try it: http://people.mozilla.org/~liuche/bug-959917/

This is the alternative that splits out the re-ordering to another context menu.

I have to say that based on the screenshots this interaction feels a little clumsy to me... (i couldn't find a build at that link, Chenxia) Now I know that drag and drop is challenging to implement in Settings. Would it be easier to implement as a separate mode that users can enter? Take a look at this quick sketch I did and let me know if this is crazy: http://cl.ly/image/1G0L0d150I3u

Reference for a single-screen context menu: http://cl.ly/image/0x0y2c3b0q3R Also, we agreed that having some kind of visual feedback after reordering is necessary. (Currently looking into adding visual feedback; simulating a click without opening the context menu was my original approach, but the Preferences API is pretty restrictive and so that seems to involve sending touch events to the parent ViewGroup and clearing/resetting the clickListener for the Preference, which is kind of gross.)

This goes on top of patch 3a.

Ian: two more builds for you to take a look at, with visual feedback. Fading in: http://people.mozilla.org/~liuche/bug-959917/reorder-fade.apk Vertical slide: http://people.mozilla.org/~liuche/bug-959917/reorder-slide.apk Any thoughts? Duration, direction of animation, etc.

(In reply to Chenxia Liu [:liuche] from comment #29) > Ian: two more builds for you to take a look at, with visual feedback. > > Fading in: http://people.mozilla.org/~liuche/bug-959917/reorder-fade.apk > Vertical slide: > http://people.mozilla.org/~liuche/bug-959917/reorder-slide.apk > > Any thoughts? Duration, direction of animation, etc. Thanks Chenxia. I think the fade one is the way to go here. Highlighting the row would have been preferable, but if fading the text is as much as we can get for now, that's a reasonable start.

Comment on attachment 8390181 [details] [diff] [review] Patch: Part 3a [Nested] alternative: Add reordering to nested context menu Review of attachment 8390181 [details] [diff] [review]: ----------------------------------------------------------------- This looks good overall but I do wonder if you could optimize the code for getting the pref's index by storing it in the PanelsPreference instance or something. You refresh the whole list every time we move a panel anyway. ::: mobile/android/base/preferences/PanelsPreference.java @@ +69,5 @@ > } > > @Override > protected String[] createDialogItems() { > + Resources res = getContext().getResources(); nit: final @@ +70,5 @@ > > @Override > protected String[] createDialogItems() { > + Resources res = getContext().getResources(); > + final String labelReorder = res.getString(R.string.pref_panels_reorder); What is the reorder string defined differently than LABEL_REMOVE, LABEL_SET_AS_DEFAULT, etc? @@ +140,5 @@ > > + private Dialog makeReorderDialog() { > + final AlertDialog.Builder builder = new AlertDialog.Builder(getContext()); > + > + Resources res = getContext().getResources(); final @@ +179,5 @@ > + * @param dialog Dialog to configure > + */ > + private void setReorderButtonsEnabled(DialogInterface dialog) { > + // Handle buttons for reordering. > + final int positionState = ((PanelsPreferenceCategory) mParentCategory).getPositionState(PanelsPreference.this); Just 'this' is enough here. @@ +190,5 @@ > + > + case STATE_IS_LAST: > + final TextView downButton = (TextView) ((AlertDialog) dialog).getListView().getChildAt(INDEX_MOVE_DOWN_BUTTON); > + downButton.setEnabled(false); > + downButton.setOnClickListener(null); What is this null click listener about?

Comment on attachment 8391091 [details] [diff] [review] Part 4a: Visual feedback for reorder [Fade-in] Review of attachment 8391091 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/preferences/PanelsPreference.java @@ +71,5 @@ > final ViewGroup group = (ViewGroup) view; > for (int i = 0; i < group.getChildCount(); i++) { > group.getChildAt(i).setEnabled(!mIsHidden); > } > + preferenceView = group; Where is preferenceView declared? @@ +75,5 @@ > + preferenceView = group; > + } > + > + if (mAnimate) { > + Animation fadeAnimation = new AlphaAnimation(0, 1); Use our PropertyAnimator for consistency? @@ +76,5 @@ > + } > + > + if (mAnimate) { > + Animation fadeAnimation = new AlphaAnimation(0, 1); > + fadeAnimation.setDuration(400); Isn't 400 a bit too long? 300 is usually a good pick. Check with ibarlow. @@ +77,5 @@ > + > + if (mAnimate) { > + Animation fadeAnimation = new AlphaAnimation(0, 1); > + fadeAnimation.setDuration(400); > + preferenceView.startAnimation(fadeAnimation); Use PropertyAnimator for consistency? ::: mobile/android/base/preferences/PanelsPreferenceCategory.java @@ +95,5 @@ > // Create and add the pref. > + final String panelId = panelConfig.getId(); > + final boolean animate = TextUtils.equals(mAnimatePanel, panelId); > + > + final PanelsPreference pref = new PanelsPreference(getContext(), PanelsPreferenceCategory.this, isRemovable, animate); This could just be 'this' no? @@ +174,2 @@ > mConfigEditor.commit(); > + setAnimate(panelKey); Wouldn't it be simpler if you just added an argument to the refresh() method containing the panel ID to animate?

Comment on attachment 8390181 [details] [diff] [review] Patch: Part 3a [Nested] alternative: Add reordering to nested context menu Review of attachment 8390181 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/preferences/PanelsPreferenceCategory.java @@ +158,5 @@ > + final int panelIndex = indexOfPreference(pref) - PANEL_PREFS_OFFSET; > + if (panelIndex > 0) { > + mConfigEditor.moveTo(pref.getKey(), panelIndex - 1); > + mConfigEditor.commit(); > + refresh(); This @@ +167,5 @@ > + final int panelIndex = indexOfPreference(pref) - PANEL_PREFS_OFFSET; > + if (panelIndex < getPreferenceCount() - 1 - PANEL_PREFS_OFFSET) { > + mConfigEditor.moveTo(pref.getKey(), panelIndex + 1); > + mConfigEditor.commit(); > + refresh(); Forgot to mention in my initial review: this is a bit harsh: you're re-reading the config, re-parsing the JSON string, re-creating all panel config instances, etc every time you move a panel up or down. However, the commit() call above (which should actually be an apply() call btw) returns the new HomeConfig.State instance representing the latest state which you can just pass to displayHomeConfig() directly.

(In reply to Lucas Rocha (:lucasr) from comment #31) > Comment on attachment 8390181 [details] [diff] [review] > Patch: Part 3a [Nested] alternative: Add reordering to nested context menu > > Review of attachment 8390181 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good overall but I do wonder if you could optimize the code for > getting the pref's index by storing it in the PanelsPreference instance or > something. You refresh the whole list every time we move a panel anyway. > Good call, I set these after creating and adding the Preferences. > > What is the reorder string defined differently than LABEL_REMOVE, > LABEL_SET_AS_DEFAULT, etc? The others are either used in a other classes, or may be called more than once within this one, so we store them statically; labelReorder is only ever used once, so we store it in a final local var. > > @@ +190,5 @@ > > + > > + case STATE_IS_LAST: > > + final TextView downButton = (TextView) ((AlertDialog) dialog).getListView().getChildAt(INDEX_MOVE_DOWN_BUTTON); > > + downButton.setEnabled(false); > > + downButton.setOnClickListener(null); > > What is this null click listener about? Since this is a TextView (there's now way to get a Button), calling setEnabled only greys out the text, but will still handle the clicks. Setting the clickListener is necessary to avoid handling these clicks. I also added a comment, so there isn't further confusion. > @@ +167,5 @@ > > + final int panelIndex = indexOfPreference(pref) - PANEL_PREFS_OFFSET; > > + if (panelIndex < getPreferenceCount() - 1 - PANEL_PREFS_OFFSET) { > > + mConfigEditor.moveTo(pref.getKey(), panelIndex + 1); > > + mConfigEditor.commit(); > > + refresh(); > > Forgot to mention in my initial review: this is a bit harsh: you're > re-reading the config, re-parsing the JSON string, re-creating all panel > config instances, etc every time you move a panel up or down. However, the > commit() call above (which should actually be an apply() call btw) returns > the new HomeConfig.State instance representing the latest state which you > can just pass to displayHomeConfig() directly. Handled - I didn't realize that was possible, thanks for pointing it out!

Ian, can you take a look at the apk [1] and let me know how you feel about the duration of the animation? [1] http://people.mozilla.org/~liuche/bug-959917/reorder-fade-2.apk

Can you revert back to the timing you were using in comment 29? This one feels a little too fast to be clear.

Comment on attachment 8394635 [details] [diff] [review] Patch: Part 3a: Add reordering to nested context menu Review of attachment 8394635 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/preferences/GeckoPreferences.java @@ +280,5 @@ > case HomePanelPicker.REQUEST_CODE_ADD_PANEL: > switch (resultCode) { > case Activity.RESULT_OK: > // Panel installed, refresh panels list. > + mPanelsPreferenceCategory.refresh(null); nit: add a version of refresh() that doesn't take any arguments and calls refresh(null) internally. ::: mobile/android/base/preferences/PanelsPreference.java @@ +158,5 @@ > + case INDEX_MOVE_UP_BUTTON: > + ((PanelsPreferenceCategory) mParentCategory).moveUp(PanelsPreference.this); > + break; > + > + case INDEX_MOVE_DOWN_BUTTON: nit: remove empty line here. ::: mobile/android/base/preferences/PanelsPreferenceCategory.java @@ +113,5 @@ > } > > + // Pass in position state to first and last preference. > + private void setPositionState() { > + final int prefCount = getPreferenceCount(); nit: move this down just before it's used in this method. @@ +176,5 @@ > + public void moveUp(PanelsPreference pref) { > + final int panelIndex = indexOfPreference(pref) - PANEL_PREFS_OFFSET; > + if (panelIndex > 0) { > + mConfigEditor.moveTo(pref.getKey(), panelIndex - 1); > + final State state = mConfigEditor.commit(); You should not use commit(), use apply() instead. @@ +185,5 @@ > + public void moveDown(PanelsPreference pref) { > + final int panelIndex = indexOfPreference(pref) - PANEL_PREFS_OFFSET; > + if (panelIndex < getPreferenceCount() - 1 - PANEL_PREFS_OFFSET) { > + mConfigEditor.moveTo(pref.getKey(), panelIndex + 1); > + final State state = mConfigEditor.commit(); Ditto. @@ +190,5 @@ > + refresh(state); > + } > + } > + > + private int indexOfPreference(Preference pref) { I thought the idea was to assign to change PanelPreference to hold its position so that you don't need to traverse all preferences here.

Comment on attachment 8394637 [details] [diff] [review] Part 4a: Visual feedback for reorder [Fade-in] Review of attachment 8394637 [details] [diff] [review]: ----------------------------------------------------------------- Looks good with the suggested fixes. ::: mobile/android/base/preferences/PanelsPreference.java @@ +46,5 @@ > protected boolean mIsHidden = false; > private boolean mIsRemovable; > > + private boolean mAnimate; > + private static final int ANIMATION_DURATION = 300; // ms nit: rename ANIMATION_DURATION_MS for extra clarity. Then you can remove the inline comment. It seems ibarlow preferred 400ms right? @@ +80,5 @@ > + > + if (mAnimate) { > + final PropertyAnimator animator = new PropertyAnimator(ANIMATION_DURATION); > + animator.attach(preferenceView, Property.ALPHA, 1); > + preferenceView.setAlpha(0); nit: move this set call before creating the animator with an empty line after it. setAlpha() is only available on post-Honeycomb. Use ViewHelper.setAlpha(preferenceView, 0) instead.

Nits addressed. I had to add an mIndex field as well as keep the first/last position state because a PanelPreference doesn't know how many panels there are, so we can't just use index. I could lift out "first" and just check for 0-index, but then we're checking for position as well as state, so it's slightly less clean.

Nits addressed, carrying over r+.

Unrelated, but cleaning up an extraneous resource fetch that I noticed in Eclipse. TextEdit.setHint takes a resid as well as a CharSequence, and we weren't using the field anyways.

Comment on attachment 8396841 [details] [diff] [review] Part -1: Cleanup Review of attachment 8396841 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

Comment on attachment 8396836 [details] [diff] [review] Part 3: Add reordering to context menu Review of attachment 8396836 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: mobile/android/base/preferences/GeckoPreferences.java @@ +280,5 @@ > case HomePanelPicker.REQUEST_CODE_ADD_PANEL: > switch (resultCode) { > case Activity.RESULT_OK: > // Panel installed, refresh panels list. > + mPanelsPreferenceCategory.refresh(null); This can be simply refresh(), no? ::: mobile/android/base/preferences/PanelsPreference.java @@ +178,5 @@ > + > + return dialog; > + } > + > + public void setPositionFirst() { Maybe setIsFirst? @@ +182,5 @@ > + public void setPositionFirst() { > + mPositionState = STATE_IS_FIRST; > + } > + > + public void setPositionLast() { Maybe setIsLast?

https://hg.mozilla.org/integration/fx-team/rev/ca8aaf73def0 https://hg.mozilla.org/integration/fx-team/rev/f83ca1637dac https://hg.mozilla.org/integration/fx-team/rev/29be6a4c3ef2 https://hg.mozilla.org/integration/fx-team/rev/a07947c3ef25 https://hg.mozilla.org/integration/fx-team/rev/1696d15729e9 https://hg.mozilla.org/integration/fx-team/rev/f3ec467a340c

https://hg.mozilla.org/mozilla-central/rev/ca8aaf73def0 https://hg.mozilla.org/mozilla-central/rev/f83ca1637dac https://hg.mozilla.org/mozilla-central/rev/29be6a4c3ef2 https://hg.mozilla.org/mozilla-central/rev/a07947c3ef25 https://hg.mozilla.org/mozilla-central/rev/1696d15729e9 https://hg.mozilla.org/mozilla-central/rev/f3ec467a340c

Setting P2 hub bugs to block hub v2 EPIC bug (targeting fx31 release). Filter on epic-hub-bugs.

Moztrap: New suite: https://moztrap.mozilla.org/manage/cases/?filter-suite=703 New TCs: https://moztrap.mozilla.org/manage/case/13911 https://moztrap.mozilla.org/manage/case/13910