Closed Bug 1116415 Opened 9 years ago Closed 8 years ago

Convert tabs tray list to use a RecyclerView

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed)

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: wesj, Assigned: twointofive)

References

Details

Attachments

(6 files, 9 obsolete files)

Bug 1116415 - 1. Create new RecyclerView and adapter base classes for tabs panels.
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1116415 - 2. Make the linear tabs list a RecyclerView.
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1116415 - 3. Add an ItemTouchHelper to the tabs panel list for swipe to close.
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1116415 - 4. Add an item animator to the tabs list for removes.
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1116415 - 5. Send the index of an added tab as data.
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1116415 - 6. Provide fixed TouchDelegate class.
58 bytes, text/x-review-board-request
sebastian
: review+
Details 
Our tabs tray right now is a ListView (err TwoWayView). Reading a bit about RecyclerView's though, it seems to do most of what we need, and in fact make some things (like animating items in or out of the list) a bit easier. We should look into making the switch and moving a bunch of code off our shoulders onto Android's. There's a compat library for this that I think should be enough for us to use this across Android versions.
Err. Looking more I didn't realize lucas had updated TwoWayView to use the new recycler stuff. I still assume we don't use that since I don't think we have the compat libraries in place. Do we?
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Wesley Johnston (:wesj) from comment #1)
> Err. Looking more I didn't realize lucas had updated TwoWayView to use the
> new recycler stuff. I still assume we don't use that since I don't think we
> have the compat libraries in place. Do we?

We just need to use the latest compat library to use RecyclerView and the stock layout manager.

I don't recommend using the new TwoWayView just yet because it's still under heavy development. Even though there are a couple of core APIs that might be useful even if we decide to use the stock layout managers. Namely: item click and selection support. See:

https://github.com/lucasr/twoway-view/tree/master/core/src/main/java/org/lucasr/twowayview
Flags: needinfo?(lucasr.at.mozilla)
Blocks: 1158303
Assignee: nobody → mhaigh
Attached patch WIP Patch (obsolete) — — Splinter Review 
I was playing about with implementing a recycler view for the tabs panel.  I was having some issues with autofit columns and highlighting.  Swipe to dismiss hasn't been implemented.  There's an issue where we're being given the incorrect width on screen rotation, so sometimes the calculated column widths are way, way off.
Flags: needinfo?(s.kaspari)
Assignee: mhaigh → s.kaspari
(In reply to Martyn Haigh (:mhaigh) from comment #3)
> Created attachment 8671931 [details] [diff] [review]
> WIP Patch
> 
> I was playing about with implementing a recycler view for the tabs panel.  I
> was having some issues with autofit columns and highlighting.  Swipe to
> dismiss hasn't been implemented.  There's an issue where we're being given
> the incorrect width on screen rotation, so sometimes the calculated column
> widths are way, way off.

Thanks for the WIP patch! I expect the switching between list / grid and swipe-to-dismiss optimization to be much easier with a RecyclerView-based implementation.
Flags: needinfo?(s.kaspari)
Assignee: s.kaspari → nobody
I'm new to android, so please let me know where there may be better/more correct ways to do some of these things :)

As far as I can tell though everything seems to be working, with the exception of testSessionOOMRestore, which is currently failing on phones because it assumes the old base class.  I can fix that in a followup commit (on this bug), but I'd like to get some feedback on how this is looking and whether you'd want to land just the list piece on this bug or if you'd want to switch the grid case to RecyclerView at the same time, in which case the test update will be simpler.  In either case I guess we'd want to wait until after the release next week to land this.  Thanks!
Assignee: nobody → twointofive
(In reply to Tom Klein from comment #6)
> I'm new to android, so please let me know where there may be better/more
> correct ways to do some of these things :)
> 
> As far as I can tell though everything seems to be working, with the
> exception of testSessionOOMRestore, which is currently failing on phones
> because it assumes the old base class.  I can fix that in a followup commit
> (on this bug), but I'd like to get some feedback on how this is looking and
> whether you'd want to land just the list piece on this bug or if you'd want
> to switch the grid case to RecyclerView at the same time, in which case the
> test update will be simpler.  In either case I guess we'd want to wait until
> after the release next week to land this.  Thanks!

Awesome. Thank you! This is a big patch and will take some time to test and review (and I will be on PTO the next two weeks), but this is a great thing.

> but I'd like to get some feedback on how this is looking and
> whether you'd want to land just the list piece on this bug or if you'd want
> to switch the grid case to RecyclerView at the same time, in which case the
> test update will be simpler.

That's something we would like to do eventually. It might make sense to land the list first and work on the grid in a follow-up bug. If it is easier to do it now then go ahead. Just split it over multiple commits for review please. :)

> In either case I guess we'd want to wait until
> after the release next week to land this.  Thanks!

Yeah, definitely. This needs some time in Nightly to make sure it is working well.
Comment on attachment 8789490 [details]
Bug 1116415 - 1. Create new RecyclerView and adapter base classes for tabs panels.

https://reviewboard.mozilla.org/r/77678/#review76256

This is already working great on my phone! :)

