Closed Bug 1288106 Opened 8 years ago Closed 8 years ago

Explore implementing layout manager for multi row panel

Categories

(Firefox for Android Graveyard :: General, defect, P1)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect

Tracking

(firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: sebastian, Assigned: ahunt)

References

Details

(Whiteboard: [MobileAS])

Attachments

(14 files, 1 obsolete file)

as-panel-mock.png
556.97 KB, image/png
Details
netflix-uiautomatorviewer.png
2.00 MB, image/png
Details
playstore-recyclerviewrecyclerview.png
1.13 MB, image/png
Details
Screen Shot 2016-08-01 at 1.11.06 AM.png
770.55 KB, image/png
Details
(WIP) - Explore implementing layout manager for multi row panel
26.49 KB, patch
Details | Diff | Splinter Review
Bug 1288106 - Fix ActivityStream layout initialisation
58 bytes, text/x-review-board-request
jonalmeida
: review+
sebastian
: review+
Details
Bug 1288106 - Explore implementing layout manager for multi row panel
58 bytes, text/x-review-board-request
Details
(WIP) Bug 1288106 - Explore implementing layout manager for multi row panel
58 bytes, text/x-review-board-request
Details
Bug 1288106 - Fix ActivityStream layout initialisation
58 bytes, text/x-review-board-request
Details
Bug 1288106 - Setting a fixed height for nested recyclerviews
58 bytes, text/x-review-board-request
Details
Bug 1288106 - More fixed height changes and tweaks
58 bytes, text/x-review-board-request
Details
Bug 1288106 - History RecyclerView layout fixtures and cleanup
58 bytes, text/x-review-board-request
Details
Bug 1288106 - Pre: make SimpleCursorLoader public to allow reuse within AS
58 bytes, text/x-review-board-request
jonalmeida
: review+
Details
Bug 1288106 - Move history (highlights) into main recyclerview, and eliminated horizontal highlights
58 bytes, text/x-review-board-request
jonalmeida
: review+
sebastian
: review+
Details
Attached image as-panel-mock.png — 
(See attached screenshot) The current mocks envision a netflix-like panel with multiple rows that can scroll. There's no out-of-the-box Android component for that, so let's explore how we can build this (maybe even outside of Fennec).

It looks like a job for a RecyclerView with a custom LayoutManager. But this could also be a static layout (because we have a static number of rows for now) with a bunch of horizontal list views.

What does Netflix use? Are there articles about writing such a layout or is there sample code? Explore!
I came across this StackOverflow question[1] that looks similar to what we want to do. One of the solutions has some useful sample code[2] we can look at that uses two RecyclerViews with dynamic content with an adapter that uses a List<List<Item>>.

[1]: https://stackoverflow.com/questions/27077878/how-to-create-scrollable-page-of-carousels-in-android
[2]: https://github.com/hardworker93/carousels
> [1]:
> https://stackoverflow.com/questions/27077878/how-to-create-scrollable-page-
> of-carousels-in-android
> [2]: https://github.com/hardworker93/carousels

Interesting! The posting and code is 2 years old.. I wonder if there are now more advanced solutions using RecyclerView.

(In reply to Jonathan Almeida (:jonalmeida) from comment #1)
> I came across this StackOverflow question[1] that looks similar to what we
> want to do. One of the solutions has some useful sample code[2] we can look
> at that uses two RecyclerViews with dynamic content with an adapter that
> uses a List<List<Item>>.

In our case we might have a List<Cursor> - at least the data we have right now comes from a content provider / database in the form of cursors (top sites, recent bookmarks).
This will have implications on how we load the data too: Can we display some data before the other data is available (I guess we do not want to wait until ALL cursors are available). Can we show a "place holder" UI until then?

@jonathan: Do you want to explore this more and write some code to evaluate some ideas?
Yes! I was having a look at all our bugs yesterday to see which ones I wanted to work on. :)

Taking.
Assignee: nobody → jonalmeida942
Looking at the netflix app with uiautomatorviewer they seem to use a listview with a bunch of viewpagers (for horizontal scrolling content; and three movie thumbnails / page).

What's nice about the viewpager is that it "snaps". However looking at the mock it seems like that's not what we are looking for (although it could "snap" AND show a little bit of the next item).

Right now I'd try to go with a vertical RecyclerView that has multiple horizontal RecyclerViews inside (+ some additional views for the headers etc.). In the best case we configure it somewhere in code and the rows appears as soon as data for this row is available (cursor loader?): Show data as early as possible but do not wait for all data for *all* rows to be available before displaying something.

Is this what you are exploring or do you have a different idea? Let me know if I can help building some of the UI parts.
Actually the Play Store has almost exactly the UI we are looking for. And it seems to use RecyclerViews inside a RecyclerView. This makes it look like a reasonable approach. ;)
I've only been looking at the Play Store's implementation and didn't think about using uiautomationviewer before. I was looking into using Lucas' TwoWayView again or using a NestedScrollView. Going about it with just two RecyclerViews seems like the most common approach now. So I'm just going to stick with that then and see if there are any scrolling issues that come from that.
Whiteboard: [MobileAS s1.1]
This is what I've got working with some hardcoded strings and thumbnail images. Each row has a separate adapter that we can tailor to a different source.

Does this look like a reasonable start? If so, how would I push this for review? Should I include my placeholder images/text as well? Or just leave it in review until I have an adapter that I can plug into for each of the data sources?
Flags: needinfo?(s.kaspari)
NI ahunt as well for reference.
Flags: needinfo?(ahunt)
(In reply to Jonathan Almeida (:jonalmeida) from comment #7)
> Does this look like a reasonable start?

Yes, definitely!

> If so, how would I push this for
> review? Should I include my placeholder images/text as well? Or just leave
> it in review until I have an adapter that I can plug into for each of the
> data sources?

Let's land it as early as possible so that we can all improve it and iterate. Placeholder text is okay (shouldn't be in the translation files though) but let's not land images. Either set no image or reference one that is already in the tree. It doesn't matter if it looks weird, it's hidden behind the build flag anyways. :)
Flags: needinfo?(s.kaspari)
Just a small note (I haven't seen the code so not sure if this applies): lint probably won't like hardcoded text, but if it's in xml you can add "tools:ignore="HardcodedText"" to the relevant xml tag that contains text, there's probably a similar thing you can do for java code (although I'm not sure Lint will complain in that case).

It would definitely be useful to land this early though and then iterate, especially since that means we could plumb in the detail activity / other related UI.
Flags: needinfo?(ahunt)
Comment on attachment 8776767 [details] [diff] [review]
(WIP)  - Explore implementing layout manager for multi row panel

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

This patch works fine when I attach it to an activity, but when I moved it over to the HomePanel the views don't show up but I'm able to see them on the Hierarchy Viewer. I'm almost certain it's how I'm attaching my inflated views to the root view, but I can't see what I've done wrong..
Attachment #8776767 - Flags: feedback?(ahunt)
Comment on attachment 8776767 [details] [diff] [review]
(WIP)  - Explore implementing layout manager for multi row panel

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

This patch is missing all the new files :)
Attachment #8776767 - Attachment is obsolete: true
Attachment #8776767 - Flags: feedback?(ahunt)
Comment on attachment 8777028 [details] [diff] [review]
(WIP)  - Explore implementing layout manager for multi row panel

(In reply to Sebastian Kaspari (:sebastian) from comment #13)
> Comment on attachment 8776767 [details] [diff] [review]
> (WIP)  - Explore implementing layout manager for multi row panel
> 
> Review of attachment 8776767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch is missing all the new files :)

Whoops, sorry!
Attachment #8777028 - Flags: feedback?(ahunt)
The `wrap_content` layout param for a RecyclerViews don't work with
our version of the support library. For now, the quicker solution
is to hardcode the height of the views as needed.

Review commit: https://reviewboard.mozilla.org/r/69278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69278/
Whiteboard: [MobileAS s1.1] → [MobileAS s1.2]
Status: NEW → ASSIGNED
Putting my comments from IRC here:

This is where I'm currently stuck - I have my recyclerview to have a layout height as match_parent but it’s still wrapping around nothing. It only shows up when I have enter a fixed size for the view and I can’t do that because the size will depend on the amount of items in the history.

It might be easier now to consider updating the recyclerview support library to fix some of the bugs we're seeing in the older version of the library.
Depends on: 1293790
Comment on attachment 8781719 [details]
Bug 1288106 - Pre: make SimpleCursorLoader public to allow reuse within AS

https://reviewboard.mozilla.org/r/72062/#review69790

::: mobile/android/base/java/org/mozilla/gecko/home/SimpleCursorLoader.java:38
(Diff revision 1)
>   * @deprecated since the framework provides an implementation, we'd like to eventually remove
>   *             this class to reduce maintenance burden. Originally planned for bug 1239491, but
>   *             it'd be more efficient to do this over time, rather than all at once.
>   */
>  @Deprecated
> -abstract class SimpleCursorLoader extends AsyncTaskLoader<Cursor> {
> +public abstract class SimpleCursorLoader extends AsyncTaskLoader<Cursor> {

Since this class is deprecated: Can we use the framework's loader here? Or will this touch some legacy code that still requires our custom loader class?
Assignee: jonalmeida942 → ahunt
(In reply to Sebastian Kaspari (:sebastian) from comment #28)
> Comment on attachment 8781719 [details]
> Bug 1288106 - Pre: make SimpleCursorLoader public to allow reuse within AS
> 
> https://reviewboard.mozilla.org/r/72062/#review69790
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/home/SimpleCursorLoader.java:38
> (Diff revision 1)
> >   * @deprecated since the framework provides an implementation, we'd like to eventually remove
> >   *             this class to reduce maintenance burden. Originally planned for bug 1239491, but
> >   *             it'd be more efficient to do this over time, rather than all at once.
> >   */
> >  @Deprecated
> > -abstract class SimpleCursorLoader extends AsyncTaskLoader<Cursor> {
> > +public abstract class SimpleCursorLoader extends AsyncTaskLoader<Cursor> {
> 
> Since this class is deprecated: Can we use the framework's loader here? Or
> will this touch some legacy code that still requires our custom loader class?

For now I'm reusing the getRecentHistory() call - upgrading that would require updating legacy code as you say. I'm only using history since we don't have any better data for Highlights, I was guessing we're going to write a new query at some point, and it would seem to be more efficient to use the framework CursorLoader when we do that? (I've created a new call for AS topsites using the framework CursorLoader, since we already know the data that goes into TopSties.)
Comment on attachment 8781720 [details]
Bug 1288106 - Move history (highlights) into main recyclerview, and eliminated horizontal highlights

https://reviewboard.mozilla.org/r/72064/#review70040

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/StreamRecyclerAdapter.java:35
(Diff revision 2)
> -        this.context = context;
> +        if (position == 0) {
> +            return ViewType.TopPanel;
> +        } else if (position == getItemCount() - 1) {
> +            return ViewType.BottomPanel;
> +        } else {
> +            // TODO: in future we'll want to create different items for some results, tbc?

From what I understand, the items that will be a `HighlightItem` would be those that have an extracted image large enough to be made into one of those thumbnail and not by the specific position. I'm not sure if we're going to be given the position or if we'll have to figure out from that criteria which items to chose.

/twocents

::: mobile/android/base/resources/layout/activity_stream_card_highlights_item.xml:66
(Diff revision 2)
> -                    android:src="@drawable/search_icon_active"
> -                    android:alpha="0.5"
> -                    android:layout_width="18dp"
> -                    android:layout_height="18dp"/>
> -
> -                <TextView
> +            <TextView

We probably need an additional `TextView` for `HighlightItem` and `CompactItem` to show where this highlight item came from. Sorry, I just realised I forgot about adding this in my initial efforts.
Attachment #8777181 - Flags: review?(s.kaspari)
Attachment #8781720 - Flags: review?(s.kaspari)
I think it's probably a good idea to land what we have here, mainly to allow developing further parts independently, and to allow easier testing. I've already split (initial) topsites into Bug 1293790, that widget can then be plugged into this layout (in my updated patches topsites is empty to allow plugging in the other patch). Once the basic layouts are landed, it's easy to tweak each part as needed (without having to build temporary scaffolding just to show any single widget).

We can then file followups to implement missing content (e.g. favicons for the Views, which depends on the favicon rework, and some refactoring made in the topsites view bug), adjust the layout, etc.
Comment on attachment 8781720 [details]
Bug 1288106 - Move history (highlights) into main recyclerview, and eliminated horizontal highlights

https://reviewboard.mozilla.org/r/72064/#review70582

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/ActivityStream.java:28
(Diff revision 2)
>  import org.mozilla.gecko.home.HomeScreen;
> +import org.mozilla.gecko.home.SimpleCursorLoader;
>  
>  public class ActivityStream extends FrameLayout implements HomeScreen {
>  
> +    StreamRecyclerAdapter adapter;

Can this be private or is the adapter accessed from other classes in this package? I often add a /* package-private */ comment in this case to express: This is not a mistake. :)

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/StreamRecyclerAdapter.java:16
(Diff revision 2)
> -import java.util.List;
> -
> -
> -class MainRecyclerAdapter extends RecyclerView.Adapter<MainRecyclerAdapter.ViewHolder> {
>  
> -    private final Context context;
> +    private enum ViewType {

I recently saw some interesting code that used the "layout id" as view type. Inflating was super simple because you didn't need to translate the view type to something that knows about the layout id.

I think it was somewhere in this I/O code but I can't find it right now seeking through the video. But the talk is pretty amazing in general :) 
https://www.youtube.com/watch?v=LqBlYJTfLP4

::: mobile/android/base/locales/en-US/android_strings.dtd:836
(Diff revision 2)
>  <!ENTITY helper_triple_readerview_open_message "Bookmark Reader View items to read them offline.">
>  <!ENTITY helper_triple_readerview_open_button "Add to Bookmarks">
>  
> +<!ENTITY activity_stream_topsites "Top Sites">
> +<!ENTITY activity_stream_highlights "Highlights">
> +<!ENTITY activity_stream_more "More">

A translation note could be helpful here to explain the context of "more".
Comment on attachment 8781720 [details]
Bug 1288106 - Move history (highlights) into main recyclerview, and eliminated horizontal highlights

https://reviewboard.mozilla.org/r/72064/#review70584
Attachment #8781720 - Flags: review?(s.kaspari) → review+
Priority: -- → P1
Whiteboard: [MobileAS s1.2] → [MobileAS]
Comment on attachment 8781720 [details]
Bug 1288106 - Move history (highlights) into main recyclerview, and eliminated horizontal highlights

https://reviewboard.mozilla.org/r/72064/#review70582

> I recently saw some interesting code that used the "layout id" as view type. Inflating was super simple because you didn't need to translate the view type to something that knows about the layout id.
> 
> I think it was somewhere in this I/O code but I can't find it right now seeking through the video. But the talk is pretty amazing in general :) 
> https://www.youtube.com/watch?v=LqBlYJTfLP4

> I think it was somewhere in this I/O code but I can't find it right now seeking through the video. But the talk is pretty amazing in general :) 
https://www.youtube.com/watch?v=LqBlYJTfLP4

Found it at 15:22. It's a nice talk. :)
Priority: P1 → P2
https://hg.mozilla.org/integration/fx-team/rev/873e1bd0833abf954ac0e0fab6c43b1ce5380e1c
Bug 1288106 - Fix ActivityStream layout initialisation r=jonalmeida

https://hg.mozilla.org/integration/fx-team/rev/0becc8c0fd30a2ac5bc1f34d80fea7b7afa4b97a
Bug 1288106 - Explore implementing layout manager for multi row panel r=ahunt

https://hg.mozilla.org/integration/fx-team/rev/c499af51f167880b1a51458a2d7d25e35e2f4b57
Bug 1288106 - Pre: make SimpleCursorLoader public to allow reuse within AS r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/08e9ded26ada149385af32e1c1d89f30e3d8279c
Bug 1288106 - Move history (highlights) into main recyclerview, and eliminated horizontal highlights r=sebastian
Comment on attachment 8781719 [details]
Bug 1288106 - Pre: make SimpleCursorLoader public to allow reuse within AS

https://reviewboard.mozilla.org/r/72062/#review70902
Attachment #8781719 - Flags: review?(jonalmeida942) → review+
Comment on attachment 8777181 [details]
Bug 1288106 - Fix ActivityStream layout initialisation

https://reviewboard.mozilla.org/r/68788/#review71296
Attachment #8777181 - Flags: review?(jonalmeida942) → review+
These were r+ already, I'm not sure why they were still in my queue. I may have forgotten to hit publish for the review, sorry!
Comment on attachment 8781720 [details]
Bug 1288106 - Move history (highlights) into main recyclerview, and eliminated horizontal highlights

https://reviewboard.mozilla.org/r/72064/#review71298
Attachment #8781720 - Flags: review?(jonalmeida942) → review+
Comment on attachment 8777181 [details]
Bug 1288106 - Fix ActivityStream layout initialisation

https://reviewboard.mozilla.org/r/68788/#review71382

I do not really understand why this is needed? Inflating an empty layout?
Attachment #8777181 - Flags: review?(s.kaspari) → review+
Priority: P2 → P1
Attachment #8777028 - Flags: feedback?(ahunt)
Attachment #8777806 - Flags: review?(ahunt)
Attachment #8777808 - Flags: review?(ahunt)
Attachment #8780345 - Flags: review?(ahunt)
Attachment #8780346 - Flags: review?(ahunt)
Attachment #8777182 - Flags: review?(ahunt)
Iteration: --- → 1.3
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: