966022 - Implement the notion of itemType on existing panel views (list, grid)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

The itemType parameter will define the kind of views to inflate for each child in panel list and grid views.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Make PanelListRow and PanelGridItemView share an interface (r=margaret)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Introduce ItemType into HomeConfig/Home.jsm (r=margaret)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Use ItemType in PanelListView and PanelGridView (r=margaret)

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8368845 [details] [diff] [review]
Make PanelListRow and PanelGridItemView share an interface (r=margaret)

Prep work for the item type stuff. We need all item views to share an interface to update from a cursor.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8368846 [details] [diff] [review]
Introduce ItemType into HomeConfig/Home.jsm (r=margaret)

Not entirely happy with the 'LINK' name (to use our current TwoLineRow items) but couldn't come up with something better. Suggestions are welcome.

Also, not sure yet if we should make the IMAGE type item figure out if it can/should show a caption text based on the dataset or not. The alternative is to add an item type that is more explicit about it such as IMAGE_WITH_CAPTION or something.

Thoughts?

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8368847 [details] [diff] [review]
Use ItemType in PanelListView and PanelGridView (r=margaret)

Use the item types in our existing item views. For now I'm only accepting item types that makes sense for each type of view. I wonder if we should add some validation on the JS side too?

Comment 7

[:Margaret Leibovic]

Comment on attachment 8368845 [details] [diff] [review]
Make PanelListRow and PanelGridItemView share an interface (r=margaret)

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

::: mobile/android/base/home/PanelListRow.java
@@ +25,5 @@
>  import android.view.View;
>  
>  import java.lang.ref.WeakReference;
>  
> +public class PanelListRow extends TwoLineRow implements PanelViewItem {

TwoLineRow already has an abstract method named updateFromCursor:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/TwoLineRow.java#76

Maybe that's the class that should be implementing the interface?

Comment 8

[:Margaret Leibovic]

Comment on attachment 8368846 [details] [diff] [review]
Introduce ItemType into HomeConfig/Home.jsm (r=margaret)

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

::: mobile/android/base/home/HomeConfig.java
@@ +460,5 @@
>          };
>      }
>  
> +    public static enum ItemType implements Parcelable {
> +        LINK("link"),

Yeah, I don't really like LINK either, since the IMAGE items will also have a corresponding URL that they can open. Maybe TEXT, or TWO_LINE_TEXT, if we want to be really specific? For our own ease of understanding, it would be nice if these matched up well with the names of the classes we use to implement them.

::: mobile/android/modules/Home.jsm
@@ +221,5 @@
>          throw "Home.panels: Invalid view type: panel.id = " + panel.id + ", view.type = " + view.type;
>        }
>  
> +      if (!this._valueExists(this.Item, view.itemType)) {
> +        throw "Home.panels: Invalid item type: panel.id = " + panel.id + ", view.itemType = " + view.itemType;

Should we come up with a default item type? Maybe LIST views default to LINK item types, and GRID views default to IMAGE item types? I'm realizing this patch adds another dimension to our configuration matrix...

Comment 9

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #8)

> ::: mobile/android/modules/Home.jsm
> @@ +221,5 @@
> >          throw "Home.panels: Invalid view type: panel.id = " + panel.id + ", view.type = " + view.type;
> >        }
> >  
> > +      if (!this._valueExists(this.Item, view.itemType)) {
> > +        throw "Home.panels: Invalid item type: panel.id = " + panel.id + ", view.itemType = " + view.itemType;
> 
> Should we come up with a default item type? Maybe LIST views default to LINK
> item types, and GRID views default to IMAGE item types? I'm realizing this
> patch adds another dimension to our configuration matrix...

Actually, looking at the next patch in the series, we only support certain item types for certain view types, so maybe we should be validating that in the JS, rather than waiting until we're trying to load the view to throw an error.

Also, until we actually do support multiple item types, I do think it would be good to just default to the one supported item type if it's not specified.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8368847 [details] [diff] [review]
Use ItemType in PanelListView and PanelGridView (r=margaret)

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

This looks good, but may need to be updated if the first two patches change to address comments.

Comment 11

[:Margaret Leibovic]

Comment on attachment 8368845 [details] [diff] [review]
Make PanelListRow and PanelGridItemView share an interface (r=margaret)

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

I think it's confusing that TwoLineRow already has an abstract method named updateFromCursor, but this patch introduces a new interface with the same method.

I think we should either make TwoLineRow the thing that implements the interface, or get rid of the abstract method on TwoLineRow. It doesn't look like we have any TwoLineRow consumers using this method, all the consumers expect to be interacting with a TwoLinePageRow or PanelListRow, so we don't even take advantage of that abstract method now as it is.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8368846 [details] [diff] [review]
Introduce ItemType into HomeConfig/Home.jsm (r=margaret)

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

Clearing review until we decide on item type names.

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: Introduce ItemType into HomeConfig/Home.jsm (r=margaret)

Comment 14

[Lucas Rocha (:lucasr)]

Attachment: Generalize text appearance style for titles and descriptions (r=margaret)

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Use ItemType to define panel view items (r=margaret)

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 8377227 [details] [diff] [review]
Introduce ItemType into HomeConfig/Home.jsm (r=margaret)

I went with 'article' and 'image' for the item type names. I can easily rename it to something else. Just didn't want to block on naming here.

I decided to not enforce item types per panel view because this is likely to explode in terms of complexity in the long term and, more importantly, there's nothing fundamentally 'wrong' with using an article item type in a grid.

LIST defaults to article item type and GRID defaults to image item type.

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 8377228 [details] [diff] [review]
Generalize text appearance style for titles and descriptions (r=margaret)

So that we can reuse the same text style across built-in rows and panel item views. I filed a follow-up (bug 973666) to get rid of the TwoLineRow base class as it's not needed anymore.

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 8377229 [details] [diff] [review]
Use ItemType to define panel view items (r=margaret)

Here we go. This patch consolidates all panel item views into a single view group (PanelItemView) that can take different layouts in its constructor. All panel item views will have to bind to the same type of data (title, description, image url, ...) so the code to bind data into the UI is pretty the same for all item views (at least for now). PanelListRow and PanelGridItemView are removed here as they're not needed anymore.

Comment 19

[Lucas Rocha (:lucasr)]

Ian, here are some screenshots of the items:
https://dl.dropboxusercontent.com/u/1187037/grid.png
https://dl.dropboxusercontent.com/u/1187037/list.png

Comment 20

[Lucas Rocha (:lucasr)]

Attachment: Use ItemType to define panel view items (r=margaret)

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 8377502 [details] [diff] [review]
Use ItemType to define panel view items (r=margaret)

Simplified alignment logic a bit.

Comment 22

[Ian Barlow (:ibarlow)]

(In reply to Lucas Rocha (:lucasr) from comment #19)
> Ian, here are some screenshots of the items:
> https://dl.dropboxusercontent.com/u/1187037/grid.png
> https://dl.dropboxusercontent.com/u/1187037/list.png

Looking good

Comment 23

[Lucas Rocha (:lucasr)]

Just a heads up: ibarlow and I will be discussing some layout details for the article and image items tomorrow. Whatever we decide, it shouldn't affect this patch in any big way i.e. it will probably be just about tweaking layout/style files a bit.

Comment 24

[:Margaret Leibovic]

Comment on attachment 8377502 [details] [diff] [review]
Use ItemType to define panel view items (r=margaret)

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

Nice, this is slick. I think it would also be good to sprinkle some documentation on the new classes, in order to keep a handle on our ever-evolving set of data types :)

::: mobile/android/base/home/PanelItemView.java
@@ +50,5 @@
> +        if (hasTitle) {
> +            mTitle.setText(title);
> +
> +            int descriptionIndex = cursor.getColumnIndexOrThrow(HomeItems.DESCRIPTION);
> +            final String description = cursor.getString(descriptionIndex);

Would we ever want to support an item with only a description and no title?

::: mobile/android/base/resources/layout/panel_article_item.xml
@@ +15,5 @@
> +    <LinearLayout android:id="@+id/title_desc_container"
> +                  android:layout_width="fill_parent"
> +                  android:layout_height="wrap_content"
> +                  android:padding="10dip"
> +                  android:minHeight="@dimen/page_row_height"

Follow-up for a more generic dimen name here?

@@ +30,5 @@
> +                  style="@style/Widget.PanelItemView.Description"
> +                  android:layout_width="fill_parent"
> +                  android:layout_height="0dp"
> +                  android:layout_weight="2"
> +                  android:maxLength="1024"/>

Nice. We should make sure to document this max length for add-on developers.

::: mobile/android/base/resources/values/styles.xml
@@ +133,5 @@
> +    <style name="Widget.PanelItemView" />
> +
> +    <style name="Widget.PanelItemView.Title">
> +        <item name="android:textAppearance">@style/TextAppearance.Widget.Home.ItemTitle</item>
> +        <item name="android:maxLines">2</item>

How does this interact with the singleLine="true" in the item layouts? Should we remove this if it has no effect?

Comment 25

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #24)
> Comment on attachment 8377502 [details] [diff] [review]
> Use ItemType to define panel view items (r=margaret)
> 
> Review of attachment 8377502 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, this is slick. I think it would also be good to sprinkle some
> documentation on the new classes, in order to keep a handle on our
> ever-evolving set of data types :)
> 
> ::: mobile/android/base/home/PanelItemView.java
> @@ +50,5 @@
> > +        if (hasTitle) {
> > +            mTitle.setText(title);
> > +
> > +            int descriptionIndex = cursor.getColumnIndexOrThrow(HomeItems.DESCRIPTION);
> > +            final String description = cursor.getString(descriptionIndex);
> 
> Would we ever want to support an item with only a description and no title?

ibarlow wants to allow such case for galleries.

> ::: mobile/android/base/resources/layout/panel_article_item.xml
> @@ +15,5 @@
> > +    <LinearLayout android:id="@+id/title_desc_container"
> > +                  android:layout_width="fill_parent"
> > +                  android:layout_height="wrap_content"
> > +                  android:padding="10dip"
> > +                  android:minHeight="@dimen/page_row_height"
> 
> Follow-up for a more generic dimen name here?

Sure, filed bug 974427.

> @@ +30,5 @@
> > +                  style="@style/Widget.PanelItemView.Description"
> > +                  android:layout_width="fill_parent"
> > +                  android:layout_height="0dp"
> > +                  android:layout_weight="2"
> > +                  android:maxLength="1024"/>
> 
> Nice. We should make sure to document this max length for add-on developers.

This is more of a 'hack' to make textview measurement a bit faster. Can't find the bug number now. In practice, we will never actually show 1024 chars as the space is very limited.

> ::: mobile/android/base/resources/values/styles.xml
> @@ +133,5 @@
> > +    <style name="Widget.PanelItemView" />
> > +
> > +    <style name="Widget.PanelItemView.Title">
> > +        <item name="android:textAppearance">@style/TextAppearance.Widget.Home.ItemTitle</item>
> > +        <item name="android:maxLines">2</item>
> 
> How does this interact with the singleLine="true" in the item layouts?
> Should we remove this if it has no effect?

singleLine takes precedence over maxLines. I should have pointed this out when I posted the patch, sorry. ibarlow wants a GridView that can contain items with variable heights. GridView as is doesn't support that. The singleLine in panel_image_item.xml is only a temporary attribute to ensure all the items have the same height. I filed bug 974434 to track the necessary work in PanelGridView.

Comment 26

[Lucas Rocha (:lucasr)]

Filed 974454 as a follow-up to allow image items to show description only (even if title is not present). I could do it here but I'd rather land the panel item framework now and work on refinements in separate bugs.

Comment 27

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/49ce1474b9af
https://hg.mozilla.org/integration/fx-team/rev/90be3775d43d
https://hg.mozilla.org/integration/fx-team/rev/c2d4afef1eb1

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49ce1474b9af
https://hg.mozilla.org/mozilla-central/rev/90be3775d43d
https://hg.mozilla.org/mozilla-central/rev/c2d4afef1eb1