The code looks good already too. It would be nice if we could make more use of notifyItemChanged(), notifyItemAdded(), notifyItemRemoved() and let the binder change views only. Modifying the views outside of the adapter flow can be problematic. And with using the method we can get a lot of things for free (if we want to do more with the animation APIs etc.)

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java:53
(Diff revision 1)
> -        tabsAdapter = new TabsListLayoutAdapter(context);
> +        tabsAdapter = new TabsListLayoutAdapter(context, R.layout.tabs_list_item_view,
> +                /* on viewItem click listener */
> +                new View.OnClickListener() {
> +                    @Override
> +                    public void onClick(View v) {
> +                        TabsLayoutItemView item = (TabsLayoutItemView) v;
> +                        final int tabId = item.getTabId();
> +                        final Tabs tabs = Tabs.getInstance();
> +                        tabs.selectTab(tabId);
> +                        autoHidePanel();
> +                        tabs.notifyListeners(tabs.getTab(tabId), Tabs.TabEvents.OPENED_FROM_TABS_TRAY);
> +                    }
> +                },
> +                /* close on click listener */
> +                new Button.OnClickListener() {
> +                    @Override
> +                    public void onClick(View v) {
> +                        // The view here is the close button, which has a reference
> +                        // to the parent TabsLayoutItemView in its tag, hence the getTag() call.
> +                        TabsLayoutItemView itemView = (TabsLayoutItemView) v.getTag();
> +                        closeTab(itemView);
> +                    }
> +                });

