Closed Bug 1310081 Opened 3 years ago Closed 3 years ago

Convert tabs tray grid to use a RecyclerView

Categories

(Firefox for Android :: General, defect, P3)

All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox53 --- verified

People

(Reporter: twointofive, Assigned: twointofive)

References

(Blocks 2 open bugs)

Details

Attachments

(10 files, 3 obsolete files)

58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
649.02 KB, image/png
Details
1.42 MB, image/png
Details
1.16 MB, image/png
Details
584.22 KB, image/png
Details
208.55 KB, image/png
Details
208.35 KB, image/png
Details
924.92 KB, image/png
Details
This is to split off the grid case of bug 1116415 into a separate bug.  The current plan is to land the list work in this release and then land the grid work in the next release.
Depends on: 1116415
OS: Unspecified → Android
Priority: -- → P3
Hardware: Unspecified → All
The list seems to work pretty well so far. You already had some patches for the grid in bug 1116415 - are they already ready for review? If we get everything ready then we can land this very early in the next cycle. :)
Depends on: 1313144
I'll rebase once bug 1313144 lands (assuming it does), but ignoring that this should be ready for review.  Thanks!
Comment on attachment 8804864 [details]
Bug 1310081 - 1. Move SpacingDecoration and introduce AutoFitRecyclerView.

https://reviewboard.mozilla.org/r/88698/#review88832

Nice!
Attachment #8804864 - Flags: review?(s.kaspari) → review+
Comment on attachment 8804865 [details]
Bug 1310081 - 1. Make the tabs list grid view a RecyclerView.

https://reviewboard.mozilla.org/r/88700/#review89570

Nice!

Future versions of the support library come with a DiffUtil class [1] that automatically calculates and dispatches all needed adapter updates. I wonder if this will simplify this a bit more. However we are far from updating to the new library version for now.

