diff --git a/Yattee/Views/Home/PlaylistCreationTarget.swift b/Yattee/Views/Home/PlaylistCreationTarget.swift new file mode 100644 index 00000000..26ef9e37 --- /dev/null +++ b/Yattee/Views/Home/PlaylistCreationTarget.swift @@ -0,0 +1,72 @@ +// +// PlaylistCreationTarget.swift +// Yattee +// +// Where a newly created playlist goes. +// + +import Foundation + +/// Where the playlists screen would put a playlist it is asked to create. +/// +/// Kept as a value rather than left to be re-derived at save time: a creation +/// travels with the sheet that collects it, so the destination cannot move +/// while the form is open — and cannot be read one update late, which is how a +/// creation on an account ended up in local data with its visibility picker +/// missing. +enum PlaylistCreationTarget: Identifiable, Equatable { + /// The device's own playlists. + case local + /// A server-side playlist on this account. + case account(Account) + + var id: String { + switch self { + case .local: "local" + case let .account(account): account.id.uuidString + } + } + + /// The account to create on, or `nil` when the device is the destination. + var account: Account? { + switch self { + case .local: nil + case let .account(account): account + } + } + + /// Where a creation started right now would go, or `nil` when there is + /// nowhere to put one and the affordance must not be offered. + /// + /// Follows the *effective* context, which is what the rows on screen came + /// from: a selected account that cannot be used falls back to local reads, + /// and creating locally is then the same answer the list is already giving + /// — with the banner above it saying why. What never happens is the other + /// direction: an account context whose server cannot be written to right + /// now creates nothing at all, rather than quietly retargeting the write at + /// local data the user did not choose. + /// + /// - Parameters: + /// - account: the account `effectiveContext` names, when it resolves. + /// - allowsServerMutations: the validator's write gate. + /// - capabilities: what that account's backend can do. + static func resolve( + effectiveContext: DataContext, + account: Account?, + allowsServerMutations: Bool, + capabilities: AccountCapabilities? + ) -> PlaylistCreationTarget? { + switch effectiveContext { + case .local: + return .local + + case .account: + guard let account, + allowsServerMutations, + capabilities?.supportsPlaylistWrites == true + else { return nil } + + return .account(account) + } + } +} diff --git a/Yattee/Views/Home/PlaylistsListView.swift b/Yattee/Views/Home/PlaylistsListView.swift index d3a8324d..95649869 100644 --- a/Yattee/Views/Home/PlaylistsListView.swift +++ b/Yattee/Views/Home/PlaylistsListView.swift @@ -12,7 +12,6 @@ struct PlaylistsListView: View { @Namespace private var sheetTransition @State private var localPlaylists: [LocalPlaylist] = [] @State private var searchText = "" - @State private var showingNewPlaylist = false @State private var playlistToEdit: LocalPlaylist? #if os(tvOS) @FocusState private var focusedPlaylistID: PlaylistListItemID? @@ -21,9 +20,15 @@ struct PlaylistsListView: View { // MARK: - Remote state - /// The account a creation is for, captured when the button was pressed — - /// the form must not follow a context that moves while it is open. - @State private var createAccount: Account? + /// The creation being collected, and where it goes — captured when the + /// button was pressed. + /// + /// The destination travels with the presentation instead of standing beside + /// it in a second `@State`: a companion value is read by the sheet's own + /// closure one update behind the button that set it, so a creation on an + /// account took the local branch — writing to the device and losing the + /// visibility picker on the way. + @State private var creation: PlaylistCreationTarget? @State private var remoteEdit: RemotePlaylistEdit? @State private var remoteDeletion: RemotePlaylistDeletion? /// Why the account's list could not be fetched, when it could not. @@ -110,23 +115,36 @@ struct PlaylistsListView: View { return appEnvironment.accountsManager.account(id: id) } + /// Where a playlist created right now would go, or `nil` when there is + /// nowhere to put one. + /// + /// The single answer both the create affordances and the row actions are + /// gated on, so the button that offers a write and the write it performs + /// can never have decided differently. + private var creationTarget: PlaylistCreationTarget? { + guard let appEnvironment else { return nil } + let account = contextAccount + + return PlaylistCreationTarget.resolve( + effectiveContext: appEnvironment.dataContextValidator.effectiveContext, + account: account, + allowsServerMutations: appEnvironment.dataContextValidator.allowsServerMutations, + capabilities: account.flatMap { appEnvironment.accountsManager.capabilities(for: $0) } + ) + } + /// The account these playlists may be changed on. /// /// Both halves are asked even though reads only ever follow a valid context /// today: the write gate is what has to hold if that ever stops being true. private var writableAccount: Account? { - guard let appEnvironment, - let account = contextAccount, - appEnvironment.dataContextValidator.allowsServerMutations, - appEnvironment.accountsManager.capabilities(for: account)?.supportsPlaylistWrites == true - else { return nil } - + guard case let .account(account) = creationTarget else { return nil } return account } /// Whether the create affordances have somewhere to create. private var canCreatePlaylist: Bool { - contextAccount == nil || writableAccount != nil + creationTarget != nil } private var contextUnavailability: DataContextUnavailability? { @@ -232,18 +250,18 @@ struct PlaylistsListView: View { .liquidGlassSheetContent(sourceID: "playlistsViewOptions", in: sheetTransition) } #endif - .sheet(isPresented: $showingNewPlaylist) { - if let createAccount { + .sheet(item: $creation) { creation in + if let account = creation.account { PlaylistFormSheet( mode: .create, - showsPrivacy: supportsPrivacy(createAccount) + showsPrivacy: supportsPrivacy(account) ) { title, description, privacy in Task { await createRemotePlaylist( title: title, description: description, privacy: privacy, - on: createAccount + on: account ) } } @@ -328,12 +346,18 @@ struct PlaylistsListView: View { @ViewBuilder private var content: some View { if isLoadingRemoteList { - ProgressView() - .frame(maxWidth: .infinity, maxHeight: .infinity) + scrollableState { + ProgressView() + .frame(maxWidth: .infinity) + } } else if let remoteErrorMessage, items.isEmpty { - remoteErrorView(remoteErrorMessage) + scrollableState { + remoteErrorView(remoteErrorMessage) + } } else if filteredItems.isEmpty { - emptyView + scrollableState { + emptyView + } } else { switch layout { case .list: @@ -344,6 +368,18 @@ struct PlaylistsListView: View { } } + /// A state with nothing to scroll, put in a scroll view anyway. + /// + /// Two things need one. Pull to refresh has nowhere to live without it — + /// and a navigation bar carrying a search field keeps its full height over + /// content that cannot scroll, which is what the banner was landing under. + private func scrollableState(@ViewBuilder _ state: () -> some View) -> some View { + ScrollView { + state() + .containerRelativeFrame(.vertical) + } + } + #if os(tvOS) // MARK: - tvOS Content @@ -714,9 +750,13 @@ struct PlaylistsListView: View { // MARK: - Creating + /// Opens the form over the destination as it stands at this moment. + /// + /// Silent when there is nowhere to create: a toolbar item can outlive the + /// condition that put it there, and a write with nowhere to go is not a + /// write to somewhere else. private func beginCreate() { - createAccount = writableAccount - showingNewPlaylist = true + creation = creationTarget } private func createRemotePlaylist( diff --git a/Yattee/Views/Subscriptions/SubscriptionsView.swift b/Yattee/Views/Subscriptions/SubscriptionsView.swift index f6c212b3..515e9c8e 100644 --- a/Yattee/Views/Subscriptions/SubscriptionsView.swift +++ b/Yattee/Views/Subscriptions/SubscriptionsView.swift @@ -165,11 +165,24 @@ struct SubscriptionsView: View { } #endif + /// The feed error worth showing, if any. + /// + /// Nothing while the context banner is up: a selected account that cannot be + /// used reads local data, and the local feed then reports the Yattee Server + /// it has no reason to have — "add a subscriptions account in Settings" + /// under a banner saying *this* account needs signing in again is two + /// answers to one question. The banner names the cause, so it is the one + /// that stays. + private var displayedFeedError: FeedLoadState.FeedLoadError? { + guard contextUnavailability == nil, + case let .error(error) = feedCache.feedLoadState + else { return nil } + + return error + } + private var isShowingFullScreenError: Bool { - if case .error = feedCache.feedLoadState, feedCache.videos.isEmpty { - return true - } - return false + displayedFeedError != nil && feedCache.videos.isEmpty } private var dataManager: DataManager? { appEnvironment?.dataManager } @@ -249,19 +262,21 @@ struct SubscriptionsView: View { .background(Color.black.opacity(0.3)) #endif - case .error(let error): - HStack(spacing: 8) { - Image(systemName: "exclamationmark.triangle") - Text(errorMessage(for: error)) - .font(.caption) + case .error: + if let error = displayedFeedError { + HStack(spacing: 8) { + Image(systemName: "exclamationmark.triangle") + Text(errorMessage(for: error)) + .font(.caption) + } + .foregroundStyle(.red) + .padding(.horizontal) + .padding(.vertical, 8) + .frame(maxWidth: .infinity) + #if os(tvOS) + .background(Color.black.opacity(0.3)) + #endif } - .foregroundStyle(.red) - .padding(.horizontal) - .padding(.vertical, 8) - .frame(maxWidth: .infinity) - #if os(tvOS) - .background(Color.black.opacity(0.3)) - #endif case .loadingMore: HStack(spacing: 8) { @@ -640,7 +655,7 @@ struct SubscriptionsView: View { /// Feed content rows or empty/loading states. @ViewBuilder private var feedContentRows: some View { - if case .error(let feedError) = feedCache.feedLoadState, feedCache.videos.isEmpty { + if let feedError = displayedFeedError, feedCache.videos.isEmpty { // Show specific error states switch feedError { case .yatteeServerRequired: @@ -730,7 +745,7 @@ struct SubscriptionsView: View { #endif // Content - if case .error(let feedError) = feedCache.feedLoadState, feedCache.videos.isEmpty { + if let feedError = displayedFeedError, feedCache.videos.isEmpty { // Show specific error states switch feedError { case .yatteeServerRequired: diff --git a/YatteeTests/PlaylistCreationTargetTests.swift b/YatteeTests/PlaylistCreationTargetTests.swift new file mode 100644 index 00000000..00baa901 --- /dev/null +++ b/YatteeTests/PlaylistCreationTargetTests.swift @@ -0,0 +1,107 @@ +// +// PlaylistCreationTargetTests.swift +// YatteeTests +// +// Tests for where a newly created playlist is sent. +// + +import Testing +import Foundation +@testable import Yattee + +@Suite("Playlist Creation Target") +struct PlaylistCreationTargetTests { + + // MARK: - Helpers + + private static let account = Account(instanceID: UUID()) + + private static func capabilities(playlistWrites: Bool) -> AccountCapabilities { + AccountCapabilities( + supportsSubscriptions: true, + supportsFeed: true, + supportsPlaylistReads: true, + supportsPlaylistWrites: playlistWrites, + supportsWatchHistory: false, + supportsPlaylistPrivacy: false + ) + } + + private static func resolve( + effectiveContext: DataContext, + account: Account? = account, + allowsServerMutations: Bool = true, + capabilities: AccountCapabilities? = capabilities(playlistWrites: true) + ) -> PlaylistCreationTarget? { + PlaylistCreationTarget.resolve( + effectiveContext: effectiveContext, + account: account, + allowsServerMutations: allowsServerMutations, + capabilities: capabilities + ) + } + + // MARK: - Local + + @Test("The device's own context creates locally") + func localContextCreatesLocally() { + #expect(Self.resolve(effectiveContext: .local, account: nil) == .local) + } + + /// The fallback case: the selected account is unusable, reads have moved to + /// local, and the rows on screen are local — so a creation is local too. + @Test("A context that fell back to local creates locally") + func fallbackCreatesLocally() { + #expect( + Self.resolve( + effectiveContext: .local, + account: nil, + allowsServerMutations: false, + capabilities: nil + ) == .local + ) + } + + // MARK: - Account + + @Test("An account context creates on that account") + func accountContextCreatesOnAccount() { + #expect(Self.resolve(effectiveContext: .account(Self.account.id)) == .account(Self.account)) + } + + /// The defect this exists to prevent: an account context must never quietly + /// become a local write. + @Test("An account that may not be written to creates nowhere") + func mutationsDisallowedCreatesNowhere() { + #expect(Self.resolve(effectiveContext: .account(Self.account.id), allowsServerMutations: false) == nil) + } + + @Test("A backend without playlist writes creates nowhere") + func withoutPlaylistWritesCreatesNowhere() { + #expect( + Self.resolve( + effectiveContext: .account(Self.account.id), + capabilities: Self.capabilities(playlistWrites: false) + ) == nil + ) + } + + @Test("An account whose capabilities are unknown creates nowhere") + func unknownCapabilitiesCreateNowhere() { + #expect(Self.resolve(effectiveContext: .account(Self.account.id), capabilities: nil) == nil) + } + + @Test("An account context that resolves to no account creates nowhere") + func unresolvedAccountCreatesNowhere() { + #expect(Self.resolve(effectiveContext: .account(Self.account.id), account: nil) == nil) + } + + // MARK: - Identity + + @Test("Each destination presents as itself") + func identityDistinguishesDestinations() { + #expect(PlaylistCreationTarget.local.id != PlaylistCreationTarget.account(Self.account).id) + #expect(PlaylistCreationTarget.account(Self.account).account == Self.account) + #expect(PlaylistCreationTarget.local.account == nil) + } +}