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

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

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

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.

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.