[1] https://developer.android.com/reference/android/support/v7/util/DiffUtil.html

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayoutAdapter.java:79
(Diff revision 1)
>  
> -        return mTabs.indexOf(tab);
> +        return tabs.indexOf(tab);
>      }
>  
> -    @Override
> -    public boolean isEnabled(int position) {
> +    /* package */ void notifyTabChanged(Tab tab) {
> +        notifyItemChanged(getPositionForTab(tab));

nit: Is (theoretically) calling notifyItemChanged(-1) a problem?
Attachment #8804865 - Flags: review?(s.kaspari) → review+
Comment on attachment 8804866 [details]
Bug 1310081 - 2. Refactor TabsListTouchHelperCallback to support any swipe alpha function.

https://reviewboard.mozilla.org/r/88702/#review89574

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsTouchHelperCallback.java:45
(Diff revision 1)
> +    // Returns the alpha an itemView should be set to when swiped by an amount dX, given that
> +    // alpha should decrease to its min at distance distanceToAlphaMin.
> +    abstract protected float alphaForItemSwipeDx(float dX, int distanceToAlphaMin);

I should have mentioned this in earlier patches: Why are you not using the multiline javadoc comment style? This will allow the IDE to show the documentation in other contexts.
Attachment #8804866 - Flags: review?(s.kaspari) → review+
Comment on attachment 8804867 [details]
Bug 1310081 - 3. Add ItemDecoration to create fixed spacing items.

https://reviewboard.mozilla.org/r/88704/#review89582

::: mobile/android/base/java/org/mozilla/gecko/widget/GridSpacingDecoration.java:13
(Diff revision 1)
> +// An ItemDecoration for a GridLayoutManager to adjust fixed width items within their columns so as
> +// to maintain as much space between items as possible.  The computations take into account and use
> +// any horizontal RecyclerView padding as part of the spacing of first and last items in a row.
> +// This decoration assumes the RecyclerView's left and right padding are equal.
> +public class GridSpacingDecoration extends RecyclerView.ItemDecoration {

As mentioned in the other patch: Comments like this should actually use the javadoc style.
Attachment #8804867 - Flags: review?(s.kaspari) → review+
Comment on attachment 8804864 [details]
Bug 1310081 - 1. Move SpacingDecoration and introduce AutoFitRecyclerView.

https://reviewboard.mozilla.org/r/88698/#review89580

::: mobile/android/base/java/org/mozilla/gecko/widget/AutoFitRecyclerView.java:61
(Diff revision 1)
> +    @Override
> +    protected void onMeasure(int widthSpec, int heightSpec) {
> +        super.onMeasure(widthSpec, heightSpec);
> +
> +        if (!autoFit || getMeasuredWidth() == 0 || desiredColumnWidth <= 0) {
> +            return;
> +        }
> +
> +        final int nonPaddingWidth = getMeasuredWidth() - getPaddingLeft() - getPaddingRight();
> +        // Adjust span based on space available (what GridView does when you say numColumns="auto_fit").
> +        final int spanCount = (int) Math.max(1, nonPaddingWidth / desiredColumnWidth);
> +        GridLayoutManager layoutManager = (GridLayoutManager) getLayoutManager();
> +        if (spanCount == layoutManager.getSpanCount()) {
> +            return;
> +        }
> +        layoutManager.setSpanCount(spanCount);
> +        onSpanCountChanged();
> +    }

You moved this code from PanelRecyclerView and I think I wrote this some time ago. Thinking about this now I wonder if this should actually be in onSizeChanged(). We only need to re-calculate the number of columns if the size has actually changed and not every time the view has been re-measured. What do you think?
Attachment #8804868 - Attachment is obsolete: true
Attachment #8804869 - Attachment is obsolete: true
Comment on attachment 8804864 [details]
Bug 1310081 - 1. Move SpacingDecoration and introduce AutoFitRecyclerView.

https://reviewboard.mozilla.org/r/88698/#review89580

> You moved this code from PanelRecyclerView and I think I wrote this some time ago. Thinking about this now I wonder if this should actually be in onSizeChanged(). We only need to re-calculate the number of columns if the size has actually changed and not every time the view has been re-measured. What do you think?

I like it!  And it looks like the order of operations is: onMeasure, then onSizeChanged, and then onLayout, so there shouldn't be any issues with flicker or thrashing if/when we're changing the layout in onSizeChanged in response to a span count change.
Attachment #8804864 - Attachment is obsolete: true
Sebastian, could you take another look at the latest version?  As we discussed, I changed the grid tabs layout behavior from:
  fixed width tab items with spacing determined by phone dimensions
to
  fixed spacing with tab items growing to fill the space allowed by phone
  dimensions
This gives more room for the tab title, and also prepares us to match what we'll be doing in the compact tabs case (bug 1202861).

As I mention in the documentation, it was necessary to allow a range of spacing values in order to get the desired span counts we currently have on varying devices.  Feel free to suggest better names for some of those variables :)

One concern with the new approach is that we wind up scaling up the thumbnail previews in some cases to fill the space available - if there are n columns, then the max factor we have to scale up by is (1 + 1/n), so the largest possible scale ups are actually with the smaller phones where we might only be able to fit two (or even one?) columns in landscape mode.  I've attached screenshots from a Galaxy S4 where I fudged the RecyclerView's left padding (and dropped all spacing) to force the largest amount of scaling we might have to do to a thumnail in the 1 and 2 column cases.  So, for example, in the image with one column there, the original thumbnail width from gecko is 468px, but the total width for tabs available is manufactured to be 935, one pixel less than enough room for two tabs, so we scale up the one tab to twice its original width.  Similarly for the two-column image where we're scaling the thumbnail by 1.5; the three column image is from the current patch with no adjustments.  The one column case is maybe just a little soft, but on the actual device I can't tell any difference, and the bunch of emulators I checked with these patches looked good.  Still, will be nice to get another set of eyes with more devices :)  Thanks!
Flags: needinfo?(s.kaspari)
(In reply to Tom Klein from comment #26)
> for example, in the image with one column there, the original thumbnail
> width from gecko is 468px

I spoke too quickly here, the situation is a bit more complicated.  Namely, the actual thumbnail width is determined by TopSitesGridView (and isn't just always R.dimen.tab_panel_item_width, the default thumbnail width, as I had been absentmindedly thinking):
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/TopSitesGridView.java#135
That sets the thumbnail width for the device in that screenshot to be basically (device width / 3), which turns out to be 596, not the 468 I quoted - so the thumbnails in those images are being scaled less than I claimed.  I haven't tried to come up with a general relationship between what thumbnail width top sites is going to set vs. what thumbnail width the tabs panel is going to use - it's possible I suppose, but there would be many cases I think.  So the safest bet would be to just have the thumbnail width be max(top sites, tabs panel), but I'll wait for feedback before making more changes.

(Note that there's an issue with just trying to make ThumbnailHelper.setThumbnailWidth set the max value it receives, namely that both onMeasure from TopSiteGridView and onSizeChanged from TabsGridLayout can be called multiple times in a single layout run, and only the last value provided is the correct one, but there's no way (to my knowledge) to know which call is the last one, so we'd have to do something more clever than just always setting thumbnail width to the max value provided.  Not to mention the possibility that actual measured device width can actually shrink (I'm thinking of the screen splitting you mentioned Sebastian).)
Attached image GooglePixelXL.png
Flags: needinfo?(s.kaspari)
I tested this on all the devices I have at hand right now and attached some screenshots. This looks pretty good on all the devices. Even if a thumbnail looks a little bit blurry in a screenshot (e.g. Z3C) it looks okay on the actual screen. I think we could try this in Nightly as-is!

The new top sites panel we are working on will not have any thumbnails anymore. As soon as this is ready (will take some time though) we only need to care about having nice thumbnails for the tabs tray.
Great! Thanks for checking all those devices :D
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d64d6d962c67
1. Make the tabs list grid view a RecyclerView. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/19f73951d4e5
2. Refactor TabsListTouchHelperCallback to support any swipe alpha function. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/59bfc0102066
3. Add ItemDecoration to create fixed spacing items. r=sebastian
Depends on: 1321002
Verified on Aurora 53.0a2 and the tabs are looking good, no issues in tabs tray view & spacing.
Devices: 
-LG G4 (Android 5.1)
-OnePlus Two (Android 6.0)
-Lenovo A536 (Android 4.4.2)
-Motorola Nexus 6 (Android 7.1.1)
-Asus Transformer Pad TF300T (Android 4.2.1)
-Huawei MediaPad M2 - Android 5.1.1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.