-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Issue mozilla-mobile#20637 - Fixed bug causing the tabs tray to scroll to the wrong tab when opened #21218
Conversation
… tab when opened
/** | ||
* The maximum time from when a tab was created or accessed until it is considered "inactive". | ||
*/ | ||
val maxActiveTime = TimeUnit.DAYS.toMillis(DEFAULT_ACTIVE_DAYS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wasn't sure where to refactor/relocate this to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, this is fine. I think we need to eventually move it somewhere else when the next set of settings land.
@@ -53,7 +55,7 @@ class TrayPagerAdapter( | |||
itemView, | |||
store, | |||
interactor, | |||
browserStore.state.normalTabs.indexOf(selectedTab) | |||
browserStore.state.normalTabs.filter { it.isNormalTabActive(maxActiveTime) }.indexOf(selectedTab) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have a single RecyclerView with multiple adapters, our "index" calculation is going to be a bit complicated.
From our last meeting, we need to scroll to the selected tab even if it's in the inactive tab section as well.
So we would need to do something like:
- Iterate through all adapters
- Find the adapter with a tab of the matching ID and viewholder type.
- Convert the relative adapter positive to the final adapter position.
- Tell the RV to scroll to this new position (because we need to say
RecyclerView.scrollToPosition
and there is noRecyclerView.Adapter.scrollToPosition
).
This will also aid in solving a scrolling regression when we land other adapters like in 21177.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke offline with Noah, we'll land this change for now and fix the scrolling problem when we land 21177 as well.
@@ -53,7 +55,7 @@ class TrayPagerAdapter( | |||
itemView, | |||
store, | |||
interactor, | |||
browserStore.state.normalTabs.indexOf(selectedTab) | |||
browserStore.state.normalTabs.filter { it.isNormalTabActive(maxActiveTime) }.indexOf(selectedTab) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke offline with Noah, we'll land this change for now and fix the scrolling problem when we land 21177 as well.
Fixes #20637
Pull Request checklist
To download an APK when reviewing a PR: