From 7512f4c72b2a688171edaa9f97d7825745a5c2dc Mon Sep 17 00:00:00 2001 From: Mohamed Afifi Date: Mon, 18 Dec 2023 20:13:47 -0500 Subject: [PATCH] Search was broken due to concurrent search requests (#608) This change ensures only one search request executes at a time. Additionally, the change ensures proper search lifecycle handling. --- Features/SearchFeature/SearchTypes.swift | 29 ++--- Features/SearchFeature/SearchView.swift | 117 ++++++++++-------- .../SearchFeature/SearchViewController.swift | 45 ++++--- Features/SearchFeature/SearchViewModel.swift | 103 ++++++++------- 4 files changed, 164 insertions(+), 130 deletions(-) diff --git a/Features/SearchFeature/SearchTypes.swift b/Features/SearchFeature/SearchTypes.swift index e9883892..ec9a4570 100644 --- a/Features/SearchFeature/SearchTypes.swift +++ b/Features/SearchFeature/SearchTypes.swift @@ -6,30 +6,19 @@ // import Foundation +import QuranText enum SearchUIState { case entry - case autocomplete - case loading - case searchResults + case search(_ term: String) } -enum SearchTerm { - case autocomplete(_ term: String) - case noAction(_ term: String) - - // MARK: Internal - - var term: String { - switch self { - case .autocomplete(let term), .noAction(let term): return term - } - } +enum SearchState { + case searching + case searchResult(_ results: [SearchResults]) +} - var isAutocomplete: Bool { - switch self { - case .autocomplete: return true - case .noAction: return false - } - } +enum KeyboardState { + case open + case closed } diff --git a/Features/SearchFeature/SearchView.swift b/Features/SearchFeature/SearchView.swift index 6cf68aa3..f093150b 100644 --- a/Features/SearchFeature/SearchView.swift +++ b/Features/SearchFeature/SearchView.swift @@ -18,12 +18,12 @@ struct SearchView: View { var body: some View { SearchViewUI( error: $viewModel.error, - state: viewModel.uiState, - term: viewModel.searchTerm.term, + uiState: viewModel.uiState, + searchState: viewModel.searchState, + term: viewModel.searchTerm, recents: viewModel.recents, populars: viewModel.populars, autocompletions: viewModel.autocompletions, - searchResults: viewModel.searchResults, start: { await viewModel.start() }, search: { viewModel.search(for: $0) }, selectSearchResult: { viewModel.select(searchResult: $0.result, source: $0.source) } @@ -34,13 +34,13 @@ struct SearchView: View { private struct SearchViewUI: View { @Binding var error: Error? - let state: SearchUIState + let uiState: SearchUIState + let searchState: SearchState let term: String let recents: [String] let populars: [String] let autocompletions: [String] - let searchResults: [SearchResults] let start: AsyncAction let search: AsyncItemAction @@ -48,46 +48,26 @@ private struct SearchViewUI: View { var body: some View { Group { - switch state { + switch uiState { case .entry: - entry - case .autocomplete: - autocompletionsView - case .loading: - NoorList { - LoadingView() + if autocompletions.isEmpty { + entry + } else { + autocompletionsView } - case .searchResults: - searchResultsView - } - } - .task(start) - .errorAlert(error: $error) - } - - @ViewBuilder - var searchResultsView: some View { - if !searchResults.isEmpty { - NoorList { - ForEach(searchResults) { result in - let plainTitle = title(of: result) - let title = lFormat("search.result.count", plainTitle, result.items.count) - NoorSection(title: title, result.items) { item in - let localizedVerse = item.ayah.localizedName - let arabicSuraName = item.ayah.sura.arabicSuraName - NoorListItem( - subheading: "\(localizedVerse) \(sura: arabicSuraName)", - title: searchResultText(of: item), - accessory: .text(NumberFormatter.shared.format(item.ayah.page.pageNumber)) - ) { - selectSearchResult((item, result.source)) - } + case .search: + switch searchState { + case .searching: + NoorList { + LoadingView() } + case .searchResult(let results): + searchResultsView(searchResults: results) } } - } else { - noResultsData } + .task(start) + .errorAlert(error: $error) } var noResultsData: some View { @@ -135,6 +115,31 @@ private struct SearchViewUI: View { } } + @ViewBuilder + func searchResultsView(searchResults: [SearchResults]) -> some View { + if !searchResults.isEmpty { + NoorList { + ForEach(searchResults) { result in + let plainTitle = title(of: result) + let title = lFormat("search.result.count", plainTitle, result.items.count) + NoorSection(title: title, result.items) { item in + let localizedVerse = item.ayah.localizedName + let arabicSuraName = item.ayah.sura.arabicSuraName + NoorListItem( + subheading: "\(localizedVerse) \(sura: arabicSuraName)", + title: searchResultText(of: item), + accessory: .text(NumberFormatter.shared.format(item.ayah.page.pageNumber)) + ) { + selectSearchResult((item, result.source)) + } + } + } + } + } else { + noResultsData + } + } + func title(of searchResults: SearchResults) -> String { switch searchResults.source { case .translation(let translation): @@ -185,20 +190,21 @@ struct SearchView_Previews: PreviewProvider { ), ]) - @State var results = [populatedResults] - @State var state = SearchUIState.searchResults + @State var uiState = SearchUIState.search("abc") + @State var searchState = SearchState.searching @State var error: Error? + @State var autocompletions: [String] = [] var body: some View { NavigationView { SearchViewUI( error: $error, - state: state, + uiState: uiState, + searchState: searchState, term: "is", recents: ["Recent 1", "Recent 2"], populars: ["Popular 1", "Popular 2"], - autocompletions: [Self.englishText, Self.arabicText], - searchResults: results, + autocompletions: autocompletions, start: { }, search: { _ in }, selectSearchResult: { _ in } @@ -207,17 +213,26 @@ struct SearchView_Previews: PreviewProvider { .toolbar { ScrollView(.horizontal) { HStack { - Button("Entry") { state = .entry } - Button("Autocomplete") { state = .autocomplete } + Button("Entry") { + uiState = .entry + autocompletions = [] + } + Button("Autocomplete") { + uiState = .entry + autocompletions = [Self.englishText, Self.arabicText] + } Button("Search") { - state = .searchResults - results = [Self.populatedResults] + uiState = .search("abc") + searchState = .searchResult([Self.populatedResults]) } Button("No Results") { - state = .searchResults - results = [] + uiState = .search("abc") + searchState = .searchResult([]) + } + Button("Loading") { + uiState = .search("abc") + searchState = .searching } - Button("Loading") { state = .loading } Button("Error") { error = URLError(.notConnectedToInternet) } } } diff --git a/Features/SearchFeature/SearchViewController.swift b/Features/SearchFeature/SearchViewController.swift index 0c5abc93..3eb95da7 100644 --- a/Features/SearchFeature/SearchViewController.swift +++ b/Features/SearchFeature/SearchViewController.swift @@ -9,8 +9,9 @@ import Combine import Foundation import Localization import SwiftUI +import VLogging -final class SearchViewController: UIViewController, UISearchResultsUpdating, UISearchBarDelegate { +final class SearchViewController: UIViewController, UISearchBarDelegate { // MARK: Lifecycle init(viewModel: SearchViewModel) { @@ -44,43 +45,59 @@ final class SearchViewController: UIViewController, UISearchResultsUpdating, UIS searchController.obscuresBackgroundDuringPresentation = false searchController.hidesNavigationBarDuringPresentation = true searchController.searchBar.placeholder = l("search.placeholder.text") - searchController.searchResultsUpdater = self searchController.searchBar.delegate = self navigationItem.searchController = searchController navigationItem.hidesSearchBarWhenScrolling = false definesPresentationContext = true viewModel.$searchTerm - .map(\.term) .receive(on: DispatchQueue.main) .sink { [weak self] searchTerm in - if searchTerm != self?.searchController.searchBar.text { - self?.searchController.searchBar.text = searchTerm - } + self?.searchController.searchBar.text = searchTerm } .store(in: &cancellables) - viewModel.resignSearchBar + viewModel.$keyboardState + .removeDuplicates() .receive(on: DispatchQueue.main) - .sink { [weak self] in - self?.searchController.searchBar.endEditing(true) + .sink { [weak self] state in + switch state { + case .closed: + self?.searchController.searchBar.endEditing(true) + case .open: + self?.searchController.searchBar.becomeFirstResponder() + } } .store(in: &cancellables) } // MARK: - Search delegate methods - func updateSearchResults(for searchController: UISearchController) { - let term = searchController.searchBar.text ?? "" - if viewModel.searchTerm.term != term { - viewModel.searchTerm = .autocomplete(term) - } + func searchBar(_ searchBar: UISearchBar, textDidChange searchText: String) { + logger.info("[Search] textDidChange to \(searchText)") + viewModel.autocomplete(searchText) + } + + func searchBarTextDidBeginEditing(_ searchBar: UISearchBar) { + logger.info("[Search] searchBarTextDidBeginEditing \(searchBar.text ?? "")") + viewModel.keyboardState = .open + } + + func searchBarTextDidEndEditing(_ searchBar: UISearchBar) { + logger.info("[Search] searchBarTextDidEndEditing \(searchBar.text ?? "")") + viewModel.keyboardState = .closed } func searchBarSearchButtonClicked(_ searchBar: UISearchBar) { + logger.info("[Search] searchBarSearchButtonClicked \(searchBar.text ?? "")") viewModel.searchForUserTypedTerm() } + func searchBarCancelButtonClicked(_ searchBar: UISearchBar) { + logger.info("[Search] searchBarCancelButtonClicked") + viewModel.reset() + } + // MARK: Private private var cancellables: Set = [] diff --git a/Features/SearchFeature/SearchViewModel.swift b/Features/SearchFeature/SearchViewModel.swift index 59acc9b6..a7ef666f 100644 --- a/Features/SearchFeature/SearchViewModel.swift +++ b/Features/SearchFeature/SearchViewModel.swift @@ -29,14 +29,19 @@ final class SearchViewModel: ObservableObject { @Published var error: Error? = nil - @Published var uiState = SearchUIState.entry + @Published var searchState = SearchState.searching - @Published var searchTerm = SearchTerm.noAction("") + @Published var searchTerm = "" @Published var autocompletions: [String] = [] - @Published var searchResults: [SearchResults] = [] @Published var recents: [String] = [] - let resignSearchBar = PassthroughSubject() + @Published var keyboardState: KeyboardState = .closed + + @Published var uiState = SearchUIState.entry { + didSet { + logger.debug("[Search] New UI state: \(uiState)") + } + } var populars: [String] { recentsService.popularTerms } @@ -44,7 +49,8 @@ final class SearchViewModel: ObservableObject { async let reading: () = observeReadingChanges() async let autocomplete: () = observeSearchTermChanges() async let recents: () = observeRecentSearchItemsChanges() - _ = await [reading, autocomplete, recents] + async let search: () = observeSearchChanges() + _ = await [reading, autocomplete, recents, search] } func select(searchResult: SearchResult, source: SearchResults.Source) { @@ -66,17 +72,27 @@ final class SearchViewModel: ObservableObject { navigateTo(searchResult.ayah) } + func reset() { + uiState = .entry + searchTerm = "" + autocompletions = [] + } + + func autocomplete(_ term: String) { + if searchTerm != term { + uiState = .entry + searchTerm = term + } + } + func searchForUserTypedTerm() { - search(for: searchTerm.term) + search(for: searchTerm) } func search(for term: String) { - resignSearchBar.send() - searchTerm = .noAction(term) - - Task { - await search() - } + keyboardState = .closed + searchTerm = term + uiState = .search(term) } // MARK: Private @@ -90,29 +106,36 @@ final class SearchViewModel: ObservableObject { private let contentStatePreferences = QuranContentStatePreferences.shared private let selectedTranslationsPreferences = SelectedTranslationsPreferences.shared - private func search() async { - uiState = .loading + private func search(for term: String) async throws -> [SearchResults] { + searchState = .searching + let quran = readingPreferences.reading.quran + let results = try await searchService.search(for: term, quran: quran) - let term = searchTerm.term - do { - let quran = readingPreferences.reading.quran - let results = try await searchService.search(for: term, quran: quran) - analytics.searching(for: term, results: results) + analytics.searching(for: term, results: results) + recentsService.addToRecents(term) - if searchTerm.term == term { - searchResults = results - recentsService.addToRecents(term) + return results + } - uiState = .searchResults - } - } catch { - logger.error("Error while searching. Error: \(error)") - if searchTerm.term != term { - return + private func observeSearchChanges() async { + let states = $uiState.values() + for await state in states { + switch state { + case .entry: + continue + case .search(let term): + searchState = .searching + let result = await Result(catching: { try await search(for: term) }) + if searchTerm == term { + switch result { + case .success(let results): + searchState = .searchResult(results) + case .failure(let error): + self.error = error + searchState = .searchResult([]) + } + } } - searchResults = [] - self.error = error - uiState = .searchResults } } @@ -129,30 +152,20 @@ final class SearchViewModel: ObservableObject { let readings = readingPreferences.$reading .values() for await _ in readings { - searchTerm = .noAction("") + searchTerm = "" } } private func observeSearchTermChanges() async { let searchTermSequence = $searchTerm - .dropFirst() - .filter(\.isAutocomplete) - .map(\.term) + .dropFirst() // Drop initial empty value. .throttle(for: .milliseconds(300), scheduler: DispatchQueue.main, latest: true) .values() for await term in searchTermSequence { - if uiState == .loading { - continue - } - + logger.debug("[Search] Autocomplete requested for \(term)") let autocompletions = await autocomplete(term) - if searchTerm.term == term { + if searchTerm == term { self.autocompletions = autocompletions - if !autocompletions.isEmpty { - uiState = .autocomplete - } else { - uiState = .entry - } } } }