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

fix showing current tab in suggestions #3562

Merged
merged 7 commits into from
Nov 12, 2024
Merged

fix showing current tab in suggestions #3562

merged 7 commits into from
Nov 12, 2024

Conversation

brindy
Copy link
Contributor

@brindy brindy commented Nov 11, 2024 •
edited
Loading

Task/Issue URL: https://app.asana.com/0/392891325557410/1208712038329496/f
Tech Design URL:
CC:

Description:
Fixes bug showing open tab for current tab when typing in suggestions.

Steps to test this PR:

  1. Go to https://news.ycombinator.com/ and open a bunch of pages in new tabs
  2. Type in to the address for one of the pages - should show as an open tab. Select it and switch to tab.
  3. Type into the address bar for the same page - should only show as history
  4. Validate that the same page open in multiple tabs only shows as a single 'switch to tab' suggestion and does not show at all when on a page with the same URL

@brindy brindy changed the title Brindy/fix showing current tab fix showing current tab in suggestions Nov 11, 2024
alessandroboron
alessandroboron approved these changes Nov 11, 2024
View reviewed changes
Copy link
Contributor

@alessandroboron alessandroboron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected! Perhaps we could add a test for AutocompleteViewController -> candidateOpenTabs to avoid regressions?

@brindy
Copy link
Contributor Author

brindy commented Nov 11, 2024

Yeah you're right. I'm gonna extract that extension into its own testable class. Bear with me.

@brindy
extract data source into own file and add tests
6cd99aa
@brindy
Merge branch 'main' into brindy/fix-showing-current-tab
2712375 
# Conflicts:
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
brindy
brindy commented Nov 11, 2024
View reviewed changes
DuckDuckGoTests/MockFeatureFlagger.swift
var enabledFeatureFlags: [FeatureFlag] = []
var enabledFeatureFlag: FeatureFlag?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is unused by tests and not part of the protocol

alessandroboron
alessandroboron approved these changes Nov 11, 2024
View reviewed changes
Copy link
Contributor

@alessandroboron alessandroboron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested again and works! Just left a “nip” comment. Thanks for addressing the tests.

DuckDuckGo/AutocompleteSuggestionsDataSource.swift
}

func history(for suggestionLoading: Suggestions.SuggestionLoading) -> [HistorySuggestion] {
return historyCoordinator.history ?? []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nip: can remove unnecessary return on line. Same applies to below methods:

  • func bookmarks(for suggestionLoading: Suggestions.SuggestionLoading) -> [Suggestions.Bookmark]

  • func internalPages(for suggestionLoading: Suggestions.SuggestionLoading) -> [Suggestions.InternalPage]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna leave it. My thinking is that functions should return something (unless they are view builders). I would like to think I'm consistent about that ... but who knows. :) Anyway, since it's just a nit I'm gonna go with what I've got.

@brindy brindy merged commit 4ed25d1 into main Nov 12, 2024
13 checks passed
@brindy brindy deleted the brindy/fix-showing-current-tab branch November 12, 2024 09:52
samsymons added a commit that referenced this pull request Nov 13, 2024
@samsymons
Merge branch 'main' into dominik/xcode-16
7f9ac0e 
* main:
  Release 7.145.0-2 (#3570)
  Fix favorites grid layout issues on New Tab Page (#3568)
  Import Add To Dock Translations (#3566)
  Release 7.145.0-1 (#3567)
  Remove Old onboarding intro (#3554)
  fix showing current tab in suggestions (#3562)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alessandroboron alessandroboron alessandroboron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brindy @alessandroboron