Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tab swiping: Integration of the new tab management #5322

Merged
merged 108 commits into from
Dec 20, 2024
Merged

Tab swiping: Integration of the new tab management #5322

merged 108 commits into from
Dec 20, 2024

Conversation

0nko
Copy link
Member

@0nko 0nko commented Nov 26, 2024 •
edited by malmstein
Loading

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

  • Long press on a tabs button
  • Verify a new tab is opened

New tab from the menu

  • Tap on the New tab menu item
  • Verify a new tab is opened

New tab from tab switcher

  • Go to tab switcher
  • Tap on the New tab button
  • Verify a new tab is opened

New tab recreation

  • Open a few tabs
  • Go to the tab switcher
  • Add a new tab
  • Verify a new tab is correctly displayed
  • Go back to the tab switcher
  • Close the NTP
  • Tap on the new tab button
  • Verify the new tab is correctly opened

Swiping position

  • Open at least 3 tabs
  • Select the 1st one in the tab switcher
  • Swipe to the last one
  • Go to tab switcher and verify the correct tab is selected
  • Tap on the 2nd tab
  • Verify the correct tab is opened

New link

  • Open any website
  • Long press on a link to open in new tab
  • Verify the link is opened in a new tab

New link in the background

  • Open any website
  • Long press on a link to open in background tab
  • Verify the link is opened in a new tab in the background

New link in the background

  • Make sure the DDG app is the default browser app
  • Open an app that can open links in custom tabs (i.e. Twitter, Facebook)
  • Open a link in a custom tab
  • Verify the link opens correctly
  • Move the tab to the app
  • Verify the tab is correctly displayed within the app

Switch to tab

  • Start typing a site address that’s already opened in another tab
  • Tap on the “Switch to tab” item
  • Verify the existing tab with the link is shown instead of opening a new tab

Changing the omnibar position

  • Open a few tabs
  • Go to settings and change the omnibar position
  • The app will reload (the cached sites are cleared)
  • Swipe through the tabs and verify the load correctly

Bookmarks

  • Load a site
  • Add it to the boorkmarks
  • Open a different tab
  • Go to Bookmarks and select the saved boormark
  • Verify the bookmark loads correctly

Default site

  • Set some default URL in Settings -> General -> Show on App Launch
  • Kill the app and relaunch it
  • Verify the default site is correctly loaded in a new tab
  • Kill the app and relaunch it
  • Verify the default site is loaded in the same tab as before (no new tab is added)

0nko and others added 30 commits November 11, 2024 10:43
@0nko
Move the existing tab management functionality to a separate class
df8b2f6
@0nko
Move the tab-related calls to TabManager
3579d31
@0nko
Remove unused code
eb1c607
@0nko
Clean up code
b29d761
@0nko
Add onCleanup function
d9f13c4
@0nko
Extract a TabManager interface
a6b8d7e
@0nko
Use an injected TabManager for tab-related operations
c2b0cd2
@0nko
Fix ktlint issues
d865d18
@0nko
Rename function for easier readability
23cf2d4
@0nko
Inject the TabManager interface instead of the implementation class
a0dc5cc
@0nko
Remove unnecessary null checks
16af89e
@0nko
Add view pager to the layout and create an adapter
d3454d5
@0nko
Initialize the adapter in the tab manager
a323814
@0nko
Add method to get tab by ID
c443b8d
@0nko
Optimize the fragment initialization for the view pager use case
57016fd
@0nko
Add methods to observe the onboarding status
84ba944
@0nko
Fix the new tab creation
d1f1413
@0nko
Handle the new tab creation using a message from another tab
9b665a0
@0nko
Initialize the tab fragment when it becomes active
b581903
@0nko
Rename the tab manager impelemntation
9e4c066
@0nko
Disable tab deletion when there is just one
1402075
@0nko
Switch to the existing empty tab instead of opening a new one
a311686
@0nko
Update tab callbacks
1635f4d
@0nko
Change the onActivityCreated to onViewCreated
e92daba
@0nko
Disable viewpager swiping over the webview
c5af1d0
@0nko
Check if the tab has changed before calling the callback
2b4abae
@0nko
Clean up
cbf1df2
@0nko
Use an existing empty tab instead of opening a new one
dedd69a
@0nko
Use a flow to access the currently selected tab
24c8fa8
@0nko
Use LiveData instead of Flows
7dc5e96
@0nko
Copy link
Member Author

0nko commented Dec 18, 2024

Thanks for the review, @malmstein!

There are lots of changes in this PR that affect the code beyond the feature flag. Everything should be self-contained. The app should behave exactly as now if the feature flag is disabled. In its current state, this is not the case.

I’ll double check this.

There is not a single test for the TabManager. If you had some, you’d see that the way you’ve built it is not testable. A class injected into an Activity should not be calling the Activity’s ViewModel per direct reference.

I planned to add tests in a separate PR.

Just for some background, the philosophy of the TabManager here is not to act as a BL model, but rather as a UI-layer container for the tab-related functionality. I kept the calls to the ViewModel from TabManager to limit the changes in other parts of the app that might use these methods. But after a closer look, it seems they are called only from the TabManager. I moved them and now the ViewModel is not referenced anymore.

I’m not sure about how you check if Onboarding is complete. Please ask @nalcalag to check it. Also, please add some tests for that.

This was actually suggested by @nalcalag here.

I’ve addressed your comments and I will address the broken/missing tests next. In the meantime, the PR's ready for another round.

@0nko
Copy link
Member Author

0nko commented Dec 18, 2024 •
edited
Loading

@malmstein I checked the code and added the missing feature flag checks in all the places. You were right, it was difficult to keep track of what’s new and what’s old with all the changes so I added the old code back and wrapped it in a FF.

Take a look here to better see what’s different compared to develop.

@0nko 0nko requested a review from malmstein December 19, 2024 14:36
Base automatically changed from feature/ondrej/tab-swiping-ff-cache to feature/ondrej/swiping-tabs December 20, 2024 11:05
0nko added 2 commits December 20, 2024 12:19
@0nko
Fix opening of links in a new tab
ec49bd7
@0nko
Merge branch 'feature/ondrej/swiping-tabs' into feature/ondrej/swipin…
943f23d 
…g-tabs-ui

# Conflicts:
#	app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt
#	app/src/main/java/com/duckduckgo/app/browser/tabs/RealTabManager.kt
@0nko
Copy link
Member Author

0nko commented Dec 20, 2024

After a discussion with @malmstein & @anikiki, squashing all PRs into a single one for easier review management.

@0nko 0nko merged commit 2ee109f into feature/ondrej/swiping-tabs Dec 20, 2024
4 checks passed
@0nko 0nko deleted the feature/ondrej/swiping-tabs-ui branch December 20, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anikiki anikiki Awaiting requested review from anikiki

@malmstein malmstein Awaiting requested review from malmstein

Assignees

@malmstein malmstein

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@0nko @malmstein