Closed Bug 1317446 Opened 8 years ago Closed 7 years ago

Make TabStripView a RecyclerView

Categories

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

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

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: twointofive, Assigned: twointofive)

References

Details

Attachments

(3 files)

Bug 1317446 - 1. Make TabStripView a RecyclerView.
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1317446 - 2. Add TabStripItemAnimator for TabStripView item animations.
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1317446 - 3. Add TabStripDividerItem for TabStripView dividers and spacing.
58 bytes, text/x-review-board-request
sebastian
: review+
Details 
      No description provided.
A couple notes:

*) The current TabStripView does a smoothScrollToPosition (i.e. an animated scroll) in some situations, including when a new tab is added using the "+" button in the tab strip, or when the TabStripView becomes visible after viewing the tabs panel.  When you add a tab, the animation is from the current position to the last position, and the time it takes is linear in the number of tabs you have to scroll through.  So, if you have 50 tabs open and you've scrolled to the first tab, and then tap "+" for a new tab, you have to wait a couple of seconds for the animation to run (on an emulator at least, but I expect it's basically the same on a real device).  So I've switched to just using scrollToPosition, and I think the animations look good, but if we really want the smooth animated scroll back just let me know.

*) The one issue I'm aware of that is sort of hackily resolved right now is: if you have a bunch of tabs opened and the last one is the selected tab, when you change orientation we're supposed to scroll to make sure the currently selected tab is in view.  But when you switch to portrait orientation from landscape, sometimes the scroll doesn't quite go far enough - it leaves about 1.25 tabs unscrolled, so the last tab isn't visible.  For all but the last two tabs it doesn't seem to be an issue :/  Removing the item decoration has no effect, so that's not the issue this time, and post()ing the scroll doesn't help.  Delaying that scroll for 50msec does seem to fix it though, so that's what's currently being done.  I don't see any flashing (I assume it's happening while the orientation animation is running?), but I don't have a physical tablet, so I've only been able to test on emulators.

*) Lastly, the current TabStripView uses an AnimatorListener to 'setLayerType(View.LAYER_TYPE_HARDWARE);' when it runs its remove, add, and restore animations.

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/tabs/TabStripView.java#431

With RecyclerView the remove and add animations have been moved into RecyclerView animations, which don't set the layer type (I assume), so is it really still worth keeping the AnimatorListener stuff just for the restore animation?  My preference would be to remove it, but I don't really know how much of a difference it makes.  What do you think?
Great!

I do not have access to a tablet this week. But I'll try this and review it next week! :)
Priority: -- → P3
Comment on attachment 8810564 [details]
Bug 1317446 - 1. Make TabStripView a RecyclerView.

https://reviewboard.mozilla.org/r/92808/#review92950

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabStripAdapter.java:73
(Diff revision 1)
> +            }
> +        }
> -        }
> +    }
>  
> -        return tabs.indexOf(tab);
> +    /* package */ void removeTab(Tab tab) {
> +        final int position =  getPositionForTab(tab);

nit: Two whitespaces after '='
Attachment #8810564 - Flags: review?(s.kaspari) → review+
Comment on attachment 8810565 [details]
Bug 1317446 - 2. Add TabStripItemAnimator for TabStripView item animations.

https://reviewboard.mozilla.org/r/92810/#review92996
Attachment #8810565 - Flags: review?(s.kaspari) → review+
Comment on attachment 8810566 [details]
Bug 1317446 - 3. Add TabStripDividerItem for TabStripView dividers and spacing.

https://reviewboard.mozilla.org/r/92812/#review92998
Attachment #8810566 - Flags: review?(s.kaspari) → review+
Code-wise this looks great. I can give you more feedback next week and try this on my tablets.
I just remembered that I wanted to test those patches on my tablet. Sorry! Setting NI flag to remember doing that later (Tablet is charging now!).
Flags: needinfo?(s.kaspari)
Just tested it on my N9 and it's great. I think this can land!
Flags: needinfo?(s.kaspari) → needinfo?(twointofive)
Flags: needinfo?(twointofive)
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2ccfe8d2a65a
1. Make TabStripView a RecyclerView. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/7931c003514e
2. Add TabStripItemAnimator for TabStripView item animations. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/a4eff01501e5
3. Add TabStripDividerItem for TabStripView dividers and spacing. r=sebastian
Blocks: 925108
Depends on: 1339066
Depends on: 1340929
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: