1288106 - Explore implementing layout manager for multi row panel

Status: RESOLVED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 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!

Comment 1

[Jonathan Almeida [:jonalmeida]]

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

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

> [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?

Comment 3

[Jonathan Almeida [:jonalmeida]]

Yes! I was having a look at all our bugs yesterday to see which ones I wanted to work on. :)

Taking.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: netflix-uiautomatorviewer.png

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.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: playstore-recyclerviewrecyclerview.png

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

Comment 6

[Jonathan Almeida [:jonalmeida]]

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.

Comment 7

[Jonathan Almeida [:jonalmeida]]

Attachment: Screen Shot 2016-08-01 at 1.11.06 AM.png

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?

Comment 8

[Jonathan Almeida [:jonalmeida]]

NI ahunt as well for reference.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 10

[Andrzej Hunt :ahunt]

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.

Comment 11

[Jonathan Almeida [:jonalmeida]]

Attachment: (WIP) - Explore implementing layout manager for multi row panel

Comment 12

[Jonathan Almeida [:jonalmeida]]

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

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 14

[Jonathan Almeida [:jonalmeida]]

Attachment: (WIP) - Explore implementing layout manager for multi row panel

Comment 15

[Jonathan Almeida [:jonalmeida]]

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!

Comment 16

[Andrzej Hunt :ahunt]

Attachment: Bug 1288106 - Fix ActivityStream layout initialisation

Review commit: https://reviewboard.mozilla.org/r/68788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68788/

Comment 17

[Andrzej Hunt :ahunt]

Attachment: Bug 1288106 - Explore implementing layout manager for multi row panel

Review commit: https://reviewboard.mozilla.org/r/68790/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68790/

Comment 18

[Jonathan Almeida [:jonalmeida]]

Attachment: (WIP) Bug 1288106 - Explore implementing layout manager for multi row panel

Review commit: https://reviewboard.mozilla.org/r/69274/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69274/

Comment 19

[Jonathan Almeida [:jonalmeida]]

Attachment: Bug 1288106 - Fix ActivityStream layout initialisation

Review commit: https://reviewboard.mozilla.org/r/69276/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69276/

Comment 20

[Jonathan Almeida [:jonalmeida]]

Attachment: Bug 1288106 - Setting a fixed height for nested recyclerviews

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/

Comment 21

[Jonathan Almeida [:jonalmeida]]

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.

Comment 22

[Jonathan Almeida [:jonalmeida]]

Attachment: Bug 1288106 - More fixed height changes and tweaks

Review commit: https://reviewboard.mozilla.org/r/71070/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71070/

Comment 23

[Jonathan Almeida [:jonalmeida]]

Attachment: Bug 1288106 - History RecyclerView layout fixtures and cleanup

Review commit: https://reviewboard.mozilla.org/r/71072/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71072/

Comment 24

[Andrzej Hunt :ahunt]

Attachment: Bug 1288106 - Pre: make SimpleCursorLoader public to allow reuse within AS

Review commit: https://reviewboard.mozilla.org/r/72062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72062/

Comment 25

[Andrzej Hunt :ahunt]

Attachment: Bug 1288106 - Move history (highlights) into main recyclerview, and eliminated horizontal highlights

Moving all vertically listed items into one RecylerView avoids the layouting issues encountered
with nested RVs. This results in a slightly more complex adapter, however overall this still
seems simpler than having to hack around the RecyclerView height measurements. (This also
makes the layout itself simpler, which hopefully means better performance too.)

Review commit: https://reviewboard.mozilla.org/r/72064/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72064/

Comment 26

[Andrzej Hunt :ahunt]

Comment on attachment 8777181 [details]
Bug 1288106 - Fix ActivityStream layout initialisation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68788/diff/1-2/

Comment 27

[Andrzej Hunt :ahunt]

Comment on attachment 8777182 [details]
Bug 1288106 - Explore implementing layout manager for multi row panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68790/diff/1-2/

Comment 28

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 29

[Andrzej Hunt :ahunt]

Comment on attachment 8777181 [details]
Bug 1288106 - Fix ActivityStream layout initialisation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68788/diff/2-3/

Comment 30

[Andrzej Hunt :ahunt]

Comment on attachment 8777182 [details]
Bug 1288106 - Explore implementing layout manager for multi row panel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68790/diff/2-3/

Comment 31

[Andrzej Hunt :ahunt]

Comment on attachment 8781719 [details]
Bug 1288106 - Pre: make SimpleCursorLoader public to allow reuse within AS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72062/diff/1-2/

Comment 32

[Andrzej Hunt :ahunt]

Comment on attachment 8781720 [details]
Bug 1288106 - Move history (highlights) into main recyclerview, and eliminated horizontal highlights

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72064/diff/1-2/

Comment 33

[Andrzej Hunt :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 34

[Jonathan Almeida [:jonalmeida]]

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.

Comment 35

[Andrzej Hunt :ahunt]

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 36

[Sebastian Kaspari (:sebastian; :pocmo)]

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 37

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8781720 [details]
Bug 1288106 - Move history (highlights) into main recyclerview, and eliminated horizontal highlights

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

Comment 38

[Jonathan Almeida [:jonalmeida]]

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

Comment 39

[Andrzej Hunt :ahunt]

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 40

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8781719 [details]
Bug 1288106 - Pre: make SimpleCursorLoader public to allow reuse within AS

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

Comment 41

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8777181 [details]
Bug 1288106 - Fix ActivityStream layout initialisation

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

Comment 42

[Jonathan Almeida [:jonalmeida]]

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 43

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8781720 [details]
Bug 1288106 - Move history (highlights) into main recyclerview, and eliminated horizontal highlights

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

Comment 44

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/873e1bd0833a
https://hg.mozilla.org/mozilla-central/rev/0becc8c0fd30
https://hg.mozilla.org/mozilla-central/rev/c499af51f167
https://hg.mozilla.org/mozilla-central/rev/08e9ded26ada