This is maybe a bit much inlining. Could the layout implement those interfaces or could we move this to a child class?

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java:156
(Diff revision 1)
> -                View view = getChildAt(tabsAdapter.getPositionForTab(tab) - getFirstVisiblePosition());
> -                if (view == null) {
> +                final ViewHolder viewHolder = findViewHolderForAdapterPosition(tabsAdapter.getPositionForTab(tab));
> +                if (viewHolder == null) {
>                      return;
>                  }
>  
> -                TabsLayoutItemView item = (TabsLayoutItemView) view;
> -                item.assignValues(tab);
> +                final TabsLayoutItemView itemView = (TabsLayoutItemView) viewHolder.itemView;
> +                itemView.assignValues(tab);

Could we call notifyItemChanged() changed here and let the binder update the views?

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java:181
(Diff revision 1)
>      private void updateSelectedStyle(int selected) {
> -        for (int i = 0; i < tabsAdapter.getCount(); i++) {
> -            setItemChecked(i, (i == selected));
> +        // We'll get null here if we just called refreshTabsData (which leads to an
> +        // adapter.notifyDataSetChanged()), so in that case we rely on the fact that we also set
> +        // checked state in onBindViewHolder.
> +        final RecyclerView.ViewHolder selectedViewHolder = findViewHolderForAdapterPosition(selected);
> +        if (selectedViewHolder == null) {
> +            return;
> +        }
> +        final View selectedView = selectedViewHolder.itemView;
> +        final int displayCount = getChildCount();
> +        for (int i = 0; i < displayCount; i++) {
> +            final View displayedView = getChildAt(i);
> +            if (displayedView != null) {
> +                final boolean checked = displayCount == 1 || displayedView == selectedView;
> +                ((TabsLayoutItemView) displayedView).setChecked(checked);
> +            }

This is a bit unusual. Could we call notifyDataSetChanged() or even better notifyItemChanged() if we know the positions and then  update the layout in the binder only?

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayoutAdapter.java:53
(Diff revision 1)
> +        this.tabs = tabs;
> +        notifyDataSetChanged();
> +    }
> +
> +    /* package */ final void clear() {
> +        tabs = null;

How about setting it here (and in the constructor) to Collections.emptyList() (or an actual empty list) to avoid having to deal with nulls everywhere.

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayoutAnimator.java:34
(Diff revision 1)
> +        if (!removalsPending) {
> +            super.runPendingAnimations();
> +            return;
> +        }
> +
> +        // Remove stuff.

I'm not sure if "Remove stuff" is a helpful comment ;-)
Attachment #8789490 - Flags: review?(s.kaspari)
Attachment #8671931 - Attachment is obsolete: true
Thanks for the quick feedback before PTO last time Sebastian :D

This series updates both the list and grid tab panel views to RecyclerViews; I think the list version is looking good so far, but the grid view has multiple issues (which ballooned this bug...).  I've been able to fix or work around most of them, but there's still one issue with scrolling a GridLayoutManager that I haven't been able to work around (1a).  I'm submitting the grid code anyway in the hope that somebody will be able to tell me that I'm doing it wrong, or, failing that, that we'll be able to return to this in some later update of the support library when some of these issues have been fixed.

I'm not really expecting a full review at this point.

So the grid issues are:
1) Scrolling.  There are several situations in which the default RecyclerView/DefaultItemAnimator leaves the selected item hidden offscreen after an update (either a first show or an add or a remove).  The solution is to scroll the selected item into view in those cases, but the underlying scrolling on a GridLayoutManager seems to be pretty flaky.

The three cases where the default behavior requires an extra grid scroll are:

a) When we change orientation to the grid view (or when we have a bunch of tabs open on the first tabs panel view) we need to scroll to the selected position.  None of the scroll methods quite works in that case.  :-(

Here's scrolling after a first view using scrollToPosition: https://youtu.be/iBmrD4xN0lo  Note that the layout is completely wrong here, not just the scrolled to position.
And here it is using scrollToPositionWithOffset (or smoothScrollToPosition): https://www.youtube.com/watch?v=lEqd6aAGipw  It's better, but still not quite right, and the result varies.

Delaying the scroll seems to fix things (with scrollToOffset), but then there's a race between "how long do I need to delay in order to get this animation to work" and "how soon is this scroll animation going to be visible to the user after we've rotated".  Maybe it would be possible to cook up an "immediate scrollTo" function (or is there one already?)?

b) When you have more than a screen's worth of tabs open and you close the first
tab and then undo the close, for some reason there's a scroll which makes the
first row almost hidden: https://www.youtube.com/watch?v=V7Au_QqobVA If you do a
scrollToPosition(0) or a scrollToPositionWithOffset(0, 0) to try to fix it you get this error:
https://www.youtube.com/watch?v=bF6KWcNemE0 (the new tab appears to animate in
from the top), while if you use smoothScrollToPosition(0) you get a different
layout error: https://www.youtube.com/watch?v=5mrlyUCB-og (the animation swings
down and then back up).

This issue could be avoided by just going back to calling notifyDataSetChanged
when we add the new item (instead of using notifyItemAdded), but of course then we lose the nice insertion animation (on the other hand we don't have it now, so it won't be missed!).

c) When you have more than a screen's worth of tabs open and you close a tab on its own on the last row, that row disappears, and then when you undo the close the row is recreated, but not scrolled to by default.  In this case you can smoothScroll to the last position and it seems to work (scrollToPosition doesn't quite work), so this is fixable/is fixed.

I thought maybe post()ing scrolls, or layoutManager.postOnAnimation()ing  scrolls would help, and in some cases it makes a difference (smoothScrollToPosition does nothing in case a) without it), but none of these issues are completely fixed by doing that.

So if we can find a workaround for a) then the grid stuff might work.  On the other hand, so much of the scrolling with a GridLayoutManager seems to be broken that I feel a little bit hesitant even if we can get a) working.  What do you think?  Again, the best answer would be that you tell me I'm doing it wrong! :-)

If we're not comfortable with the grid case then I'd be happy to just resubmit the list case for now.

2) It looks to me like Android's TouchDelegate has a bug.  See https://www.youtube.com/watch?v=PJtBEur1yIw for how it manifests with RecyclerView.  TouchDelegate maintains a state saying whether or not its the active delegate for gestures, but it never resets that state (unless it receives a cancel), so once it's received a delegate click, it always thinks it should delegate.  So in the video we click near the close button (but not on it), which sets the delegate's state to accepting, then we undo that close, which recycles that view into the first position, and then when you click anywhere outside the delegate area in that view the view checks with its delegate (on ACTION_UP), which says "yes, that's mine", and then proceeds to ignore it because it's not actually in its delegation region.

The error occurs in both the grid and the list case, but it's much more difficult to hit in the list case because in that case the delegation corner is 40x40 in the upper right, while the close button is 34 wide full height.  I have hit it before though.

The TouchDelegate in the current tabs panel suffers from this issue as well, but in the GridView case at least the ACTION_UP accompanying an action down is dispatched straight to its destination without ever asking the delegate if it wants it.  And I haven't checked what's going on with the tab strip TouchDelegate, but I can't produce any error there either.  Still, if the patched TouchDelegate looks right to you, then maybe we should do a global replacement in our code (and I should submit a bug report).  We should also maybe clean up the location of the touch delegate region in the list case.

3) With DefaultItemAnimator, notifyItemChanged triggers a fade in fade out.  We call notifyItemChanged everytime we update the title or the thumbnail etc., so it's a bit annoying to see all the flashing.  In the list layout it's enough to either setChangeDuration(0) or to call setSupportsChangeAnimations(false), but in the grid case a change animation is involved in adds and removes, so we can't just setChangeDuration(0), and setSupportsChangeAnimations(false) leads to flashing views on an add: https://youtu.be/0IIbhIjLtEo
I was able to work around that, but it's a bit of a hack.  This seems like another Android bug to me.

4) Not necessarily an issue, but is there a better way to do commit 13?  (I can add some more doc on the equations if we wind up using it.)  If we could let the thumbnail view expand then we could get more of the title in when there's room, but my (uninformed) impression is that that's not really currently possible, at least not without just scaling the fixed-size thumbnails.
Flags: needinfo?(s.kaspari)
A little more info:
If I go back to the original way of updating a view after a title/thumbnail/etc. change:
https://reviewboard-hg.mozilla.org/gecko/file/fc9e1f88787e/mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java#l156
then issues 1b) and 3) in comment 23 go away.
Here's what we're asking to happen when we have two tabs, close the first one, and then undo the close (this is the part after the undo):

