Refactor FXIOS-13499 [Trending Searches] recent searches to use A/S #29797
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,66 +4,68 @@ | |
|
|
||
| import Common | ||
| import Shared | ||
| import Storage | ||
|
|
||
| /// Abstraction for any search client that can return trending searches. Able to mock for testing. | ||
| protocol RecentSearchProvider { | ||
| func addRecentSearch(_ term: String, url: String?) | ||
| func loadRecentSearches(completion: @escaping ([String]) -> Void) | ||
| } | ||
|
|
||
| /// A provider that manages recent search terms from a user's history storage. | ||
| struct DefaultRecentSearchProvider: RecentSearchProvider { | ||
|
There was a problem hiding this comment. If you are reviewing this file, I would say to review the Raw file since it might be easier as the whole file is basically rewritten. If you are wondering why we are using There was a problem hiding this comment. So, the min is used in terms of the date timestamp. Does this mean we're fetching everything from all of time? 🤔 There was a problem hiding this comment.
No it's limited to a 1000. Anyways I didn't like how we have to get x items and have to filter it out to the limit we want. I opened mozilla/application-services#7002 to add a convenience method that returns n most recent items. This is not a blocker though. We can merge this and update later when the new method is available. @cyndichin can you please add a There was a problem hiding this comment. Thanks @issammani for opening that PR, it'll be helpful for Android too! You 🪨 ! It seems requirements may have change in that we may want to fetch all recent searches (with no limits). Still TBD, but the method looks great regardless! There was a problem hiding this comment. Updated here: d62599d |
||
| private let historyStorage: HistoryHandler | ||
| private let logger: Logger | ||
| private let nimbus: FxNimbus | ||
|
|
||
| private var maxNumberOfSuggestions: Int { | ||
| return nimbus.features.recentSearchesFeature.value().maxSuggestions | ||
| } | ||
|
|
||
| init( | ||
| historyStorage: HistoryHandler, | ||
| logger: Logger = DefaultLogger.shared, | ||
| nimbus: FxNimbus = FxNimbus.shared | ||
| ) { | ||
| self.historyStorage = historyStorage | ||
| self.logger = logger | ||
| self.nimbus = nimbus | ||
| } | ||
|
|
||
| /// Adds a search term to our history storage, `Rust Places` and saved in `places.db` locally. | ||
| /// | ||
| /// - Parameter term: The search term to store. | ||
| func addRecentSearch(_ term: String, url: String?) { | ||
| guard let url else { | ||
| logger.log("Url is needed to store recent search in history, but was nil.", | ||
| level: .debug, | ||
| category: .searchEngines) | ||
| return | ||
| } | ||
| let trimmed = term.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard !trimmed.isEmpty else { return } | ||
|
|
||
| historyStorage.noteHistoryMetadata( | ||
| for: trimmed.lowercased(), | ||
| and: url, | ||
| completion: { _ in } | ||
| ) | ||
| } | ||
|
|
||
| /// Retrieves list of search terms from our history storage, `Rust Places` and saved in `places.db` locally. | ||
| /// | ||
| /// Only care about returning the `maxNumberOfSuggestions`. | ||
| /// We don't have an interface to fetch only a certain amount, so we follow what Android does for now. | ||
| func loadRecentSearches(completion: @escaping ([String]) -> Void) { | ||
| // TODO: FXIOS-13782 Use get_most_recent method to fetch history | ||
| historyStorage.getHistoryMetadataSince(since: Int64.min) { result in | ||
| if case .success(let historyMetadata) = result { | ||
| let searches = historyMetadata.compactMap { $0.searchTerm } | ||
| let recentSearches = Array(searches.prefix(maxNumberOfSuggestions)) | ||
| completion(recentSearches) | ||
| } else { | ||
| completion([]) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,6 +117,8 @@ enum ToolbarActionType: ActionType { | |
| case clearSearch | ||
| case didDeleteSearchTerm | ||
| case didEnterSearchTerm | ||
| // User submitted a search term to load the search request | ||
| case didSubmitSearchTerm | ||
|
Comment on lines
119
to
+121
There was a problem hiding this comment. @thatswinnie I wasn't sure why we need to make this distinction, but There was a problem hiding this comment. It seems like we are just using this action in the There was a problem hiding this comment. thanks @thatswinnie ! I believe that the naming is based on where the action is called and not the consequences. Here's our wiki guidelines on that: I did write these when I was discussing with @OrlaM on Redux and think I brought it to the team in the early days. Happy to sync or update this if something has changed and I'm unaware. https://github.com/mozilla-mobile/firefox-ios/wiki/Redux-Guidelines---FAQs#faqs |
||
| case didSetSearchTerm | ||
| case didStartTyping | ||
| case translucencyDidChange | ||
|
|
||
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.
Alternatively, you could adapt the
loadRecentSearchesfunction signature to use a@MainActorcompletion. Then in theloadRecentSearchesmethod you call the completion on the main thread, and here you wouldn't need this.I just suggest it since loading the engines somewhat suggests to me that we'd perform some UI work after some networking. 😄
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 was trying to stay away from mixing completion handlers / modern swift concurrency here and we're more explicit in what we want to ensure is on main thread. However, it does seem cleaner in theory though and can't think of a case where we wouldn't want it on the main thread.
I tried to update the function signature, but I get this error, let me know if I misunderstood what you meant. Thanks!
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 can go either way then, now that I understand your goal!
In this case you'd need to Dispatch to the main queue to ensure the completion handler is always called on the main thread, that's expected actually. 👍
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.
Ah okay, I wasn't sure if there was a route without dispatching to main, lets leave this as is then. I'll need to revisit this area when I use the new method anyway.