942878 - Add a new panel from Settings

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Ian Barlow (:ibarlow)]

Let users create new lists from the "Home page lists" Settings screen. 


Designs will be posted shortly.

Comment 1

[Ian Barlow (:ibarlow)]

Attachment: Settings mockups

The mockup on the left shows the initial "reordering" screen which is being built in bug 942231, and the one on the right shows the additional "add list" functionality.

Comment 2

[:Margaret Leibovic]

What should happen when the user hits this "Add a list" item? Should it open the same "Add List" dialog that I've seen in other mock-ups [1].

I'd like to see us create one piece of UI for adding a new list, and we can just open that from wherever we want (settings, banner, promo panel, etc.).

[1] https://bug942884.bugzilla.mozilla.org/attachment.cgi?id=8345278

Comment 3

[Deb Richardson [:dria] (plz NEEDINFO)]

I think allowing users to completely remove lists that are essential UI elements (for example: there is no other way to view bookmarks) may be giving them a footgun.

How about letting them remove lists they've added themselves, and only letting them hide/unhide lists that are core to the browser UI?

Comment 4

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #2)
> What should happen when the user hits this "Add a list" item? Should it open
> the same "Add List" dialog that I've seen in other mock-ups [1].
> 
> I'd like to see us create one piece of UI for adding a new list, and we can
> just open that from wherever we want (settings, banner, promo panel, etc.).
> 
> [1] https://bug942884.bugzilla.mozilla.org/attachment.cgi?id=8345278

That's what I was thinking too, yeah. I know in older mocks I had a picker for Settings and a different picker for the Home screen. But I think they can be the same, both for simplicity's and consistency's sake.


