diff --git a/Yattee/Localizable.xcstrings b/Yattee/Localizable.xcstrings index 025706a0..e9ce693d 100644 --- a/Yattee/Localizable.xcstrings +++ b/Yattee/Localizable.xcstrings @@ -8368,6 +8368,17 @@ } } }, + "playlist.privacy.footer.edit" : { + "comment" : "Explains the playlist visibility picker when editing a playlist that already exists", + "localizations" : { + "en" : { + "stringUnit" : { + "state" : "translated", + "value" : "Who can see this playlist on the server. The server does not report this exactly, so it only changes if you pick a different one." + } + } + } + }, "playlist.privacy.private" : { "comment" : "Playlist visibility: only the account owner", "localizations" : { @@ -8478,6 +8489,17 @@ } } }, + "playlist.remote.refresh.error" : { + "comment" : "Refetching a server-side playlist before editing it failed", + "localizations" : { + "en" : { + "stringUnit" : { + "state" : "translated", + "value" : "Could not load the latest playlist" + } + } + } + }, "playlist.remote.remove.error" : { "comment" : "Removing a video from a server-side playlist failed", "localizations" : { diff --git a/Yattee/Models/RemotePlaylistEntry.swift b/Yattee/Models/RemotePlaylistEntry.swift index 928337f2..a802ac01 100644 --- a/Yattee/Models/RemotePlaylistEntry.swift +++ b/Yattee/Models/RemotePlaylistEntry.swift @@ -87,9 +87,19 @@ struct RemotePlaylistSnapshot: Hashable, Sendable { /// The rows that can be addressed for removal, in playlist order. let entries: [RemotePlaylistEntry] - init(playlist: Playlist, entries: [RemotePlaylistEntry]) { + /// Whether the server lists this playlist publicly, where it says so. + /// + /// Deliberately not a ``PlaylistPrivacy``, because it is not one: Invidious + /// serialises visibility as `isListed`, which is literally `privacy.public?` + /// — `true` is public, and `false` is unlisted *or* private with no way to + /// tell the two apart over the API. `nil` where the backend has no + /// visibility concept at all, or did not answer with one. + let isListed: Bool? + + init(playlist: Playlist, entries: [RemotePlaylistEntry], isListed: Bool? = nil) { self.playlist = playlist self.entries = entries + self.isListed = isListed } } diff --git a/Yattee/Services/API/ContentService.swift b/Yattee/Services/API/ContentService.swift index 68333892..9458810e 100644 --- a/Yattee/Services/API/ContentService.swift +++ b/Yattee/Services/API/ContentService.swift @@ -307,77 +307,54 @@ actor ContentService: ContentServiceProtocol { } } - /// Creates a playlist for the credential's owner. + /// Creates a playlist for the credential's owner. **One request.** + /// + /// Neither backend takes a description when creating, so this does not take + /// one either: setting it is a separate write, sequenced by the caller with + /// a credential recheck in between. /// /// The returned playlist is the little the server says about what it just /// made — an id and a title. Nothing else is invented here: callers refetch /// the list, which is the only description that is actually the server's. /// - Parameters: /// - privacy: Ignored by backends without a visibility concept (Piped), - /// and defaulted to private on those that have one when not given. + /// and defaulted to private on those that have one when not given — + /// Invidious rejects a creation that carries no visibility at all. func createPlaylist( title: String, - description: String?, privacy: PlaylistPrivacy?, instance: Instance, credential: String ) async throws -> Playlist { switch instance.type { case .invidious: - let api = await invidiousAPI(for: instance) - let playlist = try await api.createPlaylist( + return try await invidiousAPI(for: instance).createPlaylist( title: title, privacy: privacy ?? .private, instance: instance, sid: credential ) - // Creation takes no description, so a wanted one is a second call. - // Its failure is the caller's to see: a playlist that exists with - // the wrong description is not a silent success. - if let description, !description.isEmpty { - try await api.updatePlaylist( - id: playlist.id.playlistID, - title: title, - description: description, - privacy: privacy ?? .private, - instance: instance, - sid: credential - ) - } - return playlist case .piped: - let api = await pipedAPI(for: instance) - let playlistID = try await api.createPlaylist( + let playlistID = try await pipedAPI(for: instance).createPlaylist( name: title, instance: instance, authToken: credential ) - if let description, !description.isEmpty { - try await api.updatePlaylistDescription( - id: playlistID, - description: description, - instance: instance, - authToken: credential - ) - } - return Playlist(id: .global(playlistID), title: title, description: description) + return Playlist(id: .global(playlistID), title: title) default: throw APIError.notSupported } } - /// Edits one of the credential owner's playlists. + /// Renames one of the credential owner's playlists. **One request.** /// - /// - Parameters: - /// - description: `nil` leaves whatever the playlist has. - /// - privacy: `nil` keeps the private default on backends that have - /// visibility; ignored entirely on those that do not. - func updatePlaylist( + /// - Parameter privacy: the visibility to store, or `nil` to leave whatever + /// the playlist has. Ignored on backends without a visibility concept. + func renamePlaylist( id: String, title: String, - description: String?, privacy: PlaylistPrivacy?, instance: Instance, credential: String @@ -387,32 +364,55 @@ actor ContentService: ContentServiceProtocol { try await invidiousAPI(for: instance).updatePlaylist( id: id, title: title, - description: description, - privacy: privacy ?? .private, + description: nil, + privacy: privacy, instance: instance, sid: credential ) case .piped: - // Two calls, no transaction: Piped edits the name and the - // description through separate endpoints. The rename goes first so - // a failure of the second leaves the visible half applied, and the - // caller's refetch shows exactly what landed. - let api = await pipedAPI(for: instance) - try await api.renamePlaylist( + try await pipedAPI(for: instance).renamePlaylist( id: id, newName: title, instance: instance, authToken: credential ) - if let description { - try await api.updatePlaylistDescription( - id: id, - description: description, - instance: instance, - authToken: credential - ) - } + + default: + throw APIError.notSupported + } + } + + /// Stores a playlist's description. **One request.** + /// + /// - Parameter title: the playlist's title, resent unchanged. Invidious + /// edits every attribute through one endpoint and refuses a body without + /// a title; Piped has a description endpoint of its own and ignores this. + func setPlaylistDescription( + id: String, + description: String, + title: String, + instance: Instance, + credential: String + ) async throws { + switch instance.type { + case .invidious: + try await invidiousAPI(for: instance).updatePlaylist( + id: id, + title: title, + description: description, + privacy: nil, + instance: instance, + sid: credential + ) + + case .piped: + try await pipedAPI(for: instance).updatePlaylistDescription( + id: id, + description: description, + instance: instance, + authToken: credential + ) default: throw APIError.notSupported diff --git a/Yattee/Services/API/InvidiousAPI.swift b/Yattee/Services/API/InvidiousAPI.swift index a8d60580..de9b8203 100644 --- a/Yattee/Services/API/InvidiousAPI.swift +++ b/Yattee/Services/API/InvidiousAPI.swift @@ -208,6 +208,7 @@ actor InvidiousAPI: InstanceAPI { author: firstResponse.author, authorId: firstResponse.authorId, videoCount: firstResponse.videoCount, + isListed: firstResponse.isListed, videos: allVideos ).toPlaylist(baseURL: instance.url) } @@ -320,6 +321,7 @@ actor InvidiousAPI: InstanceAPI { author: firstPage.author, authorId: firstPage.authorId, videoCount: firstPage.videoCount, + isListed: firstPage.isListed, videos: allVideos ).toPlaylist(baseURL: baseURL) @@ -351,7 +353,7 @@ actor InvidiousAPI: InstanceAPI { ) } - return RemotePlaylistSnapshot(playlist: playlist, entries: entries) + return RemotePlaylistSnapshot(playlist: playlist, entries: entries, isListed: firstPage.isListed) } /// Runs the snapshot assembly over raw page payloads, decoding them the way @@ -704,35 +706,38 @@ actor InvidiousAPI: InstanceAPI { /// Edits a playlist's title, description or privacy. /// - /// Title and privacy are sent even when unchanged: Invidious reads a missing - /// field as "leave it alone" for the description but validates the other two - /// on every request, so a partial body is rejected rather than merged. - /// Answers 204 with no body. + /// The title is sent even when unchanged: Invidious reads it out of the body + /// without a default and answers 403 `Missing hash key: "title"` when it is + /// absent (measured). The other two fields fall back to what is stored, so + /// omitting them is how the server is told to keep them — which is the only + /// way to leave a playlist's visibility alone, since the API never says + /// which of unlisted and private it currently is. Answers 204 with no body. /// - Parameters: /// - id: The playlist ID /// - title: The title to store /// - description: The description to store, or `nil` to leave it alone - /// - privacy: The visibility to store + /// - privacy: The visibility to store, or `nil` to leave it alone /// - instance: The Invidious instance /// - sid: The session ID from login func updatePlaylist( id: String, title: String, description: String?, - privacy: PlaylistPrivacy, + privacy: PlaylistPrivacy?, instance: Instance, sid: String ) async throws { struct UpdateRequest: Encodable, Sendable { let title: String - let privacy: String + /// Omitted from the JSON when nil, which keeps the stored visibility. + let privacy: String? /// Omitted from the JSON when nil, which is how the server is told /// to keep the description it already has. let description: String? } let body = try JSONEncoder().encode( - UpdateRequest(title: title, privacy: privacy.rawValue, description: description) + UpdateRequest(title: title, privacy: privacy?.rawValue, description: description) ) let endpoint = GenericEndpoint( path: "/api/v1/auth/playlists/\(id)", @@ -1818,6 +1823,9 @@ private struct InvidiousPlaylist: Decodable, Sendable { let author: String? let authorId: String? let videoCount: Int + /// The whole of what Invidious says about visibility: `privacy.public?`, + /// serialised. Unlisted and private are both `false` and indistinguishable. + let isListed: Bool? let videos: [InvidiousPlaylistItem]? nonisolated func toPlaylist(baseURL: URL) -> Playlist { diff --git a/Yattee/Services/AccountService.swift b/Yattee/Services/AccountService.swift index c1f0112a..71191a09 100644 --- a/Yattee/Services/AccountService.swift +++ b/Yattee/Services/AccountService.swift @@ -15,6 +15,13 @@ import Foundation /// conforms to it, and tests script it. Deliberately stateless and /// credential-carrying — resolving *which* credential is this service's job, /// and the transport must never guess. +/// +/// **Every method here is exactly one request to the server.** Nothing composes +/// two of them, because the credential can be replaced between any two requests +/// and only this service can tell: a composite that awaited its own first call +/// would send the second with a secret that has since changed hands. Backends +/// that need several writes to change one playlist are sequenced above, with a +/// recheck between each pair. protocol RemotePlaylistTransport: Sendable { func userPlaylists(instance: Instance, credential: String) async throws -> [Playlist] @@ -26,21 +33,27 @@ protocol RemotePlaylistTransport: Sendable { func createPlaylist( title: String, - description: String?, privacy: PlaylistPrivacy?, instance: Instance, credential: String ) async throws -> Playlist - func updatePlaylist( + func renamePlaylist( id: String, title: String, - description: String?, privacy: PlaylistPrivacy?, instance: Instance, credential: String ) async throws + func setPlaylistDescription( + id: String, + description: String, + title: String, + instance: Instance, + credential: String + ) async throws + func deletePlaylist(id: String, instance: Instance, credential: String) async throws func addVideoToPlaylist( @@ -133,6 +146,12 @@ final class AccountService { /// Creates a playlist on the account's instance. /// + /// Two requests when a description was asked for, because no backend takes + /// one at creation — and the credential is checked again in between, since + /// it can be replaced while the first one is out. The second request's + /// failure is the caller's to see: a playlist that exists with the wrong + /// description is not a silent success. + /// /// - Parameter privacy: dropped on backends without a visibility concept, /// rather than refused: the playlist is still perfectly creatable there, /// and failing the whole call over a setting the server does not have @@ -147,18 +166,44 @@ final class AccountService { let privacy = supportedPrivacy(privacy, for: account) try requireStillCurrent(credential, for: account) - return try await tracking(account, credential: credential) { + let playlist = try await tracking(account, credential: credential) { try await transport.createPlaylist( title: title, - description: description, privacy: privacy, instance: instance, credential: credential ) } + + if let description, !description.isEmpty { + try requireStillCurrent(credential, for: account) + try await tracking(account, credential: credential) { + try await transport.setPlaylistDescription( + id: playlist.id.playlistID, + description: description, + title: title, + instance: instance, + credential: credential + ) + } + } + + return playlist } /// Edits a playlist on the account's instance. + /// + /// Two requests, no transaction, when the description is part of the edit: + /// Piped stores the name and the description behind separate endpoints, and + /// Invidious refuses to take a description without a title. The rename goes + /// first so a failure of the second leaves the visible half applied, and the + /// caller's refetch shows exactly what landed — with the credential checked + /// again in between, because it can be replaced while the rename is out. + /// + /// - Parameters: + /// - description: `nil` leaves whatever the playlist has. + /// - privacy: `nil` leaves whatever visibility the playlist has; ignored + /// entirely on backends without one. func updatePlaylist( id: String, title: String, @@ -171,15 +216,27 @@ final class AccountService { try requireStillCurrent(credential, for: account) try await tracking(account, credential: credential) { - try await transport.updatePlaylist( + try await transport.renamePlaylist( id: id, title: title, - description: description, privacy: privacy, instance: instance, credential: credential ) } + + if let description { + try requireStillCurrent(credential, for: account) + try await tracking(account, credential: credential) { + try await transport.setPlaylistDescription( + id: id, + description: description, + title: title, + instance: instance, + credential: credential + ) + } + } } /// Deletes a playlist from the account's instance. diff --git a/Yattee/Services/RemotePlaylistsCache.swift b/Yattee/Services/RemotePlaylistsCache.swift index b6cfade6..6b645f98 100644 --- a/Yattee/Services/RemotePlaylistsCache.swift +++ b/Yattee/Services/RemotePlaylistsCache.swift @@ -158,7 +158,7 @@ final class RemotePlaylistsCache { func deletePlaylist(id: String, for account: Account) async throws { do { try await accountService.deletePlaylist(id: id, for: account) - forgetSnapshot(id: id, for: account.id) + forgetPlaylist(id: id, for: account.id) await reconcileList(for: account) } catch { // The delete may have landed even though the answer did not, so the @@ -347,8 +347,7 @@ final class RemotePlaylistsCache { do { try await fetchSnapshot(id: id, for: account) } catch let error as APIError where Self.saysPlaylistIsGone(error) { - forgetSnapshot(id: id, for: account.id) - forgetListEntry(id: id, for: account.id) + forgetPlaylist(id: id, for: account.id) } catch { // Swallowed: see the doc comment on `reconcileList(for:)`. } @@ -365,17 +364,22 @@ final class RemotePlaylistsCache { } } - private func forgetSnapshot(id: String, for accountID: UUID) { + /// Drops a playlist the server is known no longer to have. + /// + /// Three things, and each one is load-bearing. The snapshot goes, so a + /// detail view still open on it has nothing left to show. The list entry + /// goes, because the list is refetched right after but that refetch is + /// allowed to fail — offline, a playlist known to be gone would otherwise + /// sit in the list until the next successful fetch. And both fences move + /// past every request already in flight: a refresh that started before the + /// delete describes a playlist that still existed, and arriving late does + /// not make it exist again. + private func forgetPlaylist(id: String, for accountID: UUID) { + appliedSnapshotSequence[SnapshotKey(accountID: accountID, playlistID: id)] = nextSequence() + appliedListSequence[accountID] = nextSequence() + snapshots[accountID]?[id] = nil snapshotFetchDates[accountID]?[id] = nil - } - - /// Takes a playlist out of the account's last-fetched list. - /// - /// The list is refetched right after, which normally says the same thing — - /// but a refetch that cannot reach the server would otherwise leave a - /// playlist that is known to be gone sitting in it. - private func forgetListEntry(id: String, for accountID: UUID) { lists[accountID]?.removeAll { $0.id.playlistID == id } } diff --git a/Yattee/Views/Instances/InstanceBrowseView.swift b/Yattee/Views/Instances/InstanceBrowseView.swift index 3745a85e..5e83baac 100644 --- a/Yattee/Views/Instances/InstanceBrowseView.swift +++ b/Yattee/Views/Instances/InstanceBrowseView.swift @@ -446,19 +446,16 @@ struct InstanceBrowseView: View { } // A credential can arrive over iCloud Keychain, or an account be added, // while this stays on screen — the Feed tab depends on it and would - // otherwise stay missing until the view was rebuilt. A status change can - // also move *which* account is effective here, since the resolution - // falls back to a sibling login on the same instance, so the content - // loaded for the previous one has to go with it. + // otherwise stay missing until the view was rebuilt. Either notification + // can also move *which* account is effective here: a status change + // because the resolution falls back to a sibling login on the same + // instance, an add or a remove because it changes what there is to fall + // back to. Content loaded for the previous one has to go with it. .onReceive(NotificationCenter.default.publisher(for: .accountStatusesDidChange)) { _ in - let accountChanged = discardAccountScopedContentIfAccountChanged() - refreshLoginState() - if accountChanged { - Task { await startContentLoad() } - } + handleEffectiveAccountChange() } .onReceive(NotificationCenter.default.publisher(for: .accountsDidChange)) { _ in - refreshLoginState() + handleEffectiveAccountChange() } // Selecting the other login on this instance changes whose feed and // playlists these are. Nothing else here would notice: the instance, @@ -466,14 +463,18 @@ struct InstanceBrowseView: View { .onReceive(NotificationCenter.default.publisher(for: .dataContextDidChange)) { _ in discardAccountScopedContent() refreshLoginState() - Task { await startContentLoad() } + if !resetSelectedTabIfUnavailable() { + Task { await startContentLoad() } + } } // Same reason, without the context moving: the credential behind this // instance's account was replaced. .onReceive(NotificationCenter.default.publisher(for: .accountCredentialDidChange)) { _ in discardAccountScopedContent() refreshLoginState() - Task { await startContentLoad() } + if !resetSelectedTabIfUnavailable() { + Task { await startContentLoad() } + } } .onChange(of: selectedTab) { _, _ in isLoading = true @@ -1255,6 +1256,37 @@ struct InstanceBrowseView: View { return true } + /// Handles a change to which account this instance resolves to, whatever + /// moved it: a status, or an account being added or removed. + private func handleEffectiveAccountChange() { + let accountChanged = discardAccountScopedContentIfAccountChanged() + refreshLoginState() + + // Moving off a tab that no longer exists reloads by itself. + guard !resetSelectedTabIfUnavailable() else { return } + + if accountChanged { + Task { await startContentLoad() } + } + } + + /// Moves off a tab that has just stopped being offered. + /// + /// The Feed and Playlists tabs exist only while an account backs them. When + /// one goes away the picker drops it, and a selection left pointing at it + /// leaves an unhighlighted tab strip over content nobody can reach. + /// + /// - Returns: whether the selection moved, which reloads through + /// `onChange(of: selectedTab)`. + @discardableResult + private func resetSelectedTabIfUnavailable() -> Bool { + let tabs = availableTabs + guard !tabs.contains(selectedTab), let first = tabs.first else { return false } + + selectedTab = first + return true + } + private func refreshLoginState() { guard instance.supportsAuthentication else { return } isLoggedIn = appEnvironment?.isSignedIn(to: instance) ?? false diff --git a/Yattee/Views/Playlist/PlaylistFormSheet.swift b/Yattee/Views/Playlist/PlaylistFormSheet.swift index 8f334390..dccb2a6d 100644 --- a/Yattee/Views/Playlist/PlaylistFormSheet.swift +++ b/Yattee/Views/Playlist/PlaylistFormSheet.swift @@ -14,9 +14,13 @@ struct PlaylistFormSheet: View { /// Editing a playlist that lives on a server. /// /// Server-side playlists are plain values rather than models, so what to - /// pre-fill is passed in. `privacy` is optional because the fetch does - /// not always say what it is — Invidious returns no visibility with the - /// playlist, so the picker falls back to its conservative default. + /// pre-fill is passed in. `privacy` is optional because the fetch cannot + /// always say what it is: Invidious reports visibility as a single + /// "listed" flag, which is exact for a public playlist and ambiguous + /// between unlisted and private for every other one. The picker then + /// starts at the conservative end — and an untouched picker sends + /// nothing at all, so a visibility that could not be read back is left + /// alone rather than overwritten with a guess. case editRemote(title: String, description: String?, privacy: PlaylistPrivacy?) static func == (lhs: Mode, rhs: Mode) -> Bool { @@ -45,7 +49,9 @@ struct PlaylistFormSheet: View { /// the value rather than inventing it. let showsPrivacy: Bool - private let onSave: (String, String?, PlaylistPrivacy) -> Void + /// - Parameter privacy: `nil` means "leave whatever the playlist has", + /// which only an edit can ask for. + private let onSave: (String, String?, PlaylistPrivacy?) -> Void /// Local playlists have no visibility, so their callers never see one. init(mode: Mode, onSave: @escaping (String, String?) -> Void) { @@ -60,7 +66,7 @@ struct PlaylistFormSheet: View { mode: Mode, showsPrivacy: Bool, privacy: PlaylistPrivacy = .private, - onSaveWithPrivacy: @escaping (String, String?, PlaylistPrivacy) -> Void + onSaveWithPrivacy: @escaping (String, String?, PlaylistPrivacy?) -> Void ) { self.mode = mode self.showsPrivacy = showsPrivacy @@ -73,6 +79,10 @@ struct PlaylistFormSheet: View { /// Private by default: the conservative choice, and the one Invidious makes. @State private var privacy: PlaylistPrivacy = .private + /// What the picker was pre-filled with when editing something that already + /// exists, so a selection left untouched can be told from one that was made. + @State private var prefilledPrivacy: PlaylistPrivacy? + private let maxDescriptionLength = 1000 private var isEditing: Bool { @@ -136,6 +146,7 @@ struct PlaylistFormSheet: View { // Only when the fetch actually said so — otherwise whatever the // caller preselected stands. if let remotePrivacy { privacy = remotePrivacy } + prefilledPrivacy = privacy } } #if os(iOS) @@ -193,7 +204,7 @@ struct PlaylistFormSheet: View { } header: { Text(String(localized: "playlist.privacy")) } footer: { - Text(String(localized: "playlist.privacy.footer")) + Text(privacyFooter) } } } @@ -250,7 +261,7 @@ struct PlaylistFormSheet: View { } header: { Text(String(localized: "playlist.privacy")) } footer: { - Text(String(localized: "playlist.privacy.footer")) + Text(privacyFooter) } } @@ -274,6 +285,18 @@ struct PlaylistFormSheet: View { // MARK: - Privacy + /// What the picker means here — which is not the same sentence when editing. + /// + /// Creating applies the selection outright. Editing applies it only if it + /// was changed, because the value it starts on is the server's answer as + /// far as the server gives one. + private var privacyFooter: String { + if case .editRemote = mode { + return String(localized: "playlist.privacy.footer.edit") + } + return String(localized: "playlist.privacy.footer") + } + private var privacyPicker: some View { Picker(String(localized: "playlist.privacy"), selection: $privacy) { ForEach(PlaylistPrivacy.allCases, id: \.self) { option in @@ -299,10 +322,22 @@ struct PlaylistFormSheet: View { let trimmedTitle = title.trimmingCharacters(in: .whitespacesAndNewlines) let trimmedDescription = descriptionText.trimmingCharacters(in: .whitespacesAndNewlines) - onSave(trimmedTitle, savedDescription(trimmedDescription), privacy) + onSave(trimmedTitle, savedDescription(trimmedDescription), savedPrivacy) dismiss() } + /// The visibility to send, or `nil` to leave the playlist's alone. + /// + /// An existing playlist keeps whatever it has unless the picker was + /// actually moved: the value it was pre-filled with is a best guess — the + /// server can say a playlist is not listed without saying whether it is + /// unlisted or private — and sending a guess back would change a setting + /// nobody asked to change. Creating is the opposite: there is nothing to + /// preserve, and Invidious refuses a creation without a visibility. + private var savedPrivacy: PlaylistPrivacy? { + privacy == prefilledPrivacy ? nil : privacy + } + /// What an emptied description field means to the caller. /// /// Editing a server-side playlist is the one case where the difference diff --git a/Yattee/Views/Playlist/UnifiedPlaylistDetailView.swift b/Yattee/Views/Playlist/UnifiedPlaylistDetailView.swift index 75a40f63..c83a343a 100644 --- a/Yattee/Views/Playlist/UnifiedPlaylistDetailView.swift +++ b/Yattee/Views/Playlist/UnifiedPlaylistDetailView.swift @@ -99,6 +99,12 @@ struct UnifiedPlaylistDetailView: View { /// The account this playlist belongs to, when it belongs to one. @State private var owningAccount: Account? + /// Whether the server lists this playlist publicly, as of the last snapshot. + @State private var remoteIsListed: Bool? + + /// Whether the refetch that has to land before the edit sheet opens is out. + @State private var isPreparingEdit = false + // MARK: - Local-only State @State private var localPlaylist: LocalPlaylist? @@ -152,6 +158,18 @@ struct UnifiedPlaylistDetailView: View { return appEnvironment?.accountsManager.capabilities(for: owningAccount)?.supportsPlaylistPrivacy == true } + /// The playlist's visibility, as far as the server said it. + /// + /// Invidious answers with a single "listed" flag, so `true` is exactly + /// public and `false` is unlisted *or* private with no way to tell which. + /// The form starts the picker at the conservative reading of `false` and + /// only sends a visibility the user actually chose, so the ambiguity never + /// turns into a change nobody asked for. + private var knownRemotePrivacy: PlaylistPrivacy? { + guard let remoteIsListed else { return nil } + return remoteIsListed ? .public : .private + } + /// Deleting a server-side playlist takes it off the server, which is a very /// different sentence from deleting a local one. private var deleteConfirmationTitle: String { @@ -232,10 +250,12 @@ struct UnifiedPlaylistDetailView: View { loadLocalPlaylist() } } else if canEditRemotePlaylist { - // The fetch carries no visibility, so the picker starts at the - // conservative default rather than pretending to know. PlaylistFormSheet( - mode: .editRemote(title: title, description: descriptionText, privacy: nil), + mode: .editRemote( + title: title, + description: descriptionText, + privacy: knownRemotePrivacy + ), showsPrivacy: remoteSupportsPrivacy ) { newTitle, newDescription, newPrivacy in Task { @@ -515,10 +535,11 @@ struct UnifiedPlaylistDetailView: View { // account owns. if (isLocal && localPlaylist != nil) || canEditRemotePlaylist { Button { - showingEditSheet = true + beginEdit() } label: { Label(String(localized: "playlist.edit"), systemImage: "pencil") } + .disabled(isPreparingEdit) Button(role: .destructive) { showingDeleteConfirmation = true @@ -679,13 +700,20 @@ struct UnifiedPlaylistDetailView: View { // account owns. if (isLocal && localPlaylist != nil) || canEditRemotePlaylist { Button { - showingEditSheet = true + beginEdit() } label: { - Label(String(localized: "playlist.edit"), systemImage: "pencil") - .frame(maxWidth: .infinity, minHeight: 50) - .font(.headline) + Group { + if isPreparingEdit { + ProgressView() + } else { + Label(String(localized: "playlist.edit"), systemImage: "pencil") + } + } + .frame(maxWidth: .infinity, minHeight: 50) + .font(.headline) } .buttonStyle(.bordered) + .disabled(isPreparingEdit) Button { showingDeleteConfirmation = true @@ -740,7 +768,11 @@ struct UnifiedPlaylistDetailView: View { let indexColumnWidth: CGFloat = 32 #endif return LazyVStack(spacing: 0) { - ForEach(Array(videos.enumerated()), id: \.element.id) { index, video in + // Keyed by row rather than by video: a playlist may hold the same + // video twice, and two rows sharing an identity leave SwiftUI free + // to pair a row's view with the other one's index — which is what + // the removal closure is addressed by. + ForEach(Array(videos.enumerated()), id: \.offset) { index, video in VideoListRow( isLast: index == videos.count - 1, rowStyle: .regular, @@ -916,8 +948,14 @@ struct UnifiedPlaylistDetailView: View { private func resolveOwningAccount(playlistID: PlaylistID, instance: Instance) -> Account? { guard let appEnvironment else { return nil } + // Only if that account is on *this* instance. An id that came in over + // Handoff was minted on another device, where it may name an account + // this one has on a different server entirely — and resolving it would + // send the authenticated fetch somewhere the playlist does not live, + // with a credential that has nothing to do with it. if let accountID = source.accountID, - let account = appEnvironment.accountsManager.account(id: accountID) { + let account = appEnvironment.accountsManager.account(id: accountID), + appEnvironment.accountsManager.instance(for: account)?.id == instance.id { return account } @@ -941,6 +979,7 @@ struct UnifiedPlaylistDetailView: View { apply(snapshot.playlist) entries = snapshot.entries entriesByRow = Self.rowEntries(for: snapshot) + remoteIsListed = snapshot.isListed } /// Pairs each displayed row with the entry that can name it to the server. @@ -1077,7 +1116,53 @@ struct UnifiedPlaylistDetailView: View { } } - private func saveRemoteEdit(title newTitle: String, description: String?, privacy: PlaylistPrivacy) async { + /// Opens the edit form — over the playlist as the server has it *now*, when + /// the playlist is one. + /// + /// The form saves every field at once, so an edit started from a snapshot + /// taken minutes ago would put that snapshot's title and description back + /// over anything another client changed since. A local playlist is the only + /// copy there is and needs none of this. + private func beginEdit() { + guard !isLocal else { + showingEditSheet = true + return + } + Task { await beginRemoteEdit() } + } + + private func beginRemoteEdit() async { + guard let appEnvironment, + let account = owningAccount, + let playlistID = remotePlaylistID + else { return } + + isPreparingEdit = true + defer { isPreparingEdit = false } + + do { + try await appEnvironment.remotePlaylistsCache.refreshSnapshot(id: playlistID, for: account) + + // Nothing came back to edit: the account was cleared while this was + // in flight and its data went with it, exactly as on load. + guard let snapshot = appEnvironment.remotePlaylistsCache.snapshot( + id: playlistID, + for: account.id + ) else { return } + + apply(snapshot) + showingEditSheet = true + } catch { + // No sheet: editing from data known to be stale is the thing this + // refetch exists to prevent. + appEnvironment.toastManager.showError( + String(localized: "playlist.remote.refresh.error"), + subtitle: error.localizedDescription + ) + } + } + + private func saveRemoteEdit(title newTitle: String, description: String?, privacy: PlaylistPrivacy?) async { guard let appEnvironment, let account = owningAccount, let playlistID = remotePlaylistID diff --git a/Yattee/Views/Video/PlaylistSelectorSheet.swift b/Yattee/Views/Video/PlaylistSelectorSheet.swift index de69b7e6..41cf78a6 100644 --- a/Yattee/Views/Video/PlaylistSelectorSheet.swift +++ b/Yattee/Views/Video/PlaylistSelectorSheet.swift @@ -44,10 +44,19 @@ struct PlaylistSelectorSheet: View { /// Every account that can be added to, on every instance — not just the one /// currently selected as the data context. Adding a video to a playlist is /// not a browsing decision, and the user may well mean the other login. + /// + /// The same three things the write layer insists on, asked here so an + /// account that could only ever fail is not offered as somewhere to put a + /// video: the backend must do playlist writes, the instance must be one + /// this app still talks to, and there must be a credential to talk with. + /// Account *status* deliberately is not among them — a stale "rejected" + /// would hide a login that works, and a failed add says so with a toast. private var remoteDestinations: [Account] { guard let appEnvironment else { return [] } - return appEnvironment.accountsManager.accounts.filter { - appEnvironment.accountsManager.capabilities(for: $0)?.supportsPlaylistWrites == true + return appEnvironment.accountsManager.accounts.filter { account in + appEnvironment.accountsManager.capabilities(for: account)?.supportsPlaylistWrites == true + && appEnvironment.accountsManager.instance(for: account)?.isEnabled == true + && appEnvironment.accountCredentialsManager(for: account)?.isLoggedIn(for: account) == true } } @@ -302,7 +311,7 @@ struct PlaylistSelectorSheet: View { on account: Account, title: String, description: String?, - privacy: PlaylistPrivacy + privacy: PlaylistPrivacy? ) async { guard let appEnvironment else { return } diff --git a/YatteeTests/AccountServiceTests.swift b/YatteeTests/AccountServiceTests.swift index e3111832..895c0fd4 100644 --- a/YatteeTests/AccountServiceTests.swift +++ b/YatteeTests/AccountServiceTests.swift @@ -22,8 +22,9 @@ private actor ScriptedPlaylistTransport: RemotePlaylistTransport { enum Call: Equatable, Sendable { case userPlaylists(credential: String) case snapshot(id: String, credential: String) - case create(title: String, description: String?, privacy: PlaylistPrivacy?, credential: String) - case update(id: String, title: String, description: String?, privacy: PlaylistPrivacy?, credential: String) + case create(title: String, privacy: PlaylistPrivacy?, credential: String) + case rename(id: String, title: String, privacy: PlaylistPrivacy?, credential: String) + case setDescription(id: String, description: String, title: String, credential: String) case delete(id: String, credential: String) case addVideo(playlistID: String, videoID: String, credential: String) case remove(entryID: RemotePlaylistEntryID, credential: String) @@ -34,6 +35,11 @@ private actor ScriptedPlaylistTransport: RemotePlaylistTransport { /// Thrown by the next call of any kind, once set. var nextError: Error? + /// The 1-based index of the first call that throws ``nextError``. Everything + /// before it answers normally, which is how a write that lands and a refetch + /// that then fails are scripted in one run. + private var firstFailingCall = 1 + /// What ``userPlaylistSnapshot(id:instance:credential:)`` answers. var snapshot = RemotePlaylistSnapshot(playlist: Playlist(id: .global("pl"), title: "Playlist"), entries: []) @@ -45,10 +51,21 @@ private actor ScriptedPlaylistTransport: RemotePlaylistTransport { private var parked: [CheckedContinuation] = [] func setError(_ error: Error?) { nextError = error } + + /// Fails every call from `index` on, leaving the earlier ones alone. + func setError(_ error: Error, fromCall index: Int) { + nextError = error + firstFailingCall = index + } + func setSnapshot(_ snapshot: RemotePlaylistSnapshot) { self.snapshot = snapshot } func setPlaylists(_ playlists: [Playlist]) { self.playlists = playlists } func holdCalls() { holdsCallsOpen = true } + /// Stops parking *new* calls without releasing the ones already parked, so + /// a request can be left in flight while other work runs to completion. + func stopHoldingNewCalls() { holdsCallsOpen = false } + func release() { holdsCallsOpen = false let waiting = parked @@ -74,7 +91,7 @@ private actor ScriptedPlaylistTransport: RemotePlaylistTransport { await withCheckedContinuation { parked.append($0) } } - if let error = nextError { + if let error = nextError, calls.count >= firstFailingCall { throw error } } @@ -95,27 +112,33 @@ private actor ScriptedPlaylistTransport: RemotePlaylistTransport { func createPlaylist( title: String, - description: String?, privacy: PlaylistPrivacy?, instance _: Instance, credential: String ) async throws -> Playlist { - try await record( - .create(title: title, description: description, privacy: privacy, credential: credential) - ) - return Playlist(id: .global("new-playlist"), title: title, description: description) + try await record(.create(title: title, privacy: privacy, credential: credential)) + return Playlist(id: .global("new-playlist"), title: title) } - func updatePlaylist( + func renamePlaylist( id: String, title: String, - description: String?, privacy: PlaylistPrivacy?, instance _: Instance, credential: String + ) async throws { + try await record(.rename(id: id, title: title, privacy: privacy, credential: credential)) + } + + func setPlaylistDescription( + id: String, + description: String, + title: String, + instance _: Instance, + credential: String ) async throws { try await record( - .update(id: id, title: title, description: description, privacy: privacy, credential: credential) + .setDescription(id: id, description: description, title: title, credential: credential) ) } @@ -282,7 +305,7 @@ struct AccountServiceTests { ) #expect(await harness.transport.calls == [ - .create(title: "Watch later", description: nil, privacy: nil, credential: "secret-original") + .create(title: "Watch later", privacy: nil, credential: "secret-original") ]) } @@ -298,8 +321,119 @@ struct AccountServiceTests { for: harness.account ) + // Two requests, because no backend takes a description at creation. #expect(await harness.transport.calls == [ - .create(title: "Watch later", description: "Later", privacy: .unlisted, credential: "secret-original") + .create(title: "Watch later", privacy: .unlisted, credential: "secret-original"), + .setDescription( + id: "new-playlist", + description: "Later", + title: "Watch later", + credential: "secret-original" + ) + ]) + } + + // MARK: - Composite writes + + @Test("A credential replaced between a creation and its description stops the second call") + func creationStopsDescribingWhenTheCredentialChanges() async throws { + let harness = AccountServiceHarness(type: .invidious) + defer { harness.tearDown() } + await harness.transport.holdCalls() + + let creation = Task { [service = harness.service!, account = harness.account] in + try await service.createPlaylist( + title: "Watch later", + description: "Later", + privacy: .private, + for: account + ) + } + + // The creation is out. Somebody signs out and back in as another user + // before it answers — the description must not go to the new login. + try await harness.transport.waitForCalls(1) + harness.credential = "secret-somebody-else" + await harness.transport.release() + + await #expect(throws: AccountServiceError.notAuthenticated) { + _ = try await creation.value + } + + #expect(await harness.transport.calls == [ + .create(title: "Watch later", privacy: .private, credential: "secret-original") + ]) + } + + @Test("An edit sends the description only after rechecking the credential") + func editRechecksBetweenRenameAndDescription() async throws { + let harness = AccountServiceHarness(type: .piped) + defer { harness.tearDown() } + await harness.transport.holdCalls() + + let edit = Task { [service = harness.service!, account = harness.account] in + try await service.updatePlaylist( + id: "pl", + title: "Renamed", + description: "New description", + privacy: nil, + for: account + ) + } + + try await harness.transport.waitForCalls(1) + harness.credential = "secret-somebody-else" + await harness.transport.release() + + await #expect(throws: AccountServiceError.notAuthenticated) { + try await edit.value + } + + // The rename landed under the old login; the description never went. + #expect(await harness.transport.calls == [ + .rename(id: "pl", title: "Renamed", privacy: nil, credential: "secret-original") + ]) + } + + @Test("An edit that keeps its credential renames and then describes") + func editSendsBothHalvesWithTheSameCredential() async throws { + let harness = AccountServiceHarness(type: .invidious) + defer { harness.tearDown() } + + try await harness.service.updatePlaylist( + id: "pl", + title: "Renamed", + description: "New description", + privacy: .public, + for: harness.account + ) + + #expect(await harness.transport.calls == [ + .rename(id: "pl", title: "Renamed", privacy: .public, credential: "secret-original"), + .setDescription( + id: "pl", + description: "New description", + title: "Renamed", + credential: "secret-original" + ) + ]) + } + + @Test("An edit that names no visibility sends none, rather than inventing one") + func editWithoutPrivacySendsNone() async throws { + let harness = AccountServiceHarness(type: .invidious) + defer { harness.tearDown() } + + try await harness.service.updatePlaylist( + id: "pl", + title: "Renamed", + description: nil, + privacy: nil, + for: harness.account + ) + + #expect(await harness.transport.calls == [ + .rename(id: "pl", title: "Renamed", privacy: nil, credential: "secret-original") ]) } @@ -594,6 +728,56 @@ struct RemotePlaylistsCacheTests { #expect(cache.playlists(for: harness.account.id)?.contains { $0.id.playlistID == "pl" } != true) } + @Test("A refresh in flight across a delete cannot bring the playlist back") + func inFlightRefreshCannotResurrectADeletedPlaylist() async throws { + let harness = AccountServiceHarness() + defer { harness.tearDown() } + let cache = harness.makeCache() + await harness.transport.setSnapshot(makeSnapshot(playlistID: "pl", videoIDs: ["aaa"], kind: .invidious)) + await harness.transport.setPlaylists([Playlist(id: .global("pl"), title: "Playlist")]) + await harness.transport.holdCalls() + + let refresh = Task { [account = harness.account] in + try await cache.refreshSnapshot(id: "pl", for: account) + } + try await harness.transport.waitForCalls(1) + + // That fetch is parked mid-flight. The delete runs to completion around + // it, and the server stops listing the playlist. + await harness.transport.stopHoldingNewCalls() + await harness.transport.setPlaylists([]) + try await cache.deletePlaylist(id: "pl", for: harness.account) + + // Only now does the older fetch answer. It describes a playlist that + // existed when it was asked for, and arriving late does not bring it + // back. + await harness.transport.release() + _ = try? await refresh.value + + #expect(cache.snapshot(id: "pl", for: harness.account.id) == nil) + #expect(cache.playlists(for: harness.account.id)?.isEmpty == true) + } + + @Test("A delete that landed drops the list entry even when the refetch fails") + func successfulDeleteForgetsTheListEntryWhenTheRefetchFails() async throws { + let harness = AccountServiceHarness() + defer { harness.tearDown() } + let cache = harness.makeCache() + await harness.transport.setSnapshot(makeSnapshot(playlistID: "pl", videoIDs: ["aaa"], kind: .invidious)) + await harness.transport.setPlaylists([Playlist(id: .global("pl"), title: "Playlist")]) + + try await cache.refreshSnapshot(id: "pl", for: harness.account) + try await cache.refreshList(for: harness.account) + + // The delete itself lands; the reconciling list refetch right after it + // cannot reach the server. + await harness.transport.setError(APIError.noConnection, fromCall: 4) + try await cache.deletePlaylist(id: "pl", for: harness.account) + + #expect(cache.snapshot(id: "pl", for: harness.account.id) == nil) + #expect(cache.playlists(for: harness.account.id)?.isEmpty == true) + } + @Test("A delete that could not reach the server keeps the playlist it knows") func failedDeleteKeepsThePlaylistWhenOffline() async throws { let harness = AccountServiceHarness() diff --git a/YatteeTests/RemotePlaylistEntryTests.swift b/YatteeTests/RemotePlaylistEntryTests.swift index 0ad49078..0202b46d 100644 --- a/YatteeTests/RemotePlaylistEntryTests.swift +++ b/YatteeTests/RemotePlaylistEntryTests.swift @@ -46,9 +46,11 @@ private func invidiousVideoJSON( private func invidiousPageJSON( playlistID: String = "IVPLtest", videoCount: Int, - videos: [String] + videos: [String], + isListed: Bool? = nil ) -> Data { - """ + let listedField = isListed.map { ",\n \"isListed\": \($0)" } ?? "" + return """ { "type": "invidiousPlaylist", "title": "Watch Later", @@ -56,7 +58,7 @@ private func invidiousPageJSON( "author": "arek", "authorId": "UCowner", "description": "A private playlist", - "videoCount": \(videoCount), + "videoCount": \(videoCount)\(listedField), "videos": [\(videos.joined(separator: ", "))] } """.data(using: .utf8)! @@ -321,6 +323,25 @@ struct InvidiousRemotePlaylistSnapshotTests { #expect(snapshot.playlist.id.playlistID == "IVPLtest") } + @Test("The listed flag is carried through, and its absence is not a no") + func snapshotCarriesVisibility() throws { + func snapshot(isListed: Bool?) throws -> RemotePlaylistSnapshot { + try InvidiousAPI.authPlaylistSnapshot( + fromPageJSON: [invidiousPageJSON(videoCount: 0, videos: [], isListed: isListed)], + playlistID: "IVPLtest", + baseURL: testBaseURL + ) + } + + // `true` is exactly public. `false` is unlisted *or* private — Invidious + // serialises `privacy.public?` and nothing else — and a page that says + // nothing leaves the visibility unknown rather than not-listed, which is + // what stops an edit from sending a guess back. + #expect(try snapshot(isListed: true).isListed == true) + #expect(try snapshot(isListed: false).isListed == false) + #expect(try snapshot(isListed: nil).isListed == nil) + } + @Test("An empty page set is a decoding failure, not an empty playlist") func emptyPageSetThrows() { #expect(throws: APIError.self) {