09-28 23:56:40.960 D/***INSERTED tab 0
09-28 23:56:40.960 D/***SELECTED tab 0 -->notifyItemChanged
09-28 23:56:40.960 D/***UNSELECTED tab 1 -->notifyItemChanged
09-28 23:56:40.970 D/***BIND: 0:TabsLayoutItemView{32757c2}
09-28 23:56:40.970 D/***BIND: 1:TabsLayoutItemView{b0b4c50}
09-28 23:56:40.970 D/***ANIMATE ADD: TabsLayoutItemView{32757c2}
09-28 23:56:40.970 D/ALREADY ANIMATING: false
09-28 23:56:40.970 D/***ANIMATE CHANGE: fromX: 162 toX: 687 fromY: 73 toY: 73
09-28 23:56:40.970 D/***ANIMATING OLD: TabsLayoutItemView{6e0dd7c}
09-28 23:56:40.970 D/***ANIMATING NEW: TabsLayoutItemView{b0b4c50}
09-28 23:56:40.970 D/ALREADY DOING ANIMATION: true
09-28 23:56:41.100 D/***TITLE tab 0 -->notifyItemChanged
09-28 23:56:41.100 D/***BIND: 0:TabsLayoutItemView{b224605}
09-28 23:56:41.100 D/***ANIMATE CHANGE: fromX: 123 toX: 123 fromY: 73 toY: 73
09-28 23:56:41.100 D/***ANIMATING OLD: TabsLayoutItemView{32757c2}
09-28 23:56:41.100 D/***ANIMATING NEW: TabsLayoutItemView{b224605}
09-28 23:56:41.100 D/ALREADY DOING ANIMATION: true
09-28 23:56:41.490 D/***THUMBNAIL tab 0 -->notifyItemChanged
09-28 23:56:41.500 D/***BIND: 0:TabsLayoutItemView{32757c2}
09-28 23:56:41.500 D/CANCELING: cancel the change animation
09-28 23:56:41.620 D/***THUMBNAIL tab 0 -->notifyItemChanged
09-28 23:56:41.630 D/***BIND: 0:TabsLayoutItemView{b224605}
09-28 23:56:41.640 D/CANCELING: cancel the change animation

When we handle the selected/unselected and title changes outside the adapter we remove 2 binds and two change animations that otherwise occur concurrently with the add animation, all during a scroll.  It would be nice if that all just worked, but...

On a separate note, I tried a GlobalLayoutListener for issue 1a), but that didn't help.
(In reply to Tom Klein from comment #23)
> Maybe it would be possible to
> cook up an "immediate scrollTo" function (or is there one already?)?
> 
Apparently scrollTo/scrollBy provide that functionality :)  scrollTo is unsupported by RecyclerView (at least at our current version (I put it that way because there's no indication in the doc that it intentionally crashes if you try to use it in our version)).

Using the following in TabsGridLayout.updateScrollPosition (on top of my current code):
  if (getChildCount() == 0) {
    final ViewTreeObserver treeObserver = getViewTreeObserver();
    treeObserver.addOnPreDrawListener(new ViewTreeObserver.OnPreDrawListener() {
      @Override
      public boolean onPreDraw() {
        treeObserver.removeOnPreDrawListener(this);
        final View aView = getChildAt(0);
        if (aView == null) {
          return true;
        }
        final int viewHeight = aView.getHeight();
        final int verticalViewPadding =
          getContext().getResources().getDimensionPixelSize(R.dimen.tab_panel_grid_item_vpadding);
        final int row = selected / layoutManager.getSpanCount();
        final int selectedYPosition = row * (viewHeight + 2 * verticalViewPadding);
        scrollBy(0, selectedYPosition);
        return true;
      }
    });
  }
seems to work as desired on an actual device, but on an emulator it produces inconsistent results, sometimes working, sometimes actually scrolling too far to the point where the selected item doesn't even appear on screen: https://youtu.be/93mpLVas7BY

Curiously, even though my current reviewboard commits use scrollToPosition in the linear case in this situation (i.e. scrolling to a selected position on first show), I never see any scrolling after a rotation, even with 150-200 tabs open, so I wonder if Android special cases that situation and does an immediate reposition instead of an animated scroll (because scrollToPosition is called before the view is visible)?  I haven't looked at the code yet.

On a related note, to answer my own question from commit 7, updating testing for the new layout probably actually requires a scrollBy instead of a scrollToPosition so that it does complete in one layout (I guess TwoWayView's setSelection doesn't animate (I think I was reading the wrong code the last time I looked at that :/)).  It probably only works now because we don't have more than a screen's worth of tabs open during testing.  Unfortunately it seems that scrollBy doesn't reliably work on an emulator in the grid case (as in the video linked above)...  Delaying the scrollBy for a couple seconds (as a test) helps - the selected tab hasn't appeared off screen yet after the scroll - but it's still not consistent: sometimes it appears where you expect, sometimes it scrolls a little further.  Ugh.  Maybe bug 1267884 will help :)
I'll take another look at this now that we're on a new version of the support library.

I did already find out that scrollToPosition /is/ the method you call to load a position without an animated scroll ... so apologies for the noise in comment 25 (though it does show that scrollBy is yet another scroll* not quite working right).  I'd feel a little worse about that if the docs were a little clearer - animations seem to run before/with a scrollToPosition if they're in the same layout pass, so I think that's why I had it in my head that scrollToPosition was doing an animated scroll; I should have checked more carefully though.

I've also newly noticed that in the scroll-to-position-on-initial-view-load errors that I've posted, there's no vertical padding between items in the cases where the scroll fails (which would account for the scroll appearing to go too far), so maybe there's an ItemDecoration timing issue or something.  I'll see what I can find.
Flags: needinfo?(s.kaspari)
Thanks for taking care of this (and documenting it here!), Tom. I'm currently swamped with other request and didn't have a chance to look at it.
Attachment #8795872 - Attachment is obsolete: true
Attachment #8795874 - Attachment is obsolete: true
Attachment #8795879 - Attachment is obsolete: true
It turns out RecyclerView doesn't invalidate an in-process layout when you change the span count on a GridLayoutManager, and that's why the ItemDecorations were intermittently failing to apply correctly and then causing a scrolling error.

Recall that we're setting the span count of our GridLayoutManager dynamically in onMeasure since we can't know what span count to set until we know the width of our RecyclerView.  Given that, here's an example run where things fail:

11:51:29.310  D/***SHOW: show GridLayoutManager RecyclerView
11:51:29.320  D/***ONMEASURE: getWidth: 1080
11:51:29.320  D/***SET_SPANCOUNT: 1
11:51:29.320  D/***ONMEASURE: getWidth: 1080
11:51:29.320  D/***SET_SPANCOUNT: 1
11:51:29.320  D/***setting ItemDecoration offsets for position 7
11:51:29.320  D/***setting ItemDecoration offsets for position 8
11:51:29.320  D/***setting ItemDecoration offsets for position 9
11:51:29.320  D/***setting ItemDecoration offsets for position 10
11:51:29.320  D/***setting ItemDecoration offsets for position 6
11:51:29.540  D/***ONMEASURE: getWidth: 1920
11:51:29.540  D/***SET_SPANCOUNT: 3
11:51:29.540  D/***ONMEASURE: getWidth: 1920
11:51:29.540  D/***SET_SPANCOUNT: 3
11:51:29.590  D/***ONMEASURE: getWidth: 1815
11:51:29.590  D/***SET_SPANCOUNT: 3
11:51:29.590  D/***ONMEASURE: getWidth: 1815
11:51:29.590  D/***SET_SPANCOUNT: 3
11:51:29.600  D/***setting ItemDecoration offsets for position 11
11:51:29.600  D/***setting ItemDecoration offsets for position 12

ItemViews 6-10 get the ItemDecoration offsets for the case where there's one column, ItemViews 11 and 12 get the ItemDecoration offsets for the case where there are three columns (it manifested differently in the previous videos because of some caching I was doing (but no longer am!)).

The fix is to remove and add back our ItemDecoration when we set a new span count, in order to force all new computations.
(In reply to Tom Klein from comment #39)
> It turns out RecyclerView doesn't invalidate an in-process layout when you
> change the span count on a GridLayoutManager

Sorry, that should be: RecyclerView doesn't invalidate already computed *ItemDecorations* during an in-process layout when you change the span count on a GridLayoutManager.
Attachment #8795870 - Attachment is obsolete: true
Attachment #8795870 - Flags: review?(s.kaspari)
Attachment #8795871 - Attachment is obsolete: true
Attachment #8795871 - Flags: review?(s.kaspari)
Attachment #8795873 - Attachment is obsolete: true
Attachment #8795873 - Flags: review?(s.kaspari)
Attachment #8795876 - Attachment is obsolete: true
Attachment #8795876 - Flags: review?(s.kaspari)
Attachment #8795878 - Attachment is obsolete: true
Attachment #8795878 - Flags: review?(s.kaspari)
See Also: → 1310081
Splitting off the grid work into bug 1310081.
Summary: Convert tabs tray to use a RecyclerView → Convert tabs tray list to use a RecyclerView
Comment on attachment 8789490 [details]
Bug 1116415 - 1. Create new RecyclerView and adapter base classes for tabs panels.

https://reviewboard.mozilla.org/r/77678/#review84496

LGTM.

I just tested the patch series on my device and it just works™. The swipe-to-dismiss functionality feels much better now and is consitent with other apps. Great work.
Attachment #8789490 - Flags: review?(s.kaspari) → review+
Comment on attachment 8795867 [details]
Bug 1116415 - 2. Make the linear tabs list a RecyclerView.

https://reviewboard.mozilla.org/r/81766/#review84500

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java:26
(Diff revision 3)
> -    private TabsPanel tabsPanel;
> -    private int closeAnimationCount;
>      private int closeAllAnimationCount;
> -    private int originalSize;
>  
>      public TabsListLayout(Context context, AttributeSet attrs) {

I like how small this class is now! :)

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/BaseTest.java:38
(Diff revision 3)
>  import android.support.v4.app.FragmentActivity;
>  import android.support.v4.app.FragmentManager;
> +import android.support.v7.widget.RecyclerView;
>  import android.text.TextUtils;
>  import android.util.DisplayMetrics;
> +import android.util.Log;

nit: I guess you added this for debugging. But now it is unused.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/BaseTest.java:616
(Diff revision 3)
> +    // A temporary tabs list/grid holder while the list and grid views are being transitioned to
> +    // RecyclerViews.

nit: Can you add the related bug numbers here?
Attachment #8795867 - Flags: review?(s.kaspari) → review+
Comment on attachment 8795868 [details]
Bug 1116415 - 3. Add an ItemTouchHelper to the tabs panel list for swipe to close.

https://reviewboard.mozilla.org/r/81768/#review84506

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsTouchHelperCallback.java:13
(Diff revision 3)
> +import android.graphics.Canvas;
> +import android.support.v7.widget.RecyclerView;
> +import android.support.v7.widget.helper.ItemTouchHelper;
> +import android.view.View;
> +
> +public class TabsTouchHelperCallback extends ItemTouchHelper.Callback {

nit: This class (and the interfaces in here) could be limited to package visibility (instead of 'public').

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsTouchHelperCallback.java:42
(Diff revision 3)
> +    public boolean onMove(RecyclerView recyclerView, RecyclerView.ViewHolder viewHolder,
> +                          RecyclerView.ViewHolder target) {
> +        return false;
> +    }

I'm looking forward to implementing this in the future (Especially after we have transformed TabStripView). :)

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsTouchHelperCallback.java:57
(Diff revision 3)
> +                            RecyclerView.ViewHolder viewHolder,
> +                            float dX,
> +                            float dY,
> +                            int actionState,
> +                            boolean isCurrentlyActive) {
> +        super.onChildDraw(c, recyclerView, viewHolder, dX, dY, actionState, isCurrentlyActive);

nit: Maybe we want to check if actionState is ACTION_STATE_SWIPE. Although we haven't implemented DRAG yet.

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsTouchHelperCallback.java:60
(Diff revision 3)
> +                            int actionState,
> +                            boolean isCurrentlyActive) {
> +        super.onChildDraw(c, recyclerView, viewHolder, dX, dY, actionState, isCurrentlyActive);
> +        if (distanceToAlphaMin == 0) {
> +            // Assumes all itemViews are the same width.
> +            distanceToAlphaMin = viewHolder.itemView.getWidth();

I wonder if caching this value can become a problem when using split-screen mode and the window is resized. I can try this on the tablet later.
Attachment #8795868 - Flags: review?(s.kaspari) → review+
Comment on attachment 8795869 [details]
Bug 1116415 - 4. Add an item animator to the tabs list for removes.

https://reviewboard.mozilla.org/r/81770/#review84518

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java
(Diff revision 3)
>  
>              final PropertyAnimator animator = new PropertyAnimator(ANIMATION_DURATION);
>              animator.attach(view, PropertyAnimator.Property.ALPHA, 0);
>  
>              animator.attach(view, PropertyAnimator.Property.TRANSLATION_X, view.getWidth());
> -

nit: Was this line removed intentionally?

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayoutAnimator.java:22
(Diff revision 3)
> +
> +import java.util.ArrayList;
> +import java.util.List;
> +
> +// This is a light rewrite of DefaultItemAnimator to support a non-default remove animation.
> +public class TabsListLayoutAnimator extends DefaultItemAnimator {

nit: Could be visible to classes in this package only.

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayoutAnimator.java:23
(Diff revision 3)
> +    private ArrayList<RecyclerView.ViewHolder> pendingRemovals = new ArrayList<>();
> +    // ViewHolders on which remove animations are currently being run.
> +    private ArrayList<RecyclerView.ViewHolder> removeAnimations = new ArrayList<>();

nit: Let's use the interface instead of the type:
private List<RecyclerView.ViewHolder> xxx = new ArrayList<>();
Attachment #8795869 - Flags: review?(s.kaspari) → review+
Comment on attachment 8795875 [details]
Bug 1116415 - 5. Send the index of an added tab as data.

https://reviewboard.mozilla.org/r/81782/#review84534
Attachment #8795875 - Flags: review?(s.kaspari) → review+
Comment on attachment 8795877 [details]
Bug 1116415 - 6. Provide fixed TouchDelegate class.

https://reviewboard.mozilla.org/r/81786/#review84542
Attachment #8795877 - Flags: review?(s.kaspari) → review+
All jobs passed on try. See my comments/nits above. After addressing them we can go ahead and land this. :)
Comment on attachment 8795868 [details]
Bug 1116415 - 3. Add an ItemTouchHelper to the tabs panel list for swipe to close.

https://reviewboard.mozilla.org/r/81768/#review84506

> I wonder if caching this value can become a problem when using split-screen mode and the window is resized. I can try this on the tablet later.

View.getWidth() is cheap (and likely to remain so I would think), so I just went ahead and removed the caching.
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a2aec8df69cc
1. Create new RecyclerView and adapter base classes for tabs panels. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/3ead79beb3fc
2. Make the linear tabs list a RecyclerView. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/ab8906b5313f
3. Add an ItemTouchHelper to the tabs panel list for swipe to close. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/9898a1a0f932
4. Add an item animator to the tabs list for removes. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/b640dc6b5c48
5. Send the index of an added tab as data. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/be3a568958a3
6. Provide fixed TouchDelegate class. r=sebastian
Depends on: 1350737
Verified on RC 52.0.2 the tabs tray functionality and UI and found no issues.
Devices: 
- Asus ZenPad 8.0 Z380KL (Android 6.0.1)
- Huawei Honor 5X (Android 5.1.1)
Status: RESOLVED → VERIFIED
Depends on: 1350718
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: