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

RESOLVED FIXED in Firefox 30

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 30
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

The itemType parameter will define the kind of views to inflate for each child in panel list and grid views.
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.
Attachment #8368845 - Flags: review?(margaret.leibovic)
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?
Attachment #8368846 - Flags: review?(margaret.leibovic)
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?
Attachment #8368847 - Flags: review?(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 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...
(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 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.
Attachment #8368847 - Flags: review?(margaret.leibovic) → review+
Blocks: 969004
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.
Attachment #8368845 - Flags: review?(margaret.leibovic) → review-
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.
Attachment #8368846 - Flags: review?(margaret.leibovic)
Blocks: 973666
Blocks: 973670
Attachment #8368845 - Attachment is obsolete: true
Attachment #8368846 - Attachment is obsolete: true
Attachment #8368847 - Attachment is obsolete: true
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.
Attachment #8377227 - Flags: review?(margaret.leibovic)
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.
Attachment #8377228 - Flags: review?(margaret.leibovic)
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.
Attachment #8377229 - Flags: review?(margaret.leibovic)
Ian, here are some screenshots of the items:
https://dl.dropboxusercontent.com/u/1187037/grid.png
https://dl.dropboxusercontent.com/u/1187037/list.png
Flags: needinfo?(ibarlow)
Comment on attachment 8377502 [details] [diff] [review]
Use ItemType to define panel view items (r=margaret)

Simplified alignment logic a bit.
Attachment #8377502 - Flags: review?(margaret.leibovic)
Attachment #8377229 - Attachment is obsolete: true
Attachment #8377229 - Flags: review?(margaret.leibovic)
(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
Flags: needinfo?(ibarlow)
Blocks: 973137
Blocks: 970700
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.
Attachment #8377227 - Flags: review?(margaret.leibovic) → review+
Attachment #8377228 - Flags: review?(margaret.leibovic) → review+
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?
Attachment #8377502 - Flags: review?(margaret.leibovic) → review+
Blocks: 974427
Blocks: 974434
(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.
Blocks: 974454
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.
You need to log in before you can comment on or make changes to this bug.