Replace Prompt.PromptListItem with GeckoMenuItems

NEW
Unassigned

Status

()

Firefox for Android
General
4 years ago
4 years ago

People

(Reporter: wesj, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Android prompts current use (mostly) native views for lists/context menus. To back them we have a PromptListItem inner class/struct that decodes and holds data.

Most of that class is duplicated in GeckoMenuItem (along with some other nice abilities like support for icons, ActionMode buttons, or submenus for instance). We should reuse the GeckoMenuItem code in our Prompt code if possible. That will result in some theme changes for our context menus, but should bring them more in line with our normal menus (and we wont have to update both places for changes any more).
(Reporter)

Comment 1

4 years ago
Created attachment 8339944 [details] [diff] [review]
WIP Patch

Most of the way there. This needs cleanup, and icons for "radio" buttons.
(Reporter)

Comment 2

4 years ago
Created attachment 8342215 [details] [diff] [review]
Patch

This works well and I think is a pretty good first step towards quick-share in context menus. Since we're likely pushing that off until sriram returns, I figure he can review :)

I still need icons for "radio" type buttons. Here's screenshots.

current: http://dl.dropboxusercontent.com/u/72157/currentSingleSelect.png
new: http://dl.dropboxusercontent.com/u/72157/select1.png
new multiselect: http://dl.dropboxusercontent.com/u/72157/select2.png
new context menus (affected but mostly unchanged?): http://dl.dropboxusercontent.com/u/72157/contextmenu.png
Attachment #8339944 - Attachment is obsolete: true
Attachment #8342215 - Flags: review?(sriram)
Flags: needinfo?(ibarlow)
Comment on attachment 8342215 [details] [diff] [review]
Patch

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

Overall looks good. Needs couple of changes.
I would like to remove the toArray() calls. The ArrayAdapter can do it. In all other places, it can be just Lists or ArrayLists.

::: mobile/android/base/ActivityHandlerHelper.java
@@ +167,5 @@
>  
>              addFilePickingActivities(context, items, "*/*", aIntents);
>          }
>  
> +        return items.toArray(new GeckoMenuItem[] {});

Why don't we return the ArrayList here?

::: mobile/android/base/BrowserApp.java
@@ +677,2 @@
>          Resources res = getResources();
> +        items[0] = new GeckoMenuItem(null, 0, 0, res.getString(R.string.contextmenu_edit_bookmark));

I think GeckoMenuItem takes a resId as fourth parameter.

::: mobile/android/base/menu/GeckoMenuItem.java
@@ +260,5 @@
>  
>      @Override
>      public MenuItem setCheckable(boolean checkable) {
>          mCheckable = checkable;
> +        return onChanged();

I would call it,

{
...
onItemChanged();
return this;
}

@@ +287,5 @@
>          mIconRes = iconRes;
> +        return onChanged();
> +    }
> +
> +    private GeckoMenuItem onChanged() {

This needn't return anything.

::: mobile/android/base/menu/MenuItemDefault.java
@@ +96,5 @@
>          setChecked(item.isChecked());
>          setSubMenuIndicator(item.hasSubMenu());
>      }
>  
> +    /* Checkable */

This comment is not needed.

@@ +151,5 @@
> +            refreshDrawableState();
> +        }
> +    }
> +
> +    /* Checkable */

This comment is not needed.

@@ +160,5 @@
>              refreshDrawableState();
>          }
>      }
>  
> +    /* Checkable */

This comment is not needed.

::: mobile/android/base/prompts/Prompt.java
@@ +175,2 @@
>                  JSONArray selected = new JSONArray();
> +                ListView list = mListView != null ? mListView : mDialog.getListView();

(mListView != null) ? .. : ..

