From 0235793bc6a3c857ed27a57f6b558549aabaa7a7 Mon Sep 17 00:00:00 2001 From: Joe Kribs Date: Sat, 15 Feb 2025 15:05:34 -0700 Subject: [PATCH] [iOS & tvOS] FilterViewModel - Cleanup (#1412) * Filter Changes * Use `viewModel.modifiedFilters` for tracking if the filter has been modified. Update the init and update. Hold only the modified filters in `modifiedFilters` instead of `(modifiedFilters, bool)` since that's just clunky and unnecessary. * Reset button should be disabled when only THAT filter is non-default. * ... * PagingLIbraryViewModel.filterQueryTask is no longer in use since that should now be handled on the FilterViewModel * fix merge * cleanup --------- Co-authored-by: Ethan Pippin --- Shared/Components/SelectorView.swift | 62 +++---- Shared/Objects/Stateful.swift | 2 +- Shared/ViewModels/FilterViewModel.swift | 151 +++++++++++++++++- .../PagingLibraryViewModel.swift | 7 +- Shared/ViewModels/SearchViewModel.swift | 14 +- .../Components/LetterPickerButton.swift | 6 +- .../FilterDrawerButton.swift | 14 +- .../NavigationBarFilterDrawer.swift | 13 +- Swiftfin/Views/FilterView.swift | 39 +---- Swiftfin/Views/FontPickerView.swift | 2 +- 10 files changed, 212 insertions(+), 98 deletions(-) diff --git a/Shared/Components/SelectorView.swift b/Shared/Components/SelectorView.swift index ba7d2a2d..27742b21 100644 --- a/Shared/Components/SelectorView.swift +++ b/Shared/Components/SelectorView.swift @@ -22,8 +22,13 @@ struct SelectorView: View { @Default(.accentColor) private var accentColor - @StateObject - private var selection: BindingBox> + @State + private var selectedItems: Set + + private let selectionBinding: Binding> + private let sources: [Element] + private var label: (Element) -> Label + private let type: SelectorType private init( selection: Binding>, @@ -31,16 +36,13 @@ struct SelectorView: View { label: @escaping (Element) -> Label, type: SelectorType ) { - self._selection = StateObject(wrappedValue: BindingBox(source: selection)) + self.selectionBinding = selection + self._selectedItems = State(initialValue: selection.wrappedValue) self.sources = sources self.label = label self.type = type } - private let sources: [Element] - private var label: (Element) -> Label - private let type: SelectorType - var body: some View { List(sources, id: \.hashValue) { element in Button { @@ -56,7 +58,7 @@ struct SelectorView: View { Spacer() - if selection.value.contains(element) { + if selectedItems.contains(element) { Image(systemName: "checkmark.circle.fill") .resizable() .backport @@ -69,33 +71,37 @@ struct SelectorView: View { } } } + .onChange(of: selectionBinding.wrappedValue) { newValue in + selectedItems = newValue + } } private func handleSingleSelect(with element: Element) { - selection.value = [element] + selectedItems = [element] + selectionBinding.wrappedValue = selectedItems } private func handleMultiSelect(with element: Element) { - if selection.value.contains(element) { - selection.value.remove(element) + if selectedItems.contains(element) { + selectedItems.remove(element) } else { - selection.value.insert(element) + selectedItems.insert(element) } + selectionBinding.wrappedValue = selectedItems } } extension SelectorView where Label == Text { - init(selection: Binding<[Element]>, sources: [Element], type: SelectorType) { - - let selectionBinding = Binding { - Set(selection.wrappedValue) - } set: { newValue in - selection.wrappedValue = sources.intersection(newValue) - } + let setBinding = Binding>( + get: { Set(selection.wrappedValue) }, + set: { newValue in + selection.wrappedValue = Array(newValue) + } + ) self.init( - selection: selectionBinding, + selection: setBinding, sources: sources, label: { Text($0.displayTitle).foregroundColor(.primary) }, type: type @@ -103,15 +109,17 @@ extension SelectorView where Label == Text { } init(selection: Binding, sources: [Element]) { - - let selectionBinding = Binding { - Set([selection.wrappedValue]) - } set: { newValue in - selection.wrappedValue = newValue.first! - } + let setBinding = Binding>( + get: { Set([selection.wrappedValue]) }, + set: { newValue in + if let first = newValue.first { + selection.wrappedValue = first + } + } + ) self.init( - selection: selectionBinding, + selection: setBinding, sources: sources, label: { Text($0.displayTitle).foregroundColor(.primary) }, type: .single diff --git a/Shared/Objects/Stateful.swift b/Shared/Objects/Stateful.swift index e2c602ac..fda22269 100644 --- a/Shared/Objects/Stateful.swift +++ b/Shared/Objects/Stateful.swift @@ -16,7 +16,7 @@ import OrderedCollections // parent class actions // TODO: official way for a cleaner `respond` method so it doesn't have all Task // construction and get bloated -// TODO: make Action: Hashable just for consistency +// TODO: move backgroundStates to just a `Set` protocol Stateful: AnyObject { diff --git a/Shared/ViewModels/FilterViewModel.swift b/Shared/ViewModels/FilterViewModel.swift index 0fef0973..94211b6f 100644 --- a/Shared/ViewModels/FilterViewModel.swift +++ b/Shared/ViewModels/FilterViewModel.swift @@ -6,26 +6,65 @@ // Copyright (c) 2025 Jellyfin & Jellyfin Contributors // +import Combine import Foundation import JellyfinAPI +import OrderedCollections import SwiftUI -final class FilterViewModel: ViewModel { +final class FilterViewModel: ViewModel, Stateful { - @Published - var currentFilters: ItemFilterCollection + // MARK: - Action + enum Action: Equatable { + case cancel + case getQueryFilters + case reset(ItemFilterType? = nil) + case update(ItemFilterType, [AnyItemFilter]) + } + + // MARK: - Background State + + enum BackgroundState: Hashable { + case gettingQueryFilters + case failedToGetQueryFilters + } + + // MARK: - State + + enum State: Hashable { + case content + } + + /// Tracks the current filters @Published - var allFilters: ItemFilterCollection = .all + private(set) var currentFilters: ItemFilterCollection + + /// All filters available + @Published + private(set) var allFilters: ItemFilterCollection = .all + + /// ViewModel Background State(s) + @Published + var backgroundStates: OrderedSet = [] + + /// ViewModel State + @Published + var state: State = .content private let parent: (any LibraryParent)? + private var queryFiltersTask: AnyCancellable? + + // MARK: - Initialize from Library Parent + init( parent: (any LibraryParent)? = nil, currentFilters: ItemFilterCollection = .default ) { self.parent = parent self.currentFilters = currentFilters + super.init() if let parent { @@ -33,9 +72,102 @@ final class FilterViewModel: ViewModel { } } + func isFilterSelected(type: ItemFilterType) -> Bool { + currentFilters[keyPath: type.collectionAnyKeyPath] != ItemFilterCollection.default[keyPath: type.collectionAnyKeyPath] + } + + // MARK: - Respond to Action + + func respond(to action: Action) -> State { + switch action { + case .cancel: + queryFiltersTask?.cancel() + backgroundStates.removeAll() + + case .getQueryFilters: + queryFiltersTask?.cancel() + queryFiltersTask = Task { + do { + await MainActor.run { + _ = self.backgroundStates.append(.gettingQueryFilters) + } + + try await setQueryFilters() + } catch { + await MainActor.run { + _ = self.backgroundStates.append(.failedToGetQueryFilters) + } + } + + await MainActor.run { + _ = self.backgroundStates.remove(.gettingQueryFilters) + } + } + .asAnyCancellable() + + case let .reset(type): + if let type { + resetCurrentFilters(for: type) + } else { + currentFilters = .default + } + + case let .update(type, filters): + updateCurrentFilters(for: type, with: filters) + } + + return state + } + + // MARK: - Reset Current Filters + + /// Reset the filter for a specific type to its default value + private func resetCurrentFilters(for type: ItemFilterType) { + switch type { + case .genres: + currentFilters.genres = ItemFilterCollection.default.genres + case .letter: + currentFilters.letter = ItemFilterCollection.default.letter + case .sortBy: + currentFilters.sortBy = ItemFilterCollection.default.sortBy + case .sortOrder: + currentFilters.sortOrder = ItemFilterCollection.default.sortOrder + case .tags: + currentFilters.tags = ItemFilterCollection.default.tags + case .traits: + currentFilters.traits = ItemFilterCollection.default.traits + case .years: + currentFilters.years = ItemFilterCollection.default.years + } + } + + // MARK: - Update Current Filters + + /// Update the filter for a specific type with new values + private func updateCurrentFilters(for type: ItemFilterType, with newValue: [AnyItemFilter]) { + switch type { + case .genres: + currentFilters.genres = newValue.map(ItemGenre.init) + case .letter: + currentFilters.letter = newValue.map(ItemLetter.init) + case .sortBy: + currentFilters.sortBy = newValue.map(ItemSortBy.init) + case .sortOrder: + currentFilters.sortOrder = newValue.map(ItemSortOrder.init) + case .tags: + currentFilters.tags = newValue.map(ItemTag.init) + case .traits: + currentFilters.traits = newValue.map(ItemTrait.init) + case .years: + currentFilters.years = newValue.map(ItemYear.init) + } + } + + // MARK: - Set Query Filters + /// Sets the query filters from the parent - func setQueryFilters() async { - let queryFilters = await getQueryFilters() + private func setQueryFilters() async throws { + let queryFilters = try await getQueryFilters() await MainActor.run { allFilters.genres = queryFilters.genres @@ -44,7 +176,10 @@ final class FilterViewModel: ViewModel { } } - private func getQueryFilters() async -> (genres: [ItemGenre], tags: [ItemTag], years: [ItemYear]) { + // MARK: - Get Query Filters + + /// Gets the query filters from the parent + private func getQueryFilters() async throws -> (genres: [ItemGenre], tags: [ItemTag], years: [ItemYear]) { let parameters = Paths.GetQueryFiltersLegacyParameters( userID: userSession.user.id, @@ -52,7 +187,7 @@ final class FilterViewModel: ViewModel { ) let request = Paths.getQueryFiltersLegacy(parameters: parameters) - guard let response = try? await userSession.client.send(request) else { return ([], [], []) } + let response = try await userSession.client.send(request) let genres: [ItemGenre] = (response.value.genres ?? []) .map(ItemGenre.init) diff --git a/Shared/ViewModels/LibraryViewModel/PagingLibraryViewModel.swift b/Shared/ViewModels/LibraryViewModel/PagingLibraryViewModel.swift index be0b62c8..a41f5f36 100644 --- a/Shared/ViewModels/LibraryViewModel/PagingLibraryViewModel.swift +++ b/Shared/ViewModels/LibraryViewModel/PagingLibraryViewModel.swift @@ -119,7 +119,6 @@ class PagingLibraryViewModel: ViewModel, Eventful, Stateful { // tasks - private var filterQueryTask: AnyCancellable? private var pagingTask: AnyCancellable? private var randomItemTask: AnyCancellable? @@ -252,14 +251,10 @@ class PagingLibraryViewModel: ViewModel, Eventful, Stateful { return .error(error) case .refresh: - filterQueryTask?.cancel() pagingTask?.cancel() randomItemTask?.cancel() - filterQueryTask = Task { - await filterViewModel?.setQueryFilters() - } - .asAnyCancellable() + filterViewModel?.send(.getQueryFilters) pagingTask = Task { [weak self] in guard let self else { return } diff --git a/Shared/ViewModels/SearchViewModel.swift b/Shared/ViewModels/SearchViewModel.swift index 88f474e2..6114def1 100644 --- a/Shared/ViewModels/SearchViewModel.swift +++ b/Shared/ViewModels/SearchViewModel.swift @@ -117,10 +117,7 @@ final class SearchViewModel: ViewModel, Stateful { return .searching } case .getSuggestions: - Task { - await filterViewModel.setQueryFilters() - } - .store(in: &cancellables) + filterViewModel.send(.getQueryFilters) Task { let suggestions = try await getSuggestions() @@ -223,6 +220,15 @@ final class SearchViewModel: ViewModel, Stateful { parameters.tags = filters.tags.map(\.value) parameters.years = filters.years.map(\.intValue) + if filters.letter.first?.value == "#" { + parameters.nameLessThan = "A" + } else { + parameters.nameStartsWith = filters.letter + .map(\.value) + .filter { $0 != "#" } + .first + } + let request = Paths.getItemsByUserID(userID: userSession.user.id, parameters: parameters) let response = try await userSession.client.send(request) diff --git a/Swiftfin/Components/LetterPickerBar/Components/LetterPickerButton.swift b/Swiftfin/Components/LetterPickerBar/Components/LetterPickerButton.swift index 0960f7be..7aff431a 100644 --- a/Swiftfin/Components/LetterPickerBar/Components/LetterPickerButton.swift +++ b/Swiftfin/Components/LetterPickerBar/Components/LetterPickerButton.swift @@ -31,10 +31,10 @@ extension LetterPickerBar { var body: some View { Button { - if !viewModel.currentFilters.letter.contains(letter) { - viewModel.currentFilters.letter = [ItemLetter(stringLiteral: letter.value)] + if viewModel.currentFilters.letter.contains(letter) { + viewModel.send(.update(.letter, [])) } else { - viewModel.currentFilters.letter = [] + viewModel.send(.update(.letter, [ItemLetter(stringLiteral: letter.value).asAnyItemFilter])) } } label: { ZStack { diff --git a/Swiftfin/Components/NavigationBarFilterDrawer/FilterDrawerButton.swift b/Swiftfin/Components/NavigationBarFilterDrawer/FilterDrawerButton.swift index 75244ac2..97fcf6c6 100644 --- a/Swiftfin/Components/NavigationBarFilterDrawer/FilterDrawerButton.swift +++ b/Swiftfin/Components/NavigationBarFilterDrawer/FilterDrawerButton.swift @@ -16,9 +16,11 @@ extension NavigationBarFilterDrawer { @Default(.accentColor) private var accentColor + @Environment(\.isSelected) + private var isSelected + private let systemName: String? private let title: String - private let activated: Bool private var onSelect: () -> Void var body: some View { @@ -43,12 +45,12 @@ extension NavigationBarFilterDrawer { .padding(.vertical, 5) .background { Capsule() - .foregroundColor(activated ? accentColor : Color(UIColor.secondarySystemFill)) + .foregroundColor(isSelected ? accentColor : Color(UIColor.secondarySystemFill)) .opacity(0.5) } .overlay { Capsule() - .stroke(activated ? accentColor : Color(UIColor.secondarySystemFill), lineWidth: 1) + .stroke(isSelected ? accentColor : Color(UIColor.secondarySystemFill), lineWidth: 1) } } } @@ -57,20 +59,18 @@ extension NavigationBarFilterDrawer { extension NavigationBarFilterDrawer.FilterDrawerButton { - init(title: String, activated: Bool) { + init(title: String) { self.init( systemName: nil, title: title, - activated: activated, onSelect: {} ) } - init(systemName: String, activated: Bool) { + init(systemName: String) { self.init( systemName: systemName, title: "", - activated: activated, onSelect: {} ) } diff --git a/Swiftfin/Components/NavigationBarFilterDrawer/NavigationBarFilterDrawer.swift b/Swiftfin/Components/NavigationBarFilterDrawer/NavigationBarFilterDrawer.swift index ad624b57..efcfe3e1 100644 --- a/Swiftfin/Components/NavigationBarFilterDrawer/NavigationBarFilterDrawer.swift +++ b/Swiftfin/Components/NavigationBarFilterDrawer/NavigationBarFilterDrawer.swift @@ -24,22 +24,25 @@ struct NavigationBarFilterDrawer: View { if viewModel.currentFilters.hasFilters { Menu { Button(L10n.reset, role: .destructive) { - viewModel.currentFilters = .default + viewModel.send(.reset()) } } label: { - FilterDrawerButton(systemName: "line.3.horizontal.decrease.circle.fill", activated: true) + FilterDrawerButton(systemName: "line.3.horizontal.decrease.circle.fill") + .environment(\.isSelected, true) } } ForEach(filterTypes, id: \.self) { type in FilterDrawerButton( - title: type.displayTitle, - activated: viewModel.currentFilters[keyPath: type.collectionAnyKeyPath] != ItemFilterCollection - .default[keyPath: type.collectionAnyKeyPath] + title: type.displayTitle ) .onSelect { onSelect(.init(type: type, viewModel: viewModel)) } + .environment( + \.isSelected, + viewModel.isFilterSelected(type: type) + ) } } .padding(.horizontal) diff --git a/Swiftfin/Views/FilterView.swift b/Swiftfin/Views/FilterView.swift index afe25400..8885bf27 100644 --- a/Swiftfin/Views/FilterView.swift +++ b/Swiftfin/Views/FilterView.swift @@ -9,8 +9,6 @@ import JellyfinAPI import SwiftUI -// Note: Keep all of the ItemFilterCollection/ItemFilter/AnyItemFilter KeyPath wackiness in this file - // TODO: multiple filter types? // - for sort order and sort by combined struct FilterView: View { @@ -39,27 +37,11 @@ struct FilterView: View { } .topBarTrailing { Button(L10n.reset) { - switch type { - case .genres: - viewModel.currentFilters.genres = ItemFilterCollection.default.genres - case .letter: - viewModel.currentFilters.letter = ItemFilterCollection.default.letter - case .sortBy: - viewModel.currentFilters.sortBy = ItemFilterCollection.default.sortBy - case .sortOrder: - viewModel.currentFilters.sortOrder = ItemFilterCollection.default.sortOrder - case .tags: - viewModel.currentFilters.tags = ItemFilterCollection.default.tags - case .traits: - viewModel.currentFilters.traits = ItemFilterCollection.default.traits - case .years: - viewModel.currentFilters.years = ItemFilterCollection.default.years - } + viewModel.send(.reset(type)) } .environment( \.isEnabled, - viewModel.currentFilters[keyPath: type.collectionAnyKeyPath] != ItemFilterCollection - .default[keyPath: type.collectionAnyKeyPath] + viewModel.isFilterSelected(type: type) ) } } @@ -75,22 +57,7 @@ extension FilterView { let selectionBinding: Binding<[AnyItemFilter]> = Binding { viewModel.currentFilters[keyPath: type.collectionAnyKeyPath] } set: { newValue in - switch type { - case .genres: - viewModel.currentFilters.genres = newValue.map(ItemGenre.init) - case .letter: - viewModel.currentFilters.letter = newValue.map(ItemLetter.init) - case .sortBy: - viewModel.currentFilters.sortBy = newValue.map(ItemSortBy.init) - case .sortOrder: - viewModel.currentFilters.sortOrder = newValue.map(ItemSortOrder.init) - case .tags: - viewModel.currentFilters.tags = newValue.map(ItemTag.init) - case .traits: - viewModel.currentFilters.traits = newValue.map(ItemTrait.init) - case .years: - viewModel.currentFilters.years = newValue.map(ItemYear.init) - } + viewModel.send(.update(type, newValue)) } self.init( diff --git a/Swiftfin/Views/FontPickerView.swift b/Swiftfin/Views/FontPickerView.swift index a1406961..830bfd23 100644 --- a/Swiftfin/Views/FontPickerView.swift +++ b/Swiftfin/Views/FontPickerView.swift @@ -24,6 +24,6 @@ struct FontPickerView: View { .foregroundColor(.primary) .font(.custom(fontFamily, size: 18)) } - .navigationTitle("Font") + .navigationTitle(L10n.subtitleFont) } }