Skip to content

Commit

Permalink
Refactor FXIOS-10381 [Unified Search] Rename SearchEngines to SearchE…
Browse files Browse the repository at this point in the history
…nginesManager (#22749)

* Minor refactoring to the existing SearchEngines class, renaming it to SearchEnginesManager to better describe its role. Updated related documentation. Moved Locale extension to its own file.

* Refactor SearchEnginesManager naming throughout the unit test suite.

* Documentation update.
  • Loading branch information
ih-codes authored Oct 25, 2024
1 parent b9c798b commit 2a44c75
Show file tree
Hide file tree
Showing 24 changed files with 213 additions and 196 deletions.
24 changes: 14 additions & 10 deletions firefox-ios/Client.xcodeproj/project.pbxproj

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/

import Foundation

extension Locale {
func possibilitiesForLanguageIdentifier() -> [String] {
var possibilities: [String] = []
let components = identifier.components(separatedBy: "-")

possibilities.append(identifier)

if components.count == 3,
let first = components.first,
let last = components.last {
possibilities.append("\(first)-\(last)")
}
if components.count >= 2,
let first = components.first {
possibilities.append("\(first)")
}

return possibilities
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ extension BrowserViewController: URLBarDelegate {
if let url = searchURL, InternalURL.isValid(url: url) {
searchURL = url
}
if let query = profile.searchEngines.queryForSearchURL(searchURL as URL?) {
if let query = profile.searchEnginesManager.queryForSearchURL(searchURL as URL?) {
return (query, true)
} else {
return (url?.absoluteString, false)
Expand Down Expand Up @@ -255,7 +255,7 @@ extension BrowserViewController: URLBarDelegate {
}

func submitSearchText(_ text: String, forTab tab: Tab) {
guard let engine = profile.searchEngines.defaultEngine,
guard let engine = profile.searchEnginesManager.defaultEngine,
let searchURL = engine.searchURLForQuery(text)
else {
DefaultLogger.shared.log("Error handling URL entry: \"\(text)\".", level: .warning, category: .tabs)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1572,12 +1572,12 @@ class BrowserViewController: UIViewController,
let searchViewModel = SearchViewModel(isPrivate: isPrivate,
isBottomSearchBar: isBottomSearchBar,
profile: profile,
model: profile.searchEngines,
model: profile.searchEnginesManager,
tabManager: tabManager)
let searchController = SearchViewController(profile: profile,
viewModel: searchViewModel,
tabManager: tabManager)
searchViewModel.searchEngines = profile.searchEngines
searchViewModel.searchEnginesManager = profile.searchEnginesManager
searchController.searchDelegate = self

let searchLoader = SearchLoader(
Expand Down Expand Up @@ -2581,7 +2581,7 @@ class BrowserViewController: UIViewController,
func openSearchNewTab(isPrivate: Bool = false, _ text: String) {
popToBVC()

guard let engine = profile.searchEngines.defaultEngine,
guard let engine = profile.searchEnginesManager.defaultEngine,
let searchURL = engine.searchURLForQuery(text)
else {
DefaultLogger.shared.log("Error handling URL entry: \"\(text)\".", level: .warning, category: .tabs)
Expand Down Expand Up @@ -2994,7 +2994,7 @@ class BrowserViewController: UIViewController,
.searchScreenState
.showSearchSugestionsView ?? false

let isSettingEnabled = profile.searchEngines.shouldShowPrivateModeSearchSuggestions
let isSettingEnabled = profile.searchEnginesManager.shouldShowPrivateModeSearchSuggestions

return featureFlagEnabled && !alwaysShowSearchSuggestionsView && !isSettingEnabled
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ class SearchViewController: SiteTableViewController,
]
)

if engine === self.viewModel.searchEngines?.quickSearchEngines.last {
if engine === self.viewModel.searchEnginesManager?.quickSearchEngines.last {
engineButton.trailingAnchor.constraint(
equalTo: searchEngineScrollViewContent.trailingAnchor
).isActive = true
Expand Down Expand Up @@ -436,7 +436,7 @@ class SearchViewController: SiteTableViewController,
searchTelemetry?.engagementType = .tap
switch SearchListSection(rawValue: indexPath.section)! {
case .searchSuggestions:
guard let defaultEngine = viewModel.searchEngines?.defaultEngine else { return }
guard let defaultEngine = viewModel.searchEnginesManager?.defaultEngine else { return }

searchTelemetry?.selectedResult = .searchSuggest
// Assume that only the default search engine can provide search suggestions.
Expand Down Expand Up @@ -525,7 +525,7 @@ class SearchViewController: SiteTableViewController,
case SearchListSection.firefoxSuggestions.rawValue:
title = .Search.SuggestSectionTitle
case SearchListSection.searchSuggestions.rawValue:
title = viewModel.searchEngines?.defaultEngine?.headerSearchTitle ?? ""
title = viewModel.searchEnginesManager?.defaultEngine?.headerSearchTitle ?? ""
default: title = ""
}

Expand Down
16 changes: 8 additions & 8 deletions firefox-ios/Client/Frontend/Browser/Search/SearchViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class SearchViewModel: FeatureFlaggable, LoaderListener {
var filteredOpenedTabs = [Tab]()
var searchHighlights = [HighlightItem]()
var firefoxSuggestions = [RustFirefoxSuggestion]()
let model: SearchEngines
let model: SearchEnginesManager
var suggestions: [String]? = []
static var userAgent: String?
var searchFeature: FeatureHolder<Search>
Expand Down Expand Up @@ -52,22 +52,22 @@ class SearchViewModel: FeatureFlaggable, LoaderListener {
}

var quickSearchEngines: [OpenSearchEngine] {
guard let defaultEngine = searchEngines?.defaultEngine else { return [] }
guard let defaultEngine = searchEnginesManager?.defaultEngine else { return [] }

var engines = searchEngines?.quickSearchEngines
var engines = searchEnginesManager?.quickSearchEngines

// If we're not showing search suggestions, the default search engine won't be visible
// at the top of the table. Show it with the others in the bottom search bar.
if !(searchEngines?.shouldShowSearchSuggestions ?? false) {
if !(searchEnginesManager?.shouldShowSearchSuggestions ?? false) {
engines?.insert(defaultEngine, at: 0)
}

return engines!
}

var searchEngines: SearchEngines? {
var searchEnginesManager: SearchEnginesManager? {
didSet {
guard let defaultEngine = searchEngines?.defaultEngine else { return }
guard let defaultEngine = searchEnginesManager?.defaultEngine else { return }

suggestClient?.cancelPendingRequest()

Expand Down Expand Up @@ -95,7 +95,7 @@ class SearchViewModel: FeatureFlaggable, LoaderListener {

/// Whether to show suggestions from the search engine.
var shouldShowSearchEngineSuggestions: Bool {
return searchEngines?.shouldShowSearchSuggestions ?? false
return searchEnginesManager?.shouldShowSearchSuggestions ?? false
}

var shouldShowSyncedTabsSuggestions: Bool {
Expand Down Expand Up @@ -146,7 +146,7 @@ class SearchViewModel: FeatureFlaggable, LoaderListener {

init(isPrivate: Bool, isBottomSearchBar: Bool,
profile: Profile,
model: SearchEngines,
model: SearchEnginesManager,
tabManager: TabManager,
featureConfig: FeatureHolder<Search> = FxNimbus.shared.features.search,
highlightManager: HistoryHighlightsManagerProtocol = HistoryHighlightsManager()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ class DefaultSearchEngineProvider: SearchEngineProvider {
// might not work to change the default.
guard let orderedEngineNames = orderedEngineNames else {
// We haven't persisted the engine order, so return whatever order we got from disk.

DispatchQueue.main.async {
completion(unorderedEngines)
}
Expand Down Expand Up @@ -83,13 +82,11 @@ class DefaultSearchEngineProvider: SearchEngineProvider {
with: pluginDirectory.appendingPathComponent("list.json")
) else {
logger.log("Failed to parse List.json", level: .fatal, category: .setup)
// swiftlint:disable line_length
fatalError("We are unable to populate search engines for this locale because list.json could not be parsed.")
// swiftlint:enable line_length
}
let possibilities = possibleLanguageIdentifier
let engineNames = defaultSearchPrefs.visibleDefaultEngines(for: possibilities, and: region)
let defaultEngineName = defaultSearchPrefs.searchDefault(for: possibilities, and: region)

let engineNames = defaultSearchPrefs.visibleDefaultEngines(for: possibleLanguageIdentifier, and: region)
let defaultEngineName = defaultSearchPrefs.searchDefault(for: possibleLanguageIdentifier, and: region)

guard !engineNames.isEmpty else {
logger.log("No search engines.", level: .fatal, category: .setup)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,26 @@ protocol SearchEngineDelegate: AnyObject {
func searchEnginesDidUpdate()
}

/// Manage a set of Open Search engines.
/// Manages a set of `OpenSearchEngine`s.
///
/// The search engines are ordered.
/// The search engines are ordered and can be enabled and disabled by the user. Order and disabled state are backed by a
/// write-through cache into a Prefs instance (i.e. UserDefaults).
///
/// Individual search engines can be enabled and disabled.
/// Default search engines are localized and given by the `SearchEngineProvider` (from list.json). The user may add
/// additional custom search engines. Custom search engines entered by the user are saved to a file.
///
/// The first search engine is distinguished and labeled the "default" search engine; it can never be
/// disabled. Search suggestions should always be sourced from the default search engine.
///
/// Two additional bits of information are maintained: whether search suggestions are enabled and whether
/// search suggestions in private mode is disabled
/// The first search engine is distinguished and labeled the "default" search engine; it can never be disabled.
/// [FIXME FXIOS-10187 this will change soon ->] Search suggestions should always be sourced from the default search engine
///
/// Consumers will almost always use `defaultEngine` if they want a single search engine, and
/// `quickSearchEngines()` if they want a list of enabled quick search engines (possibly empty,
/// since the default engine is never included in the list of enabled quick search engines, and
/// it is possible to disable every non-default quick search engine).
/// Two additional bits of information are maintained: whether search suggestions are enabled and whether search suggestions
/// in private mode are disabled.
///
/// The search engines are backed by a write-through cache into a ProfilePrefs instance. This class
/// is not thread-safe -- you should only access it on a single thread (usually, the main thread)!
class SearchEngines {
/// Consumers will almost always use `defaultEngine` if they want a single search engine, and `quickSearchEngines()` if they
/// want a list of enabled quick search engines (possibly empty, since the default engine is never included in the list of
/// enabled quick search engines, and it is possible to disable every non-default quick search engine).
///
/// This class is not thread-safe -- you should only access it on a single thread (usually, the main thread)!
class SearchEnginesManager {
private let prefs: Prefs
private let fileAccessor: FileAccessor
private let orderedEngineNames = "search.orderedEngineNames"
Expand Down Expand Up @@ -116,6 +116,9 @@ class SearchEngines {
}
}

/// The subset of search engines that are enabled and not the default search engine.
///
/// The results can be empty if the user disables all search engines besides the default (which can't be disabled).
var quickSearchEngines: [OpenSearchEngine]! {
return self.orderedEngines.filter({ (engine) in !self.isEngineDefault(engine) && self.isEngineEnabled(engine) })
}
Expand Down Expand Up @@ -302,20 +305,3 @@ class SearchEngines {
}
}
}

extension Locale {
func possibilitiesForLanguageIdentifier() -> [String] {
var possibilities: [String] = []
let languageIdentifier = self.identifier
let components = languageIdentifier.components(separatedBy: "-")
possibilities.append(languageIdentifier)

if components.count == 3, let first = components.first, let last = components.last {
possibilities.append("\(first)-\(last)")
}
if components.count >= 2, let first = components.first {
possibilities.append("\(first)")
}
return possibilities
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class AddressToolbarContainerModel: Equatable {
let borderPosition: AddressToolbarBorderPosition?
let searchEngineName: String?
let searchEngineImage: UIImage?
let searchEngines: SearchEngines
let searchEnginesManager: SearchEnginesManager
let lockIconImageName: String?
let safeListedURLImageName: String?
let url: URL?
Expand All @@ -30,7 +30,7 @@ class AddressToolbarContainerModel: Equatable {
let windowUUID: UUID

var addressToolbarState: AddressToolbarState {
let term = searchTerm ?? searchTermFromURL(url, searchEngines: searchEngines)
let term = searchTerm ?? searchTermFromURL(url, searchEnginesManager: searchEnginesManager)

var droppableUrl: URL?
if let url, !InternalURL.isValid(url: url) {
Expand Down Expand Up @@ -92,9 +92,9 @@ class AddressToolbarContainerModel: Equatable {
isShowingTopTabs: state.isShowingTopTabs,
windowUUID: windowUUID)
self.windowUUID = windowUUID
self.searchEngineName = profile.searchEngines.defaultEngine?.shortName
self.searchEngineImage = profile.searchEngines.defaultEngine?.image
self.searchEngines = profile.searchEngines
self.searchEngineName = profile.searchEnginesManager.defaultEngine?.shortName
self.searchEngineImage = profile.searchEnginesManager.defaultEngine?.image
self.searchEnginesManager = profile.searchEnginesManager
self.lockIconImageName = state.addressToolbar.lockIconImageName
self.safeListedURLImageName = state.addressToolbar.safeListedURLImageName
self.url = state.addressToolbar.url
Expand All @@ -108,14 +108,14 @@ class AddressToolbarContainerModel: Equatable {
self.canShowNavigationHint = state.canShowNavigationHint
}

func searchTermFromURL(_ url: URL?, searchEngines: SearchEngines) -> String? {
func searchTermFromURL(_ url: URL?, searchEnginesManager: SearchEnginesManager) -> String? {
var searchURL: URL? = url

if let url = searchURL, InternalURL.isValid(url: url) {
searchURL = url
}

guard let query = searchEngines.queryForSearchURL(searchURL) else { return nil }
guard let query = searchEnginesManager.queryForSearchURL(searchURL) else { return nil }
return query
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ class TopSitesDataAdaptorImplementation: TopSitesDataAdaptor, FeatureFlaggable {
if sponsoredTileSpaces > 0 {
sites.addSponsoredTiles(sponsoredTileSpaces: sponsoredTileSpaces,
contiles: contiles,
defaultSearchEngine: profile.searchEngines.defaultEngine)
defaultSearchEngine: profile.searchEnginesManager.defaultEngine)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ extension BookmarkItemData: BookmarksFolderCell {
// (e.g., "http://foo.com/bar/?query=%s"), and this will get them the same behavior as if
// they'd copied and pasted into the URL bar.
// See BrowserViewController.urlBar:didSubmitText:.
guard let url = URIFixup.getURL(url) ?? profile.searchEngines.defaultEngine?.searchURLForQuery(url) else {
guard let url = URIFixup.getURL(url) ?? profile.searchEnginesManager.defaultEngine?.searchURLForQuery(url) else {
logger.log("Invalid URL, and couldn't generate a search URL for it.",
level: .warning,
category: .library)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class CustomSearchViewController: SettingsTableViewController {
do {
let engine = try await createEngine(query: trimmedQuery, name: trimmedTitle)
self.spinnerView.stopAnimating()
self.profile.searchEngines.addSearchEngine(engine)
self.profile.searchEnginesManager.addSearchEngine(engine)

CATransaction.begin() // Use transaction to call callback after animation has been completed
CATransaction.setCompletionBlock(self.successCallback)
Expand Down Expand Up @@ -124,7 +124,7 @@ class CustomSearchViewController: SettingsTableViewController {
}

private func engineExists(name: String, template: String) -> Bool {
return profile.searchEngines.orderedEngines.contains { (engine) -> Bool in
return profile.searchEnginesManager.orderedEngines.contains { (engine) -> Bool in
return engine.shortName == name || engine.searchTemplate == template
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class SearchSetting: Setting {

override var status: NSAttributedString {
return NSAttributedString(
string: profile.searchEngines.defaultEngine?.shortName ?? ""
string: profile.searchEnginesManager.defaultEngine?.shortName ?? ""
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ final class SearchSettingsTableViewController: ThemedTableViewController, Featur
}

private let profile: Profile
private let model: SearchEngines
private let model: SearchEnginesManager

var shouldHidePrivateModeFirefoxSuggestSetting: Bool {
return !model.shouldShowBookmarksSuggestions &&
Expand Down Expand Up @@ -82,7 +82,7 @@ final class SearchSettingsTableViewController: ThemedTableViewController, Featur

init(profile: Profile, windowUUID: WindowUUID) {
self.profile = profile
model = profile.searchEngines
model = profile.searchEnginesManager

super.init(windowUUID: windowUUID)
}
Expand Down
Loading

0 comments on commit 2a44c75

Please sign in to comment.