Tab swiping #5408
Merged
Tab swiping #5408
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ilability.kt Co-authored-by: Noelia Alcala <[email protected]>
Task/Issue URL: https://app.asana.com/0/1202552961248957/1208659739219426/f ### Description This PR adds a feature flag that will allow us to disable the tab swiping. --------- Co-authored-by: Noelia Alcala <[email protected]>
Task/Issue URL: https://app.asana.com/0/72649045549333/1208648123066959/f ### Description This PR refactors the tab-related code into a separate `TabManager` class to ease the integration of the tab-swiping functionality. The refactoring doesn’t change the existing functionality. ### Steps to test this PR Smoke testing of tab-related functionality is sufficient (changing of tabs in the tab switcher, creating a new tab, etc.) --------- Co-authored-by: Noelia Alcala <[email protected]>
Task/Issue URL: https://app.asana.com/0/1207418217763355/1208852760335259/f ### Description This PR just caches the feature flag value. I’ve set the scope of the `IsSwipingTabsFeatureEnabled` to `AppScope` because it’s used inside the `TabManager`, which is used across 2 different activities (`BrowserActivity` and `TabSwitcherActivity`). ### Steps to test this PR Testing optional.
Task/Issue URL: https://app.asana.com/0/72649045549333/1208648123066962/f ### Description This PR includes the complete implementation of the tab-swiping functionality. ### Steps to test this PR **Make sure you’ve completed the onboarding!** #### New tab from long press - [x] Long press on a tabs button - [x] Verify a new tab is opened #### New tab from the menu - [x] Tap on the New tab menu item - [x] Verify a new tab is opened #### New tab from tab switcher - [x] Go to tab switcher - [x] Tap on the New tab button - [x] Verify a new tab is opened #### New tab recreation - [x] Open a few tabs - [x] Go to the tab switcher - [x] Add a new tab - [x] Verify a new tab is correctly displayed - [x] Go back to the tab switcher - [x] Close the NTP - [x] Tap on the new tab button - [x] Verify the new tab is correctly opened #### Swiping position - [x] Open at least 3 tabs - [x] Select the 1st one in the tab switcher - [x] Swipe to the last one - [x] Go to tab switcher and verify the correct tab is selected - [x] Tap on the 2nd tab - [x] Verify the correct tab is opened #### New link - [x] Open any website - [x] Long press on a link to open in new tab - [x] Verify the link is opened in a new tab #### New link in the background - [x] Open any website - [x] Long press on a link to open in background tab - [x] Verify the link is opened in a new tab in the background #### New link in the background - [x] Make sure the DDG app is the default browser app - [x] Open an app that can open links in custom tabs (i.e. Twitter, Facebook) - [x] Open a link in a custom tab - [x] Verify the link opens correctly - [x] Move the tab to the app - [x] Verify the tab is correctly displayed within the app #### Switch to tab - [x] Start typing a site address that’s already opened in another tab - [x] Tap on the “Switch to tab” item - [x] Verify the existing tab with the link is shown instead of opening a new tab #### Changing the omnibar position - [x] Open a few tabs - [x] Go to settings and change the omnibar position - [x] The app will reload (the cached sites are cleared) - [x] Swipe through the tabs and verify the load correctly #### Bookmarks - [x] Load a site - [x] Add it to the boorkmarks - [x] Open a different tab - [x] Go to Bookmarks and select the saved boormark - [x] Verify the bookmark loads correctly #### Default site - [x] Set some default URL in Settings -> General -> Show on App Launch - [x] Kill the app and relaunch it - [x] Verify the default site is correctly loaded in a new tab - [x] Kill the app and relaunch it - [x] Verify the default site is loaded in the same tab as before (no new tab is added)
Task/Issue URL: https://app.asana.com/0/72649045549333/1208648123066972/f ### Description This PR adds pixel for tab swiping. ### Steps to test this PR _Change tabs with swiping_ - [x] Add a few tabs - [x] Swipe tabs 3 times - [x] Verify `m_swipe_tabs_used` pixel is fired 3 times - [x] Verify `m_swipe_tabs_used_daily` pixel is fired once _Change tabs with not swiping_ - [x] Add a few tabs - [x] Long press on a tabs button to open a new tab - [x] Go to tab switch - [x] Select a different tab - [x] Add a new tab from the menu - [x] Verify `m_swipe_tabs_used` pixel is never fired
0nko
requested review from
malmstein and
anikiki
and removed request for
malmstein
…e it for new tests
# Conflicts: # app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
|
Thanks for the thorough review @anikiki! I’ve addressed your feedback — ready for another round. |
Task/Issue URL: https://app.asana.com/0/0/1208648123066970/f ### Description Adds unit tests for DefaultTabManager. ### Steps to test this PR Run the unit tests and make sure they’re passing.
anikiki
approved these changes
Jan 8, 2025
# Conflicts: # app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt # app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt # app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt # app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt
# Conflicts: # app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt
0nko
force-pushed
the
feature/ondrej/swiping-tabs
branch
from
4a370ac to
8d03786
Compare
Task/Issue URL: https://app.asana.com/0/1207418217763355/1209152058231741/f ### Description This PR removes the onboarding restriction that prevents tab swiping until onboarding is complete. It also fixes a bug that shows duplicate omnibar highlight animations. The bug is caused due to RecyclerView calling the `View.onAttachedToWindow()` every time the current item is changed, resulting in duplicate `PulseAnimation` instances. The fix involves removing `PulseAnimation` initialization from the `onAttachedToWindow()` callback, so that it’s initialized only once. I also moved the injection to the constructor to avoid repeated injection calls and refactored the ViewModel flows collection to only happen during active lifecycle states. While working on this, I discovered that edit mode wasn’t working properly and there’s a simpler way to do it, so I included it here. ### Steps to test this PR - [x] Make sure you enable the tab swiping feature in the settings and restart the app - [x] Go through onboarding until you get to the privacy shield step (the shield animation is displayed) - [x] Add a new tab - [x] Swipe back to the tab with the shield animation - [x] Notice there is only one animation - [x] Go to the tab switcher and come back - [x] Notice there is still only one animation
Task/Issue URL: https://app.asana.com/0/1207418217763355/1209252224432933/f ### Description This PR fixes the frozen typing animation in onboarding dialogs when tabs are swiped while the animation is in progress. The fix restarts the animation when a view is reattached to window. ### Steps to test this PR - [x] Clean the app storage - [x] Enable tab swiping flag and restart the app - [x] Make sure you have at least 2 tabs - [x] Start the onboarding and navigate to one of the steps (blocked trackers, for example) - [x] Wait for the typing to start and then swipe to the other tab before the animation can finish - [x] Swipe back - [x] Notice the animation is restarted - [x] Wait for the animation to finish - [x] Swipe to the other tab - [x] Swipe back - [x] Notice the typing animation is not restarted ### UI changes https://github.com/user-attachments/assets/662c6701-c4ba-4fe4-8852-b7f90d0fd1c3
# Conflicts: # app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt # app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt # app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt # app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt # app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt # app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt # app/src/test/java/com/duckduckgo/app/browser/BrowserViewModelTest.kt # app/src/test/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModelTest.kt
Comment on lines
+161
to
+172
| private var _currentTab: BrowserTabFragment? = null | ||
| private var currentTab: BrowserTabFragment? | ||
| get() { | ||
| return if (swipingTabsFeature.isEnabled) { | ||
| tabPagerAdapter.currentFragment | ||
| } else { | ||
| _currentTab | ||
| } | ||
| } | ||
| set(value) { | ||
| _currentTab = value | ||
| } |
There was a problem hiding this comment.
Hi, I am new to this code base, and I decided to read this project from BrowserActivity. I'm stuck in these lines. Since we are using the MVVM design pattern, isn't it better to move these lines to BrowserViewModel?
There was a problem hiding this comment.
This is a UI layer code that doesn't belong in the ViewModel.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Task/Issue URL: https://app.asana.com/0/1207418217763355/1209006734942815/f
Description
This PR includes the complete implementation of the tab-swiping functionality.
Steps to test this PR
Make sure you’ve completed the onboarding!
New tab from long press
New tab from the menu
New tab from tab switcher
New tab recreation
Swiping position
New link
New link in the background
New link in the background
Switch to tab
Changing the omnibar position
Bookmarks
Default site