Add tabs to suggestions/autocomplete on iOS #998
Conversation
… case the incoming query
Sources/Suggestions/Score.swift
Outdated
| @@ -39,7 +40,9 @@ extension Score { | |||
| score += 300 | |||
| // Prioritize root URLs most | |||
| if url.isRoot { score += 2000 } | |||
| } else if lowercasedTitle.starts(with: query) { | |||
| } else if lowercasedTitle | |||
| .trimmingCharacters(in: .alphanumerics.inverted) | |||
There was a problem hiding this comment.
This fixes a bug where if a title was like "Cats and Dogs" Cats would not get matched at all because of the quote.
There was a problem hiding this comment.
Does it mean that matching these characters won't work from now on?
What if I want to find a title starting with a special character?
I checked in the currently released version and I can match a title with quotes which might be a valid use case.
There was a problem hiding this comment.
Yeah that example won't work now because:
- It won't match the first character any more
- The second character wouldn't match anyway
But honestly, I think that's gonna be rare? I assume people are more likely to search for HBO than " or "HBO
I can make it an OR condition though if you feel strongly?
There was a problem hiding this comment.
The new solution looks better to me 👍 Thank you, Brindy!
| @@ -26,6 +26,7 @@ extension Score { | |||
| // swiftlint:disable:next cyclomatic_complexity | |||
| init(title: String?, url: URL, visitCount: Int, query: Query, queryTokens: [Query]? = nil) { | |||
| // To optimize, query tokens can be precomputed | |||
| let query = query.lowercased() | |||
There was a problem hiding this comment.
Probably not strictly required because usually input will be lowercase, but while testing caused some confusion.
| @@ -104,12 +111,9 @@ extension Score { | |||
| } | |||
|
|
|||
| static func tokens(from query: Query) -> [Query] { | |||
| return query | |||
| .split(whereSeparator: { | |||
| $0.unicodeScalars.contains(where: { CharacterSet.whitespacesAndNewlines.contains($0) }) | |||
There was a problem hiding this comment.
This seemed superfluous and appeared to result in some strings not tokenising as expected, so this is aligned with the similar code for Bookmarks on iOS.
| let mergedSuggestions = merge(navigationalSuggestions, maximum: maximumOfNavigationalSuggestions) | ||
| let expandedSuggestions = replaceHistoryWithBookmarksAndTabs(navigationalSuggestions) | ||
|
|
||
| let dedupedNavigationalSuggestions = Array(dedupSuggestions(expandedSuggestions).prefix(maximumOfNavigationalSuggestions)) |
There was a problem hiding this comment.
Should behave the same as before but split out to make reasoning a bit easier. Effectively history items are replaced by the first matching bookmark and/or tab that is found, but otherwise the list is not changed. Then we run a deduplication across the whole list.
| for (i, suggestion) in suggestions.enumerated() { | ||
| guard i <= Self.maximumNumberOfTopHits else { break } | ||
| for suggestion in suggestions { | ||
| guard topHits.count < Self.maximumNumberOfTopHits else { break } |
Sources/Suggestions/Score.swift
Outdated
| @@ -39,7 +40,9 @@ extension Score { | |||
| score += 300 | |||
| // Prioritize root URLs most | |||
| if url.isRoot { score += 2000 } | |||
| } else if lowercasedTitle.starts(with: query) { | |||
| } else if lowercasedTitle | |||
| .trimmingCharacters(in: .alphanumerics.inverted) | |||
There was a problem hiding this comment.
Does it mean that matching these characters won't work from now on?
What if I want to find a title starting with a special character?
I checked in the currently released version and I can match a title with quotes which might be a valid use case.
Required:
Task/Issue URL: https://app.asana.com/0/392891325557410/1208036752520497/f
iOS PR: duckduckgo/iOS#3371
macOS PR: duckduckgo/macos-browser#3309
What kind of version bump will this require?: Major
Optional:
Tech Design URL: https://app.asana.com/0/481882893211075/1208270496320813/f
CC:
Description:
Adds support for tabs to suggestions.
Steps to test this PR: