973013 - Pull PromptListItem and PromptListAdapater out of Prompt.java

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

We have some subclasses in Prompt.java to handle lists and row items. It would be nice to pull them out and decouple them from the class a bit.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - Pull out PromptListItem

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch 2 - Pull out PromptListAdapter

This patch is a little more key for the quick share in context menu stuff. i.e. AFAICT, we need to make a dialog with tabs in it. Those tabs show lists on context menu items, and those lists need access to this adapter.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch 2 - Pull out PromptListAdapter

Left some cruft in this on accident :(

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch 2 - Pull out PromptListAdapter

Sorry brian. Didn't realize this was coming out of the wrong queue.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - Pull out PromptListItem

Comment 6

[Brian Nicholson (:bnicholson)]

Comment on attachment 8376539 [details] [diff] [review]
Patch 2 - Pull out PromptListAdapter

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

r- for mSelected issue.

::: mobile/android/base/prompts/PromptListAdapter.java
@@ +26,5 @@
> +    private static final int VIEW_TYPE_COUNT = 2;
> +
> +    private static final String LOGTAG = "GeckoPromptListAdapter";
> +
> +    public ListView listView;

Nit: s/listView/mListView/

@@ +27,5 @@
> +
> +    private static final String LOGTAG = "GeckoPromptListAdapter";
> +
> +    public ListView listView;
> +    private int mResourceId = -1;

Nit: Use final for constant class members

@@ +121,5 @@
> +    private void maybeUpdateCheckedState(int position, PromptListItem item, ViewHolder viewHolder) {
> +        viewHolder.textView.setEnabled(!item.disabled && !item.isGroup);
> +        viewHolder.textView.setClickable(item.isGroup || item.disabled);
> +
> +        if (mSelected == null) {

Looks like a bug here: mSelected is always going to be null since it's no longer using the mSelected from the outer Prompt class.

Is mSelected something we can just pass to PromptListAdapter when constructing it? If not, things could ugly.

@@ +171,5 @@
> +
> +        return convertView;
> +    }
> +
> +    private class ViewHolder {

Nit: This class should be static since it doesn't need any references to its outer parent class.

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 8376540 [details] [diff] [review]
Patch 1/2 - Pull out PromptListItem

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

::: mobile/android/base/prompts/PromptListItem.java
@@ +15,5 @@
> +    public final boolean isGroup;
> +    public final boolean inGroup;
> +    public final boolean disabled;
> +    public final int id;
> +    public boolean isParent;

Any reason you didn't keep isParent final as well?

@@ +19,5 @@
> +    public boolean isParent;
> +
> +    public Drawable icon;
> +
> +    PromptListItem(JSONObject aObject) {

Is there any reason for PromptListItem to get tied up in JSON details? PromptListItem could be stripped down to a nice, clean, struct-like object. Instead of taking a JSONObject, how about having a constructor that accepts each of the fields it's setting?

@@ +28,5 @@
> +        id = aObject.optInt("id");
> +        isParent = aObject.optBoolean("isParent");
> +    }
> +
> +    public PromptListItem(String aLabel) {

...then this constructor could just call the one mentioned above with all of these default values.

@@ +37,5 @@
> +        id = 0;
> +        isParent = false;
> +    }
> +
> +    static PromptListItem[] getArray(JSONArray items) {

Why move this method into this class? It was private and contained in Prompt.java before; now PromptListItem exposes this weird util method that's only useful in a single specific case.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Move promptlistadapter

Grr. I originally moved all of mSelected into the adapter, but I lost that somewhere. This does the same thing.

I really want to kill off this mSelected variable entirely. Will file a separate bug for that.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8378069 [details] [diff] [review]
Patch 2/2 - Move promptlistadapter

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

r- for some cleanup questions/comments below -- mostly for getting rid of these nasty exposed listView/selected fields.

::: mobile/android/base/prompts/Prompt.java
@@ +254,3 @@
>       *        left and allow multiple selection. 
>      */
> +    private void addlistItems(AlertDialog.Builder builder, PromptListItem[] listItems, boolean multipleSelection, boolean[] selected) {

While here: s/addlist/addList/

@@ +457,2 @@
>          boolean multiple = geckoObject.optBoolean("multiple");
> +        show(title, text, menuitems, multiple, getBooleanArray(geckoObject, "selected"));

Instead of passing around a separate selected array with the same cardinality as the menuitems array, can PromptListItem just have a selected state? e.g.,

boolean selected = getBooleanArray(geckoObject, "selected");
for (int i = 0; i < selected.length; i++) {
    menuitems[i].setSelected(selected[i]);
}

Then you can remove the selected[] boolean array from PromptListAdapter, we can just read the selected state directly off of the PromptListItem, we can directly use ArrayAdapter#getCount() in addSingleSelectList, etc.

::: mobile/android/base/prompts/PromptListAdapter.java
@@ +26,5 @@
> +    private static final int VIEW_TYPE_COUNT = 2;
> +
> +    private static final String LOGTAG = "GeckoPromptListAdapter";
> +
> +    public ListView listView;

This isn't JS! :P General consensus among Java programmers (and the convention we've been following) is that fields should only be public if they are final.

Hopefully we can just get rid of this (see below), but if not: please use private, s/listView/mListView/, with a public setListView().

@@ +27,5 @@
> +
> +    private static final String LOGTAG = "GeckoPromptListAdapter";
> +
> +    public ListView listView;
> +    private int mResourceId = -1;

Remove '= -1', and make mResourceId final

@@ +38,5 @@
> +    private static int mMinRowSize;
> +    private static int mIconTextPadding;
> +    private static boolean mInitialized = false;
> +
> +    boolean[] selected;

Hopefully we can just get rid of this, but if not: please use private, s/selected/mSelected/, and have setSelectedItems()/getSelectedItems() methods.

@@ +133,5 @@
> +            // Apparently just using ct.setChecked(true) doesn't work, so this
> +            // is stolen from the android source code as a way to set the checked
> +            // state of these items
> +            if (listView != null) {
> +                listView.setItemChecked(position, selected[position]);

So the whole reason we're holding onto this listView here is to use this setItemChecked hack, right? maybeUpdateCheckedState() is called only from getView(). And getView() is given a parent ViewGroup, and that *is* the ListView. So can we drop the class-level listView altogether, and have maybeUpdateCheckedState accept the parent ListView as an argument?

@@ +135,5 @@
> +            // state of these items
> +            if (listView != null) {
> +                listView.setItemChecked(position, selected[position]);
> +            }
> +        } catch (Exception e) {

What are we catching here? Can you remove this or catch a more explicit Exception (ideally with logging)?

@@ +172,5 @@
> +
> +        return convertView;
> +    }
> +
> +    private class ViewHolder {

This class should be static since it doesn't need any references to its outer parent class.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Seems good. I left removing mSelected for bug 974286.

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 8378667 [details] [diff] [review]
Patch v3

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

::: mobile/android/base/prompts/Prompt.java
@@ +258,2 @@
>      */
> +    private void addlistItems(AlertDialog.Builder builder, PromptListItem[] listItems, boolean multipleSelection, boolean[] selected) {

s/addlist/addList/

::: mobile/android/base/prompts/PromptListAdapter.java
@@ +173,5 @@
> +
> +        return convertView;
> +    }
> +
> +    private class ViewHolder {

Nit: static class

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 8376540 [details] [diff] [review]
Patch 1/2 - Pull out PromptListItem

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

r- since I'd like to see another revision of this (see comment 7).

Comment 13

[Wesley Johnston (:wesj)]

> Any reason you didn't keep isParent final as well?
If this is a share action provider, I wanted to also automatically force isParent to true (overriding any value passed up here). We don't have to do that. It just felt annoying to force the API consumers to set it.

> Is there any reason for PromptListItem to get tied up in JSON details?
> PromptListItem could be stripped down to a nice, clean, struct-like object.
> Instead of taking a JSONObject, how about having a constructor that accepts
> each of the fields it's setting?

Maybe we can talk about this tomorrow on irc. This parsing grows a little more complex the more we add to menus. I liked PromptListItem knowing about itself rather than requiring someone else to know about it. i.e. this felt more contained to me.

> Why move this method into this class? It was private and contained in
> Prompt.java before; now PromptListItem exposes this weird util method that's
> only useful in a single specific case.

Same thing basically.

I sorta imagine classes that might be used in other places. For instance, if we ever move to using GeckoMenuItem here, we'd be sharing this code with the main menu code. I don't want to duplicate this parsing in two different classes (and have to update it each time menus are extended). But I can see both sides here.

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 8376540 [details] [diff] [review]
Patch 1/2 - Pull out PromptListItem

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

Still not a fan of spreading the JSON around, but we can address that when we discuss bug 976249.

::: mobile/android/base/prompts/PromptListItem.java
@@ +39,5 @@
> +    }
> +
> +    static PromptListItem[] getArray(JSONArray items) {
> +        if (items == null)
> +            return new PromptListItem[0];

Nit: braces

Comment 15

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/6c1982ae4400
https://hg.mozilla.org/integration/fx-team/rev/b92e36eba349

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b92e36eba349
https://hg.mozilla.org/mozilla-central/rev/6c1982ae4400