(In reply to :dria from comment #3)
> How about letting them remove lists they've added themselves, and only letting them
> hide/unhide lists that are core to the browser UI?

I am down with that

Comment 5

[:Margaret Leibovic]

We need bug 942875 before we can start working on this.

Comment 6

[:Margaret Leibovic]

We should be able to start working on this. For this bug, we'll need to add a new item to the home panels settings page, and have that item launch a dialog to add a new list.

We'll need to decide how to populate this list of suggested new panels. To start, I think we should just have it pull from the currently available panels in PanelManager. However, we'll have to figure out how we'll want to implement suggested panels, since if we go this route, we'd need to include those suggested panels as pre-bundled add-ons, which seems inefficient.

Comment 7

[Chenxia Liu [:liuche]]

Ian, can I get the plus icon for this? I'm currently using the icon for new tab, which is the right size but not the right color.

Comment 8

[Ian Barlow (:ibarlow)]

Here you go! http://cl.ly/2K2u0l2Q2L1z

Comment 9

[Chenxia Liu [:liuche]]

The patches for this will need to be built on top of bug 964375, which adds PanelInfo => PanelConfig code, as well as safe adding of panels and removing of panels.

Comment 10

[Chenxia Liu [:liuche]]

Attachment: Part 0: Cleanup

Kind of an optional patch.

Comment 11

[Chenxia Liu [:liuche]]

Attachment: Part 1: Support adding panels from HomeConfig

Comment 12

[Chenxia Liu [:liuche]]

Attachment: Part 2: Add picker

Comment 13

[Chenxia Liu [:liuche]]

Attachment: WIP Part 3: UI feedback for empty list of available panels

Comment 14

[Chenxia Liu [:liuche]]

Attachment: Part 3.5 - Fake panels for testing

The next part that needs to come is supporting the removal of new panels.

Comment 15

[Chenxia Liu [:liuche]]

Attachment: Part 2: Add picker

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 8370643 [details] [diff] [review]
Part 1: Support adding panels from HomeConfig

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

The HomeConfigInvalidator code has changed a bit. You'll have to rebase. The new code should make your patch a bit simpler.

::: mobile/android/base/home/HomeConfigInvalidator.java
@@ +85,5 @@
>  
>      /**
> +     * Add a new panel.
> +     */
> +    public void addPanel(PanelConfig panelConfig) {

I'd go with installPanel() instead of addPanel().

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 8370657 [details] [diff] [review]
Part 2: Add picker

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

Looking good overall but needs some important changes and cleanups.

::: mobile/android/base/home/HomeConfig.java
@@ +194,5 @@
>          public PanelConfig(PanelType type, String title, String id, EnumSet<Flags> flags) {
>              this(type, title, id, null, null, flags);
>          }
>  
> +        public PanelConfig(PanelType type, String title, String id, LayoutType layoutType, List<ViewConfig> views) {

Unrelated? Remove.

::: mobile/android/base/home/HomePanelPicker.java
@@ +37,5 @@
> +    public static final String INTENT_EXTRAS_PANEL_IDS = "panelIds";
> +    public static final int REQUEST_CODE_ADD_PANEL = 1;
> +
> +    private PanelManager mPanelManager;
> +    protected HomeConfig mHomeConfig;

Why not private?

@@ +39,5 @@
> +
> +    private PanelManager mPanelManager;
> +    protected HomeConfig mHomeConfig;
> +
> +    protected List<PanelInfo> mPanelInfos = new ArrayList<PanelInfo>();

Why not private? Actually, I feel like the list of panel infos should be held by the custom adapter, not the activity.

@@ +41,5 @@
> +    protected HomeConfig mHomeConfig;
> +
> +    protected List<PanelInfo> mPanelInfos = new ArrayList<PanelInfo>();
> +    private final List<PanelConfig> mPanelConfigs = new ArrayList<PanelConfig>();
> +    protected List<String> homePanelsIds;

mHomePanelIds

@@ +54,5 @@
> +        super.onCreate(savedInstance);
> +
> +        Bundle intentExtras = getIntent().getExtras();
> +        if (intentExtras != null) {
> +            String[] panelIdsArray = intentExtras.getStringArray(INTENT_EXTRAS_PANEL_IDS);

INTENT_EXTRAS_PANEL_IDS and homePanelsIds could probably have names that are more explicit about their intent.

@@ +61,5 @@
> +            }
> +        }
> +
> +        // TODO: Instantiate PanelManager somewhere else that won't die.
> +        // XXX Maybe should be fetched from GeckoAppShell?

Why so? AFAIK, it's fine to use PanelManager in self-contained cases like this. What would be the advantage of having a long-lived PanelManager instance?

@@ +84,5 @@
> +        mPanelManager.requestAvailablePanels(requestHandler);
> +    }
> +
> +    private RequestCallback makeRequestHandler() {
> +        return new RequestCallback() {

Isn't this callback always the same? Create it once and reuse it on every requestAvailablePanels() call.

@@ +90,5 @@
> +            public void onComplete(final List<PanelInfo> panelInfos) {
> +                // Fetch current home panels if neccessary.
> +                if (homePanelsIds == null) {
> +                    homePanelsIds = new ArrayList<String>();
> +                    mLoadTask = new UiAsyncTask<Void, Void, List<PanelConfig>>(ThreadUtils.getBackgroundHandler()) {

Given that this is an activity, you can simply use HomeConfigLoader here. It will look cleaner and it's generally safer than using UIAsyncTasks.

@@ +125,5 @@
> +            if (!homePanelsIds.contains(panelInfo.getId())) {
> +                mPanelInfos.add(panelInfo);
> +                Map<String, String> map = new HashMap<String, String>();
> +                map.put("title", panelInfo.getTitle());
> +                adapterList.add(map);

If you implement a custom adapter, you can just use PanelInfo instances directly instead of creating these ad-hoc data structures.

@@ +129,5 @@
> +                adapterList.add(map);
> +            }
> +        }
> +
> +        SimpleAdapter adapter = new SimpleAdapter(HomePanelPicker.this, adapterList, R.layout.home_panel_picker_row, ADAPTER_FROM, ADAPTER_TO);

You'll probably need to go beyond the basic adapter in the mid-term anyway (for loading icons and all). I'd implement a simple custom adapter that does the proper view recycling bits (viewholder in getview, etc) from day 1. Using these stock adapters forces you to create these weird data structures to comply with the API.

@@ +130,5 @@
> +            }
> +        }
> +
> +        SimpleAdapter adapter = new SimpleAdapter(HomePanelPicker.this, adapterList, R.layout.home_panel_picker_row, ADAPTER_FROM, ADAPTER_TO);
> +        HomePanelPicker.this.setListAdapter(adapter);

Why is HomePanelPicker.this necessary?

@@ +133,5 @@
> +        SimpleAdapter adapter = new SimpleAdapter(HomePanelPicker.this, adapterList, R.layout.home_panel_picker_row, ADAPTER_FROM, ADAPTER_TO);
> +        HomePanelPicker.this.setListAdapter(adapter);
> +    }
> +
> +    private void addNewPanelAndQuit(PanelInfo panelInfo) {

addNewPanelAndQuit -> installPanelAndQuit

::: mobile/android/base/home/PanelManager.java
@@ +29,5 @@
>  
>  public class PanelManager implements GeckoEventListener {
>      private static final String LOGTAG = "GeckoPanelManager";
>  
> +    public static class PanelInfo {

Unrelated, I'd move this to another patch.

Comment 18

[Chenxia Liu [:liuche]]

Attachment: Part 1: Support adding panels from HomeConfig v2

Comment 19

[Chenxia Liu [:liuche]]

Attachment: Part 2 WIP: Add picker

This addresses most of the points from comment #19, except for using HomeConfigLoader instead of AsyncUITasks. In order to do that, this needs to be a Fragment within a FragmentActivity.

If someone wants to pick this up while I'm away, the additional things that need to be done are
- Part 3: Toast feedback if there are no new/available panels to add
- Support "Remove" as a PanelsPreference option

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 8371721 [details] [diff] [review]
Part 1: Support adding panels from HomeConfig v2

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

Yep.

Comment 21

[Lucas Rocha (:lucasr)]

Assigning to sola as liuche is on PTO this week.

Comment 22

[Sola Ogunsakin [:sola]]

Attachment: Part 3: UI feedback for empty list of available panels

Shows a plain view in the picker when no panels are available. Will file bug to clean it up.

Comment 23

[Sola Ogunsakin [:sola]]

Attachment: Part 4: Support "Remove" as a PanelsPreference option

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 8375723 [details] [diff] [review]
Part 3: UI feedback for empty list of available panels

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

Wondering: before building stuff on top of the existing patch, maybe I should review the WIP one first? Have you made any changes to the original patch?

::: mobile/android/base/resources/layout/no_available_panels_view.xml
@@ +1,1 @@
> +<?xml version="1.0" encoding="utf-8"?>

Maybe just use the existing home_empty_panel layout?

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 8375873 [details] [diff] [review]
Part 4: Support "Remove" as a PanelsPreference option

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

Nice, but needs some cleanups. Maybe this patch should be moved to another bug? Seems out of scope.

::: mobile/android/base/home/HomeConfig.java
@@ +268,5 @@
>              return mFlags.contains(Flags.DEFAULT_PANEL);
>          }
>  
> +        public boolean isBuiltIn() {
> +            return BUILT_IN_PANELS.contains(getId());

No need to create a new method here as (built-in == !isDynamic)

::: mobile/android/base/preferences/CustomListPreference.java
@@ -70,5 @@
>          // Fetch these strings now, instead of every time we ever want to relabel a button.
>          LABEL_IS_DEFAULT = res.getString(R.string.pref_default);
>          LABEL_SET_AS_DEFAULT = res.getString(R.string.pref_dialog_set_default);
> -
> -        mDialogItems = getDialogStrings();

This seems unrelated to the core intent of this patch. Why did you have to do this?

::: mobile/android/base/preferences/PanelsPreferenceCategory.java
@@ +91,5 @@
>              if (panelConfig.isDisabled()) {
>                  pref.setHidden(true);
>              }
> +
> +            pref.setIsRemovable(!panelConfig.isBuiltIn());

Just use panelConfig.isDynamic() instead.

Comment 26

[Sola Ogunsakin [:sola]]

(In reply to Lucas Rocha (:lucasr) from comment #24)
> Comment on attachment 8375723 [details] [diff] [review]
> Part 3: UI feedback for empty list of available panels
> 
> Review of attachment 8375723 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wondering: before building stuff on top of the existing patch, maybe I
> should review the WIP one first? Have you made any changes to the original
> patch?

This is a completely separate patch. The WIP patch https://bug942878.bugzilla.mozilla.org/attachment.cgi?id=8370645 only had logic for showing a toast message, but we decided to show an empty view. 

> 
> Maybe just use the existing home_empty_panel layout?

the one with the firefox logo?

Comment 27

[Sola Ogunsakin [:sola]]

(In reply to Lucas Rocha (:lucasr) from comment #25)
> Comment on attachment 8375873 [details] [diff] [review]
> Part 4: Support "Remove" as a PanelsPreference option
> 
> Review of attachment 8375873 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems unrelated to the core intent of this patch. Why did you have to
> do this?

CustomListPreference was caching the dialog strings (mDialogItems) in its constructor. At that point mIsRemovable in PanelsPreference isn't set yet so it caches a stale version. Didn't want to add mIsRemovable to PanelsPreference's constructor (didn't seem concise).

> > +
> > +            pref.setIsRemovable(!panelConfig.isBuiltIn());
> 
> Just use panelConfig.isDynamic() instead.

true, alright will do

Comment 29

[Chenxia Liu [:liuche]]

Lucas, I think I'm going to pick this one up again, to finish the Fragment-ing of the WIP because that still needs to be done.

Comment 30

[Lucas Rocha (:lucasr)]

(In reply to Chenxia Liu [:liuche] from comment #19)
> Created attachment 8372985 [details] [diff] [review]
> Part 2 WIP: Add picker
> 
> This addresses most of the points from comment #19, except for using
> HomeConfigLoader instead of AsyncUITasks. In order to do that, this needs to
> be a Fragment within a FragmentActivity.

Unless I'm missing something, you don't need to have a fragment to use HomeConfigLoader. Just make HomePanelPicker a FragmentActivity and use getSupportLoaderManager() to trigger a HomeConfigLoader. Using ListActivity doesn't give us much anyway.

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 8372985 [details] [diff] [review]
Part 2 WIP: Add picker

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

Please coordinate with sola to make sure the empty view stuff he did aligns with whatever direction you take with this patch.

::: mobile/android/base/home/HomePanelPicker.java
@@ +72,5 @@
> +    private void ensureRequestCallback() {
> +        if (mPanelsRequestCallback == null) {
> +            mPanelsRequestCallback = new RequestCallback() {
> +                @Override
> +                public void onComplete(final List<PanelInfo> panelInfos) {

Just some quick feedback here. The many levels of indentation here is a sign that this should be broken down into smaller chunks.

Comment 32

[Chenxia Liu [:liuche]]

Attachment: Part 2: Add picker

The picker now uses LoaderManager instead of AsyncTaskLoaders.

One thing that this currently doesn't do is handle the refresh of the Panels settings page after adding a panel. I played around with this a little, and the problem is that after installing a new panel, forcing a reload of HomeConfig doesn't give the most recent configuration, possibly because HomeConfigInvalidator hasn't updated the values by the time we close the picker dialog.

We could do a hack where the picker tells Settings to add the new Home panel (just by giving it the title), but that seems hacky.

Not sure if I need to do thread-safety here - this should only be accessed through the UI by user input.

Comment 33

[Lucas Rocha (:lucasr)]

Comment on attachment 8381114 [details] [diff] [review]
Part 2: Add picker

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

Looks good overall. I'd like to see some simplification around how the loading path is implemented. It feels a bit hard to follow right now.

::: mobile/android/base/home/HomePanelPicker.java
@@ +35,5 @@
> + */
> +public class HomePanelPicker extends FragmentActivity {
> +    private static final String LOGTAG = "HomePanelPicker";
> +
> +    public static final String INTENT_EXTRAS_CURRENT_PANEL_IDS = "currentPanelIds";

CURRENT_PANELS_IDS?

@@ +57,5 @@
> +                mCurrentPanelsIds = Arrays.asList(panelIdsArray);
> +            }
> +        }
> +
> +        setContentView(R.layout.home_panel_picker);

Move setContentView() up, just after the super.onCreate() call.

@@ +74,5 @@
> +    private void ensureRequestCallback() {
> +        if (mPanelsRequestCallback == null) {
> +            mPanelsRequestCallback = new RequestCallback() {
> +                @Override
> +                public void onComplete(final List<PanelInfo> panelInfos) {

The RequestCallback code should just forward to a private method that does the real job. This will make the code a bit easier to follow. I suggest something like:

private void requestAvailablePanels() {
    final PanelManager panelManager = new PanelManager();
    panelManager.requestAvailablePanels(new RequestCallback() {
        @Override
        public void onComplete(List<PanelInfo> panelInfos) {
            updatePanelInfos()
        }
    });
}

@@ +90,5 @@
> +        }
> +    }
> +
> +    private void loadConfig() {
> +        mCurrentPanelsIds = new ArrayList<String>();

You just create that in ConfigLoaderCallbacks.onLoaderFinished(), no?

@@ +96,5 @@
> +        LoaderManager lm = HomePanelPicker.this.getSupportLoaderManager();
> +        lm.initLoader(LOADER_ID_CONFIG, null, loaderCallbacks);
> +    }
> +
> +    protected void setPanelsListAdapter(List<PanelInfo> panelInfos) {

You can probably create the adapter unconditionally in the activity's onCreate() and add a method to the adapter to update it with a new list of panelinfos. Something like updateFromPanelInfos()

@@ +111,5 @@
> +        mListView.setAdapter(adapter);
> +    }
> +
> +    private void installNewPanelAndQuit(PanelInfo panelInfo) {
> +        PanelConfig newPanelConfig = panelInfo.toPanelConfig();

final

@@ +112,5 @@
> +    }
> +
> +    private void installNewPanelAndQuit(PanelInfo panelInfo) {
> +        PanelConfig newPanelConfig = panelInfo.toPanelConfig();
> +        HomeConfigInvalidator.getInstance().installPanel(newPanelConfig);

nit: add empty line here.

@@ +118,5 @@
> +        finish();
> +    }
> +
> +    // ViewHolder for rows of the panel picker.
> +    private class PanelRow {

static class

@@ +119,5 @@
> +    }
> +
> +    // ViewHolder for rows of the panel picker.
> +    private class PanelRow {
> +        TextView title;

final

@@ +124,5 @@
> +        int position;
> +        View view;
> +
> +        public PanelRow(View view) {
> +            this.view = view;

No need to hold a reference to the parent view. Just set the click listener in the convertView in the Adapter's getView().

@@ +129,5 @@
> +            title = (TextView) view.findViewById(R.id.title);
> +        }
> +    }
> +
> +    private class PanelsAdapter extends BaseAdapter {

static class?

@@ +130,5 @@
> +        }
> +    }
> +
> +    private class PanelsAdapter extends BaseAdapter {
> +        private LayoutInflater mInflater;

final

@@ +132,5 @@
> +
> +    private class PanelsAdapter extends BaseAdapter {
> +        private LayoutInflater mInflater;
> +        private final List<PanelInfo> mPanelInfos;
> +        private View.OnClickListener mOnClickListener;

final

@@ +136,5 @@
> +        private View.OnClickListener mOnClickListener;
> +
> +        public PanelsAdapter(Context context, List<PanelInfo> panelInfos) {
> +            mInflater = LayoutInflater.from(context);
> +            mPanelInfos = panelInfos;

nit: add empty line here,

@@ +163,5 @@
> +        }
> +
> +        @Override
> +        public View getView(int position, View convertView, ViewGroup parent) {
> +            PanelRow row;

final

@@ +168,5 @@
> +
> +            if (convertView == null) {
> +                convertView = mInflater.inflate(R.layout.home_panel_picker_row, null);
> +                row = new PanelRow(convertView);
> +                row.view.setOnClickListener(mOnClickListener);

Just the click listener directly in convertView.

@@ +184,5 @@
> +
> +    private class ConfigLoaderCallbacks implements LoaderCallbacks<List<PanelConfig>> {
> +        @Override
> +        public Loader<List<PanelConfig>> onCreateLoader(int id, Bundle args) {
> +            HomeConfig homeConfig = HomeConfig.getDefault(HomePanelPicker.this);

final

@@ +192,5 @@
> +        @Override
> +        public void onLoadFinished(Loader<List<PanelConfig>> loader, List<PanelConfig> panelConfigs) {
> +            for (PanelConfig panelConfig : panelConfigs) {
> +                mCurrentPanelsIds.add(panelConfig.getId());
> +            }

nit: add empty line here.

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +273,5 @@
> +                  // Pass this result up to the parent activity.
> +                  setResult(RESULT_CODE_EXIT_SETTINGS);
> +                  finish();
> +              }
> +              break;

nit: add empty line here.

@@ +275,5 @@
> +                  finish();
> +              }
> +              break;
> +          case HomePanelPicker.REQUEST_CODE_ADD_PANEL:
> +              if (resultCode == Activity.RESULT_OK) {

Use switch instead?

@@ +278,5 @@
> +          case HomePanelPicker.REQUEST_CODE_ADD_PANEL:
> +              if (resultCode == Activity.RESULT_OK) {
> +                  setResult(RESULT_CODE_EXIT_SETTINGS);
> +                  // TODO: Either open about:home to the new panel, or refresh the panels settings
> +                  // and display a toast.

File a follow-up for this.

@@ +415,5 @@
> +                } else if (PREFS_HOME_ADD_PANEL.equals(key)) {
> +                    pref.setOnPreferenceClickListener(new OnPreferenceClickListener() {
> +                        @Override
> +                        public boolean onPreferenceClick(Preference preference) {
> +                            // TODO: Use a real callback for updating.

What's this 'real callback' about?

::: mobile/android/base/preferences/PanelsPreferenceCategory.java
@@ +228,5 @@
>       */
>      @Override
>      protected void setFallbackDefault() {
> +        // First preference (index 0) is Preference to add panels.
> +        for (int i = 1; i < getPreferenceCount(); i++) {

Two things to discuss with ibarlow:
- Maybe the 'Add panel' item should have different style so that it stands out from the panel items in the list?
- Maybe having an 'Add panel' action directly in the action bar would be a better spot?

::: mobile/android/base/resources/layout/home_panel_picker_row.xml
@@ +6,5 @@
> +<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
> +              android:layout_width="match_parent"
> +              android:layout_height="match_parent"
> +              android:minHeight="?android:attr/listPreferredItemHeight"
> +              android:orientation="horizontal">

File a follow-up to display an icon for each row both in the picker and the 'Home' settings UI.

Comment 34

[Chenxia Liu [:liuche]]

Attachment: Part 2: Add picker v2

Comment 35

[Chenxia Liu [:liuche]]

Addressed nit comments, and cleaned up install flow a bit better.
> 
> @@ +96,5 @@
> > +        LoaderManager lm = HomePanelPicker.this.getSupportLoaderManager();
> > +        lm.initLoader(LOADER_ID_CONFIG, null, loaderCallbacks);
> > +    }
> > +
> > +    protected void setPanelsListAdapter(List<PanelInfo> panelInfos) {
> 
> You can probably create the adapter unconditionally in the activity's
> onCreate() and add a method to the adapter to update it with a new list of
> panelinfos. Something like updateFromPanelInfos()

Done. I originally wanted to set the adapter once so any empty text we show isn't flashed when the (empty) adapter is set the first time - I ran into some problems with that for the empty about:home screens, but we can take the same approach here.

> @@ +129,5 @@
> > +            title = (TextView) view.findViewById(R.id.title);
> > +        }
> > +    }
> > +
> > +    private class PanelsAdapter extends BaseAdapter {
> 
> static class?
> 

PanelsAdapter needs to use some methods that aren't declared static - should I make them static? Is the reason to use private static classes for performance, or is there another reason?

> 
> @@ +278,5 @@
> > +          case HomePanelPicker.REQUEST_CODE_ADD_PANEL:
> > +              if (resultCode == Activity.RESULT_OK) {
> > +                  setResult(RESULT_CODE_EXIT_SETTINGS);
> > +                  // TODO: Either open about:home to the new panel, or refresh the panels settings
> > +                  // and display a toast.
> 
> File a follow-up for this.
> 

Done, bug 976925.

> @@ +415,5 @@
> > +                } else if (PREFS_HOME_ADD_PANEL.equals(key)) {
> > +                    pref.setOnPreferenceClickListener(new OnPreferenceClickListener() {
> > +                        @Override
> > +                        public boolean onPreferenceClick(Preference preference) {
> > +                            // TODO: Use a real callback for updating.
> 
> What's this 'real callback' about?
> 

Just an old comment, I ended up using onActivityResult in GeckoPreferences to handle the flow after the dialog is closed.

> ::: mobile/android/base/preferences/PanelsPreferenceCategory.java
> @@ +228,5 @@
> >       */
> >      @Override
> >      protected void setFallbackDefault() {
> > +        // First preference (index 0) is Preference to add panels.
> > +        for (int i = 1; i < getPreferenceCount(); i++) {
> 
> Two things to discuss with ibarlow:
> - Maybe the 'Add panel' item should have different style so that it stands
> out from the panel items in the list?

I'll ask about this.

> - Maybe having an 'Add panel' action directly in the action bar would be a
> better spot?

I think inside the Panels category is not a bad spot for it - once we add the "update over wifi only" above the Panels category, it'll look more like Search (and less weird).

> 
> ::: mobile/android/base/resources/layout/home_panel_picker_row.xml
> @@ +6,5 @@
> > +<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
> > +              android:layout_width="match_parent"
> > +              android:layout_height="match_parent"
> > +              android:minHeight="?android:attr/listPreferredItemHeight"
> > +              android:orientation="horizontal">
> 
> File a follow-up to display an icon for each row both in the picker and the
> 'Home' settings UI.

Done, bug 976983.

Comment 36

[Lucas Rocha (:lucasr)]

Comment on attachment 8381902 [details] [diff] [review]
Part 2: Add picker v2

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

This is looking great. Just needs some final tweaks.

::: mobile/android/base/home/HomePanelPicker.java
@@ +36,5 @@
> +public class HomePanelPicker extends FragmentActivity {
> +    private static final String LOGTAG = "HomePanelPicker";
> +
> +    // Intent extra key for current panel ids.
> +    public static final String CURRENT_PANEL_IDS = "currentPanelIds";

CURRENT_PANELS_IDS for consistency with mCurrentPanelsIds?

@@ +63,5 @@
> +
> +        mListView = (ListView) findViewById(R.id.list);
> +
> +        // Set an empty adapter.
> +        final PanelsAdapter adapter = new PanelsAdapter(this, new ArrayList<PanelInfo>());

No need to pass an empty list that will be discarded anyway. Just handle null mPanelInfos in PanelsAdapter.

@@ +75,5 @@
> +        final PanelManager panelManager = new PanelManager();
> +        panelManager.requestAvailablePanels(new RequestCallback() {
> +            @Override
> +            public void onComplete(final List<PanelInfo> panelInfos) {
> +                mPanelInfos = panelInfos;

nit: add empty line here.

@@ +81,5 @@
> +                if (mCurrentPanelsIds == null) {
> +                    // If adapter data changes, loadConfig will refresh the adapter.
> +                    loadConfig();
> +                } else {
> +                    updatePanelsListAdapter(mPanelInfos);

Maybe rename to updatePanelsAdapter()?

@@ +88,5 @@
> +        });
> +    }
> +
> +    private void loadConfig() {
> +        ConfigLoaderCallbacks loaderCallbacks = new ConfigLoaderCallbacks();

final

@@ +89,5 @@
> +    }
> +
> +    private void loadConfig() {
> +        ConfigLoaderCallbacks loaderCallbacks = new ConfigLoaderCallbacks();
> +        LoaderManager lm = HomePanelPicker.this.getSupportLoaderManager();

final

@@ +100,5 @@
> +     *
> +     * @param panelInfos List of all available panels.
> +     */
> +    protected void updatePanelsListAdapter(List<PanelInfo> panelInfos) {
> +        List<PanelInfo> availablePanelInfos = new ArrayList<PanelInfo>();

Maybe availablePanels?

@@ +131,5 @@
> +            title = (TextView) view.findViewById(R.id.title);
> +        }
> +    }
> +
> +    private class PanelsAdapter extends BaseAdapter {

Maybe PickerAdapter is a better name?

@@ +143,5 @@
> +
> +            mOnClickListener = new View.OnClickListener() {
> +                @Override
> +                public void onClick(View view) {
> +                    PanelRow panelsRow = (PanelRow) view.getTag();

final

@@ +151,5 @@
> +        }
> +
> +        @Override
> +        public int getCount() {
> +            return mPanelInfos.size();

if (mPanelInfos == null) {
    return 0;
}

return mPanelInfos.size();

@@ +155,5 @@
> +            return mPanelInfos.size();
> +        }
> +
> +        @Override
> +        public PanelInfo getItem(int position) {

if (mPanelInfos == null) {
    return null;
}

return mPanelInfos.get(position);

::: mobile/android/base/preferences/PanelsPreferenceCategory.java
@@ +227,5 @@
>       * if possible and set it as mDefaultReference.
>       */
>      @Override
>      protected void setFallbackDefault() {
> +        // First preference (index 0) is Preference to add panels.

final int prefsCount = getPreferenceCount();
for (int i = 1; i < prefsCount; i++) {
   ...
}

Comment 37

[Chenxia Liu [:liuche]]

Attachment: Part 2: Add picker v3

Comment 38

[Chenxia Liu [:liuche]]

Comment on attachment 8375723 [details] [diff] [review]
Part 3: UI feedback for empty list of available panels

Split off into bug 973039.

Comment 39

[Chenxia Liu [:liuche]]

Comment on attachment 8375873 [details] [diff] [review]
Part 4: Support "Remove" as a PanelsPreference option

Split off into bug 973039 per comment #25.

Comment 40

[Chenxia Liu [:liuche]]

Attachment: (Part 3) Fake panel for testing

Comment 41

[Lucas Rocha (:lucasr)]

Comment on attachment 8383457 [details] [diff] [review]
Part 2: Add picker v3

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

Awesome. Please show this to ibarlow to get some early feedback and file follow-up for the pending design tweaks. For instance, we should probably set a minimum height for the dialog to avoid showing a weird-looking dialog when it only has one row. Also, ping sola to rebase his patch for the dialog's empty state.

::: mobile/android/base/home/HomePanelPicker.java
@@ +11,5 @@
> +import android.support.v4.app.FragmentActivity;
> +import android.support.v4.app.LoaderManager;
> +import android.support.v4.app.LoaderManager.LoaderCallbacks;
> +import android.support.v4.content.Loader;
> +import android.util.Log;

Unused? Please audit this file for unused imports.

@@ +37,5 @@
> +    private static final String LOGTAG = "HomePanelPicker";
> +
> +    // Intent extra key for current panel ids.
> +    public static final String CURRENT_PANELS_IDS = "currentPanelsIds";
> +

Document this public constant.

@@ +38,5 @@
> +
> +    // Intent extra key for current panel ids.
> +    public static final String CURRENT_PANELS_IDS = "currentPanelsIds";
> +
> +    public static final int REQUEST_CODE_ADD_PANEL = 1;

nit: add empty line here.

@@ +44,5 @@
> +
> +    private ListView mListView;
> +    private List<String> mCurrentPanelsIds;
> +    private List<PanelInfo> mPanelInfos;
> +    private RequestCallback mPanelsRequestCallback;

mPanelsRequestCallback is not used anywhere.

@@ +63,5 @@
> +
> +        mListView = (ListView) findViewById(R.id.list);
> +
> +        // Set an empty adapter.
> +        final PickerAdapter adapter = new PickerAdapter(this, null);

Add a constructor that only takes a Context to make this a bit cleaner.

@@ +77,5 @@
> +            @Override
> +            public void onComplete(final List<PanelInfo> panelInfos) {
> +                mPanelInfos = panelInfos;
> +
> +                // Fetch current home panels if neccessary.

typo: necessary

@@ +79,5 @@
> +                mPanelInfos = panelInfos;
> +
> +                // Fetch current home panels if neccessary.
> +                if (mCurrentPanelsIds == null) {
> +                    // If adapter data changes, loadConfig will refresh the adapter.

Not sure I understand this comment. I guess you mean something like: "If the current panel ids have not been passed directly to the activity, load them here before updating the adapter."

@@ +100,5 @@
> +     * installed panels filtered out).
> +     *
> +     * @param panelInfos List of all available panels.
> +     */
> +    protected void updatePanelsAdapter(List<PanelInfo> panelInfos) {

private?

@@ +101,5 @@
> +     *
> +     * @param panelInfos List of all available panels.
> +     */
> +    protected void updatePanelsAdapter(List<PanelInfo> panelInfos) {
> +        List<PanelInfo> availablePanels = new ArrayList<PanelInfo>();

final

@@ +135,5 @@
> +
> +    private class PickerAdapter extends BaseAdapter {
> +        private final LayoutInflater mInflater;
> +        private List<PanelInfo> mPanelInfos;
> +        private final View.OnClickListener mOnClickListener;

nit: import OnClickListener to make this look a bit cleaner.

@@ +154,5 @@
> +        @Override
> +        public int getCount() {
> +            if (mPanelInfos == null) {
> +                return 0;
> +            }

nit: add empty line here.

@@ +162,5 @@
> +        @Override
> +        public PanelInfo getItem(int position) {
> +            if (mPanelInfos == null) {
> +                return null;
> +            }

nit: add empty line here.

@@ +177,5 @@
> +            PanelRow row;
> +
> +            if (convertView == null) {
> +                convertView = mInflater.inflate(R.layout.home_panel_picker_row, null);
> +                row = new PanelRow(convertView);

nit: reorder and split this for clarity:

convertView = ...;
convertView.setOnClickListener(...);

row = new PanelRow(...);
convertView.setTag(row);

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +274,5 @@
> +                  // Pass this result up to the parent activity.
> +                  setResult(RESULT_CODE_EXIT_SETTINGS);
> +                  finish();
> +              }
> +              break;

nit: add empty line here.

::: mobile/android/base/preferences/PanelsPreferenceCategory.java
@@ +228,5 @@
>       */
>      @Override
>      protected void setFallbackDefault() {
> +        // First preference (index 0) is Preference to add panels.
> +        int prefsCount = getPreferenceCount();

final

Comment 42

[Chenxia Liu [:liuche]]

Attachment: Part 2: Add picker v4

Addressed comments, carrying over r+.

Ian, this has the WIP patches for "no panels available" and "remove panel" built, and still needs post-install UI - let me know what you think about the add panels part: http://people.mozilla.org/~liuche/bug-942878/add-panels.apk

Comment 43

[Ian Barlow (:ibarlow)]

This looks good, and seems to work as intended. Couple bits of feedback / questions. 

* I believe we decided to only show "Remove" and not "Hide" in the context menus.
* Is this bug meant to capture the styling for the "list chooser" menu? Or is that happening somewhere else. I'd like to see it reflect the original designs more, but I don't know where we're doing that work. Or if we're just starting with stock context menus for this release, and refining later.
* Lastly, I believe we decided not to take users to the home panel after installing a new panel -- rather, we just show a confirmation toast. Check with Lucas on this, I believe he had a bug open for that.

Comment 44

[Lucas Rocha (:lucasr)]

(In reply to Ian Barlow (:ibarlow) from comment #43)
> This looks good, and seems to work as intended. Couple bits of feedback /
> questions. 
> 
> * I believe we decided to only show "Remove" and not "Hide" in the context
> menus.

This will be covered in bug 978991 I suppose.

> * Is this bug meant to capture the styling for the "list chooser" menu? Or
> is that happening somewhere else. I'd like to see it reflect the original
> designs more, but I don't know where we're doing that work. Or if we're just
> starting with stock context menus for this release, and refining later.

I think we should reduce the scope of this bug to be more like a panel picker 'MVP' i.e. get the core feature in place for testing. Ian, could you please a follow-up with a link to the original design and a list of needed changes?

> * Lastly, I believe we decided not to take users to the home panel after
> installing a new panel -- rather, we just show a confirmation toast. Check
> with Lucas on this, I believe he had a bug open for that.

liuche filed bug 976925 to track this.

Comment 45

[Chenxia Liu [:liuche]]

(In reply to Lucas Rocha (:lucasr) from comment #44)

> > * I believe we decided to only show "Remove" and not "Hide" in the context
> > menus.
> 
> This will be covered in bug 978991 I suppose.

That's true; Margaret and I discussed this a little yesterday though, and it seemed Hide and Remove might actually be different enough use cases, especially for panels that store data or use credentials. Perhaps we should reconsider, but see comments in bug 978991.

> 
> > * Is this bug meant to capture the styling for the "list chooser" menu? Or
> > is that happening somewhere else. I'd like to see it reflect the original
> > designs more, but I don't know where we're doing that work. Or if we're just
> > starting with stock context menus for this release, and refining later.
> 
> I think we should reduce the scope of this bug to be more like a panel
> picker 'MVP' i.e. get the core feature in place for testing. Ian, could you
> please a follow-up with a link to the original design and a list of needed
> changes?
> 
Filed a follow-up for styling and other open questions, bug 979439.

Comment 46

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/97b4197570c4
https://hg.mozilla.org/integration/fx-team/rev/1c6e9e9c13c3
https://hg.mozilla.org/integration/fx-team/rev/4d1775112e0e

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97b4197570c4
https://hg.mozilla.org/mozilla-central/rev/1c6e9e9c13c3
https://hg.mozilla.org/mozilla-central/rev/4d1775112e0e

Comment 48

[Aaron Train [:aaronmt]]

A 'Add Panel' setting landed and is in Nightly (03/07) but is non-functional. Is that intentional?

Comment 49

[Chenxia Liu [:liuche]]

Clicking on the "Add panel" setting currently opens a dialog - if you do not have any panels, you don't get the option of adding a new panel. Bug 987869 is open for showing promoted panels, even if you don't have any installed.

Are you seeing problems with the panel picker, or is this an old comment?

Comment 50

[Aaron Train [:aaronmt]]

This works fine now. If I remove registered panels from add-ons, I see them listed again in the 'add a new panel' dialog once they're loaded in after a second.

Comment 51

[:Margaret Leibovic]

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.