@@ +175,3 @@
>                  JSONArray selected = new JSONArray();
> +                ListView list = mListView != null ? mListView : mDialog.getListView();
> +                for (int i = 0; i < mListView.getCount(); i++) {

Cache getCount() in a variable.

@@ +264,5 @@
>       */
> +    private void addMultiSelectList(AlertDialog.Builder builder, GeckoMenuItem[] listItems) {
> +        PromptListAdapter adapter = new PromptListAdapter(mContext, listItems);
> +        mListView = (ListView) mInflater.inflate(R.layout.select_dialog_list, null);
> +

Newline not needed.

@@ +275,2 @@
>  
> +        for (int i = 0; i < mListView.getCount(); i++) {

Cache this in a variable.

@@ +304,5 @@
> +            builder.setSingleChoiceItems(adapter, selectedIndex, this);
> +        } else {
> +        */
> +            builder.setAdapter(adapter, this);
> +        // }

If all these aren't needed. Please remove them.

@@ +363,5 @@
>       */
>      @Override
>      public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
>          ThreadUtils.assertOnUiThread();
> +        ListView list = mListView != null ? mListView : mDialog.getListView();

(mListView != null) ? ... : ...

@@ +367,5 @@
> +        ListView list = mListView != null ? mListView : mDialog.getListView();
> +        PromptListAdapter adapter = (PromptListAdapter) list.getAdapter();
> +        GeckoMenuItem item = adapter.getItem(position);
> +        if (item.isCheckable()) {
> +            item.setChecked( !item.isChecked() );

(!item.isChecked());
And since this is being used twice, I recommend caching this is in a variable.

@@ +441,5 @@
>          }
>  
> +        ArrayList<GeckoMenuItem> items = getListItemArray(geckoObject, "listitems");
> +        mMultiSelect = geckoObject.optBoolean("multiple");
> +        show(title, text, items.toArray(new GeckoMenuItem[] { }));

I like passing ArrayList here, instead of changing it to an array.

@@ +487,4 @@
>          for (int i = 0; i < length; i++) {
>              try {
> +                JSONObject obj = items.getJSONObject(i);
> +                final GeckoMenuItem item = new GeckoMenuItem(obj);

I don't know if we should move this to GeckoMenuItem. I like to have GeckoMenuItem abstract -- as in -- same as what a menu would be -- without Gecko logic in it. The decoding should happen here and should be assigned to a GeckoMenuItem instead.

@@ +508,5 @@
>      }
>  
> +    private Drawable getBlankDrawable() {
> +        if (mBlankDrawable == null) {
> +            Resources res = mContext.getResources();

No need for a local variable for Resources.

@@ +550,2 @@
>              if (convertView == null) {
> +                view = item.getView(parent.getContext());

GeckoMenuItem is abstract. Something that creates a "type of MenuItem" should take care of its appearance, and feed the values from the GeckoMenuItem. This is the logic used in the adapter inside GeckoMenu and other places.

So, instead of using "item.getView()" here. It should "create" a "type of MenuItem" here.

if (item.isHeader()) {
   view = new MenuItemHeader( ... );
} else {
   view = new MenuItemDefault( ... );
}

And this is good because.. you can initialize the view with the item later.

Your current code reads like,

view = item.getView();
view.initialize(item);

--> If the item knows the view it is going to be like, why not initialize itself?

Since that logic fails, it's better to take the getView() call out of GeckoMenuItem and use it where it needs to be inflated.

@@ +558,3 @@
>  
> +            if (item.isCheckable() && !mMultiSelect) {
> +                ((MenuItemDefault) view).setRadio(true);

Hardcoding. mmm. Me no likey. :(
Shall we make this better somehow?

::: mobile/android/modules/Prompt.jsm
@@ +171,5 @@
>      aItems.forEach(function(item) {
>        let obj = { id: item.id };
>  
>        obj.label = item.label;
> +      obj.title = item.label;

Why is this needed?

@@ +187,4 @@
>          }
> +        hasSelected = true;
> +
> +        obj.checkable = true; 

Whitespace.
Attachment #8342215 - Flags: review?(sriram)
Attachment #8342215 - Flags: review-
Attachment #8342215 - Flags: feedback+
(Reporter)

Updated

4 years ago
Duplicate of this bug: 918407

Updated

4 years ago
Flags: needinfo?(ibarlow)
You need to log in before you can comment on or make changes to this bug.