From 0c5745e4fbcb0f770fd575247a31ca8914d54f30 Mon Sep 17 00:00:00 2001 From: Arkadiusz Fal Date: Fri, 4 Sep 2026 23:18:04 +0200 Subject: [PATCH] Let a subscribe name the account it lands on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subscribing went wherever the selected account happened to be, and the button offering it read the local store first — so under a server account it claimed a channel was subscribed when only the device knew about it. The button now asks the active provider and nothing else, except under a local context, where the local store is that provider and can answer without a hop. The channel menu gains a destination list: local, plus every account whose backend does subscriptions, whose instance is switched on, and which has a credential on this device. A row names its own target, so it deliberately does not care whether the selected context can be written to — that is precisely the case where naming somewhere else is the only way to subscribe at all. The main button is disabled instead, with the reason beside it. tvOS reaches the same list through its own button and a full-screen picker, because a menu cannot be driven with a remote. Membership is asked for only once the rows are on screen: an unwarmed provider answers by fetching that account's entire subscription list, and asking every destination up front is a burst nobody requested. Answers carry the round they were asked under, and "not asked yet" is kept apart from "asked, not subscribed" — a checkmark the user acts on must not be a guess. SubscriptionService grows the destination-parameterized subscribe, unsubscribe and membership calls the menu writes through, with the context-implicit ones left as wrappers over the current context. A membership answer that outlived its login now says nothing rather than "no". Building a provider moves behind SubscriptionProviderFactory, which is what lets a test see which destination a write was routed to without a server behind it. The unsubscribe-only screens are untouched: they act on the account being browsed, which is the account whose list they are showing. --- Yattee/Core/AppEnvironment.swift | 13 +- Yattee/Localizable.xcstrings | 22 ++ Yattee/Services/SubscriptionService.swift | 147 ++++---- .../SubscriptionProviderFactory.swift | 61 ++++ Yattee/Views/Channel/ChannelView.swift | 128 +++++-- .../SubscriptionDestinationMenu.swift | 265 ++++++++++++++ .../SubscriptionDestinationTests.swift | 332 ++++++++++++++++++ 7 files changed, 859 insertions(+), 109 deletions(-) create mode 100644 Yattee/Services/Subscriptions/SubscriptionProviderFactory.swift create mode 100644 Yattee/Views/Components/SubscriptionDestinationMenu.swift create mode 100644 YatteeTests/SubscriptionDestinationTests.swift diff --git a/Yattee/Core/AppEnvironment.swift b/Yattee/Core/AppEnvironment.swift index 3a6c4beb..ac7be099 100644 --- a/Yattee/Core/AppEnvironment.swift +++ b/Yattee/Core/AppEnvironment.swift @@ -285,12 +285,13 @@ final class AppEnvironment { // Initialize SubscriptionService with all required dependencies self.subscriptionService = SubscriptionService( dataManager: dm, - settingsManager: settings, - instancesManager: instances, - invidiousCredentialsManager: invidiousCreds, - pipedCredentialsManager: pipedCreds, - invidiousAPI: invidiousAPI, - pipedAPI: pipedAPI + providerFactory: DefaultSubscriptionProviderFactory( + invidiousAPI: invidiousAPI, + pipedAPI: pipedAPI, + invidiousCredentialsManager: invidiousCreds, + pipedCredentialsManager: pipedCreds, + settingsManager: settings + ) ) subscriptionService.setAccountsManager(accounts) diff --git a/Yattee/Localizable.xcstrings b/Yattee/Localizable.xcstrings index 127ffd76..b637d394 100644 --- a/Yattee/Localizable.xcstrings +++ b/Yattee/Localizable.xcstrings @@ -743,6 +743,17 @@ } } }, + "channel.menu.subscribeOn" : { + "comment" : "Submenu listing the accounts a channel can be subscribed on", + "localizations" : { + "en" : { + "stringUnit" : { + "state" : "translated", + "value" : "Subscribe On…" + } + } + } + }, "channel.menu.unsubscribe" : { "localizations" : { "en" : { @@ -814,6 +825,17 @@ } } }, + "channel.subscribe.contextUnavailable" : { + "comment" : "Menu hint when the selected account cannot be subscribed on right now", + "localizations" : { + "en" : { + "stringUnit" : { + "state" : "translated", + "value" : "The selected account is unavailable. Pick a destination below." + } + } + } + }, "channel.subscribe.error.title" : { "comment" : "Toast title when subscription fails", "localizations" : { diff --git a/Yattee/Services/SubscriptionService.swift b/Yattee/Services/SubscriptionService.swift index 25bf6d5c..d0dc5c6a 100644 --- a/Yattee/Services/SubscriptionService.swift +++ b/Yattee/Services/SubscriptionService.swift @@ -37,12 +37,7 @@ final class SubscriptionService { // MARK: - Dependencies private let dataManager: DataManager - private let settingsManager: SettingsManager - private let instancesManager: InstancesManager - private let invidiousCredentialsManager: InvidiousCredentialsManager - private let pipedCredentialsManager: PipedCredentialsManager - private let invidiousAPI: InvidiousAPI - private let pipedAPI: PipedAPI + private let providerFactory: any SubscriptionProviderFactory // MARK: - Providers @@ -67,20 +62,10 @@ final class SubscriptionService { init( dataManager: DataManager, - settingsManager: SettingsManager, - instancesManager: InstancesManager, - invidiousCredentialsManager: InvidiousCredentialsManager, - pipedCredentialsManager: PipedCredentialsManager, - invidiousAPI: InvidiousAPI, - pipedAPI: PipedAPI + providerFactory: any SubscriptionProviderFactory ) { self.dataManager = dataManager - self.settingsManager = settingsManager - self.instancesManager = instancesManager - self.invidiousCredentialsManager = invidiousCredentialsManager - self.pipedCredentialsManager = pipedCredentialsManager - self.invidiousAPI = invidiousAPI - self.pipedAPI = pipedAPI + self.providerFactory = providerFactory // Account providers are built lazily: which accounts exist is not known // until AccountsManager has loaded and iCloud has delivered instances. @@ -138,27 +123,11 @@ final class SubscriptionService { instance.type.accountCapabilities.supportsSubscriptions else { return nil } - let provider: (any SubscriptionProvider)? - switch instance.type { - case .invidious: - provider = InvidiousSubscriptionProvider( - account: account, - invidiousAPI: invidiousAPI, - credentialsManager: invidiousCredentialsManager, - accountsManager: accountsManager, - settingsManager: settingsManager - ) - case .piped: - provider = PipedSubscriptionProvider( - account: account, - pipedAPI: pipedAPI, - credentialsManager: pipedCredentialsManager, - accountsManager: accountsManager, - settingsManager: settingsManager - ) - default: - provider = nil - } + let provider = providerFactory.provider( + for: account, + instance: instance, + accountsManager: accountsManager + ) accountProviders[id] = provider return provider @@ -220,92 +189,116 @@ final class SubscriptionService { // MARK: - Subscribe - /// Subscribes to a channel using the current provider. - /// - Parameter channel: The channel to subscribe to. + /// Subscribes to a channel on a named destination. + /// + /// The context is passed in rather than read: a destination the user picked + /// names its own target, so it must keep working while the *selected* + /// context is unusable. /// - Throws: SubscriptionProviderError if the operation fails. - func subscribe(to channel: Channel) async throws { - guard let provider = currentProvider else { + func subscribe(to channel: Channel, in context: DataContext) async throws { + guard let provider = provider(for: context) else { throw SubscriptionError.providerNotAvailable } isLoading = true lastError = nil + // The provider is captured before the await, so the write lands where + // it was aimed even if the logins are replaced meanwhile. The service's + // error cannot be treated the same way: it is shared by every caller, + // and one raised for a login that has since been replaced would be read + // as the current one's. + let generation = providerGeneration + + defer { isLoading = false } + do { try await provider.subscribe(to: channel) - postChangeNotificationForServerAccount() + postChangeNotification(for: context) LoggingService.shared.info( - "Subscribed to \(channel.name) via \(provider.context)", + "Subscribed to \(channel.name) via \(context)", category: .general ) } catch { - lastError = error + if generation == providerGeneration { lastError = error } LoggingService.shared.error( "Failed to subscribe to \(channel.name): \(error.localizedDescription)", category: .general ) throw error } - - isLoading = false } - /// Subscribes to a channel from an Author. - /// - Parameters: - /// - author: The author/channel to subscribe to. - /// - source: The content source. - func subscribe(to author: Author, source: ContentSource) async throws { - let channel = Channel( - id: ChannelID(source: source, channelID: author.id), - name: author.name, - subscriberCount: author.subscriberCount, - thumbnailURL: author.thumbnailURL - ) - try await subscribe(to: channel) + /// Subscribes to a channel using the current provider. + /// - Parameter channel: The channel to subscribe to. + /// - Throws: SubscriptionProviderError if the operation fails. + func subscribe(to channel: Channel) async throws { + try await subscribe(to: channel, in: currentContext) } // MARK: - Unsubscribe - /// Unsubscribes from a channel using the current provider. - /// - Parameter channelID: The channel ID to unsubscribe from. + /// Unsubscribes from a channel on a named destination. /// - Throws: SubscriptionProviderError if the operation fails. - func unsubscribe(from channelID: String) async throws { - guard let provider = currentProvider else { + func unsubscribe(from channelID: String, in context: DataContext) async throws { + guard let provider = provider(for: context) else { throw SubscriptionError.providerNotAvailable } isLoading = true lastError = nil + let generation = providerGeneration + + defer { isLoading = false } + do { try await provider.unsubscribe(from: channelID) - postChangeNotificationForServerAccount() + postChangeNotification(for: context) LoggingService.shared.info( - "Unsubscribed from \(channelID) via \(provider.context)", + "Unsubscribed from \(channelID) via \(context)", category: .general ) } catch { - lastError = error + if generation == providerGeneration { lastError = error } LoggingService.shared.error( "Failed to unsubscribe from \(channelID): \(error.localizedDescription)", category: .general ) throw error } + } - isLoading = false + /// Unsubscribes from a channel using the current provider. + /// - Parameter channelID: The channel ID to unsubscribe from. + /// - Throws: SubscriptionProviderError if the operation fails. + func unsubscribe(from channelID: String) async throws { + try await unsubscribe(from: channelID, in: currentContext) } // MARK: - Query + /// Whether a channel is subscribed on a named destination. + /// + /// Answers nil rather than false when nothing can be said: the destination + /// has no provider, or the logins were replaced while the answer was in + /// flight. A menu showing "not subscribed" for either would be a lie the + /// user could act on. + func isSubscribed(to channelID: String, in context: DataContext) async -> Bool? { + guard let provider = provider(for: context) else { return nil } + + let generation = providerGeneration + let subscribed = await provider.isSubscribed(to: channelID) + + guard generation == providerGeneration else { return nil } + return subscribed + } + /// Checks if subscribed to a channel. /// - Parameter channelID: The channel ID to check. /// - Returns: `true` if subscribed, `false` otherwise. func isSubscribed(to channelID: String) async -> Bool { - guard let provider = readProvider else { - return false - } - return await provider.isSubscribed(to: channelID) + await isSubscribed(to: channelID, in: contextValidator?.effectiveContext ?? currentContext) ?? false } /// Fetches all subscriptions from the current provider. @@ -419,7 +412,7 @@ final class SubscriptionService { } if imported > 0 { - postChangeNotificationForServerAccount() + postChangeNotification(for: currentContext) } return (imported, skipped) @@ -429,8 +422,12 @@ final class SubscriptionService { /// Posts a subscriptions-changed notification for server accounts. /// Local accounts already post it from DataManager with a change payload. - private func postChangeNotificationForServerAccount() { - guard !currentContext.isLocal else { return } + /// + /// Takes the context the write went to, not the selected one: a subscribe + /// aimed at another account still changed something, and the views that + /// listen decide for themselves whether it concerns them. + private func postChangeNotification(for context: DataContext) { + guard !context.isLocal else { return } NotificationCenter.default.post(name: .subscriptionsDidChange, object: nil) } } diff --git a/Yattee/Services/Subscriptions/SubscriptionProviderFactory.swift b/Yattee/Services/Subscriptions/SubscriptionProviderFactory.swift new file mode 100644 index 00000000..563c2432 --- /dev/null +++ b/Yattee/Services/Subscriptions/SubscriptionProviderFactory.swift @@ -0,0 +1,61 @@ +// +// SubscriptionProviderFactory.swift +// Yattee +// +// Builds the provider that serves one account's subscriptions. +// + +import Foundation + +/// Makes the provider for an account. +/// +/// A seam rather than a switch inside `SubscriptionService`: the provider is +/// what a destination write actually lands on, so which one a context resolves +/// to has to be observable without a server behind it. +@MainActor +protocol SubscriptionProviderFactory { + /// The provider for an account on a resolved, subscription-capable + /// instance, or nil when the backend has no subscriptions API. + func provider( + for account: Account, + instance: Instance, + accountsManager: AccountsManager + ) -> (any SubscriptionProvider)? +} + +/// The app's factory: one provider per backend that has an accounts API. +@MainActor +struct DefaultSubscriptionProviderFactory: SubscriptionProviderFactory { + let invidiousAPI: InvidiousAPI + let pipedAPI: PipedAPI + let invidiousCredentialsManager: InvidiousCredentialsManager + let pipedCredentialsManager: PipedCredentialsManager + let settingsManager: SettingsManager + + func provider( + for account: Account, + instance: Instance, + accountsManager: AccountsManager + ) -> (any SubscriptionProvider)? { + switch instance.type { + case .invidious: + return InvidiousSubscriptionProvider( + account: account, + invidiousAPI: invidiousAPI, + credentialsManager: invidiousCredentialsManager, + accountsManager: accountsManager, + settingsManager: settingsManager + ) + case .piped: + return PipedSubscriptionProvider( + account: account, + pipedAPI: pipedAPI, + credentialsManager: pipedCredentialsManager, + accountsManager: accountsManager, + settingsManager: settingsManager + ) + default: + return nil + } + } +} diff --git a/Yattee/Views/Channel/ChannelView.swift b/Yattee/Views/Channel/ChannelView.swift index 4d988249..163d0bbc 100644 --- a/Yattee/Views/Channel/ChannelView.swift +++ b/Yattee/Views/Channel/ChannelView.swift @@ -28,9 +28,11 @@ struct ChannelView: View { @State private var isLoading = true @State private var isLoadingMore = false @State private var errorMessage: String? - @State private var subscription: Subscription? @State private var isSubscribed = false @State private var showingUnsubscribeConfirmation = false + #if os(tvOS) + @State private var showingSubscriptionDestinations = false + #endif @State private var scrollOffset: CGFloat = 0 @State private var scrollToTop: Bool = false @State private var cachedHeader: CachedChannelData? @@ -727,6 +729,7 @@ struct ChannelView: View { if !isDescriptionScrollLocked { tvOSSubscribeButton + tvOSSubscriptionDestinationsButton } if let description, !description.isEmpty { @@ -789,9 +792,33 @@ struct ChannelView: View { .font(.headline) } .buttonStyle(.borderedProminent) + .disabled(!allowsCurrentContextMutation) .focused($isSubscribeFocused) } + /// The destination list, as its own button: a menu is unusable with a + /// remote, and the primary tap has to stay the current account's toggle. + @ViewBuilder + private var tvOSSubscriptionDestinationsButton: some View { + if let channelToSubscribe, SubscriptionDestinations.areOffered(in: appEnvironment) { + Button { + showingSubscriptionDestinations = true + } label: { + Label( + String(localized: "channel.menu.subscribeOn"), + systemImage: "person.crop.circle.badge.plus" + ) + .frame(maxWidth: .infinity, minHeight: 60) + } + .buttonStyle(.bordered) + .fullScreenCover(isPresented: $showingSubscriptionDestinations) { + SubscriptionDestinationPicker(channel: channelToSubscribe) { context, subscribed in + destinationDidToggle(context, subscribed: subscribed) + } + } + } + } + private var tvOSTabButtons: some View { HStack(spacing: 12) { ForEach([ChannelTab.videos, .shorts, .streams, .playlists]) { tab in @@ -1197,6 +1224,23 @@ struct ChannelView: View { systemImage: isSubscribed ? "person.fill.xmark" : "person.badge.plus" ) } + .disabled(!allowsCurrentContextMutation) + + if !allowsCurrentContextMutation { + Text(String(localized: "channel.subscribe.contextUnavailable")) + } + + // Naming a destination is a different decision from the one the + // main button makes, so it survives an unusable current context. + // tvOS reaches the same list through its own button — this menu is + // never presented there. + #if !os(tvOS) + if let channelToSubscribe { + SubscriptionDestinationMenu(channel: channelToSubscribe) { context, subscribed in + destinationDidToggle(context, subscribed: subscribed) + } + } + #endif // Notifications toggle (only visible when subscribed) if isSubscribed { @@ -1222,6 +1266,29 @@ struct ChannelView: View { // MARK: - Subscription Actions + /// Whether the main button may write to the account being browsed. + /// + /// Local always may. A server account may only while it is usable: a + /// subscribe that cannot reach its account must be refused rather than + /// land in local data the user did not choose. + private var allowsCurrentContextMutation: Bool { + guard let appEnvironment else { return false } + guard !appEnvironment.accountsManager.currentContext.isLocal else { return true } + return appEnvironment.dataContextValidator.allowsServerMutations + } + + /// The channel as it must be stored: the id carries the source this view + /// was opened for, which an extracted channel does not report itself. + private var channelToSubscribe: Channel? { + guard let channel else { return nil } + return Channel( + id: ChannelID(source: source, channelID: channel.id.channelID), + name: channel.name, + subscriberCount: channel.subscriberCount, + thumbnailURL: channel.thumbnailURL + ) + } + private func toggleSubscription() { if isSubscribed { showingUnsubscribeConfirmation = true @@ -1231,26 +1298,19 @@ struct ChannelView: View { } private func subscribe() async { - guard let channel, + guard let channelToSubscribe, let subscriptionService = appEnvironment?.subscriptionService, let dataManager = appEnvironment?.dataManager else { return } - let author = Author( - id: channel.id.channelID, - name: channel.name, - thumbnailURL: channel.thumbnailURL, - subscriberCount: channel.subscriberCount - ) - do { - try await subscriptionService.subscribe(to: author, source: source) - + try await subscriptionService.subscribe(to: channelToSubscribe) + // Set default notification preference for new subscription let defaultNotifications = appEnvironment?.settingsManager.defaultNotificationsForNewChannels ?? false if defaultNotifications { - dataManager.setNotificationsEnabled(true, for: channel.id.channelID) + dataManager.setNotificationsEnabled(true, for: channelToSubscribe.id.channelID) } - + isSubscribed = true refreshSubscription() } catch { @@ -1261,6 +1321,21 @@ struct ChannelView: View { } } + /// A destination row toggled something. Only the account on screen changes + /// what the main button says, and notification preferences are local and + /// keyed by channel, so they follow the same rule. + private func destinationDidToggle(_ context: DataContext, subscribed: Bool) { + guard context == appEnvironment?.accountsManager.currentContext else { return } + + if subscribed, + appEnvironment?.settingsManager.defaultNotificationsForNewChannels == true, + let channelID = channelToSubscribe?.id.channelID { + appEnvironment?.dataManager.setNotificationsEnabled(true, for: channelID) + } + + refreshSubscription() + } + private func unsubscribe() { guard let channel else { return } @@ -1268,7 +1343,6 @@ struct ChannelView: View { do { try await appEnvironment?.subscriptionService.unsubscribe(from: channel.id.channelID) isSubscribed = false - subscription = nil } catch { appEnvironment?.toastManager.showError( String(localized: "channel.unsubscribe.error.title"), @@ -2097,13 +2171,7 @@ struct ChannelView: View { isLoading = true errorMessage = nil - // Load subscription state: optimistic from the local store, then - // corrected by the active provider (server accounts differ from local) - subscription = appEnvironment.dataManager.subscription(for: channelID) - isSubscribed = subscription != nil - Task { - isSubscribed = await appEnvironment.subscriptionService.isSubscribed(to: channelID) - } + refreshSubscription() // Load cached header data for immediate display cachedHeader = CachedChannelData.load(for: channelID, using: appEnvironment.dataManager) @@ -2214,11 +2282,7 @@ struct ChannelView: View { isLoading = false // Check subscription status using extracted channel ID - subscription = appEnvironment.dataManager.subscription(for: fetchedChannel.id.channelID) - isSubscribed = subscription != nil - Task { - isSubscribed = await appEnvironment.subscriptionService.isSubscribed(to: fetchedChannel.id.channelID) - } + refreshSubscription() } } catch let error as APIError { await MainActor.run { @@ -2248,12 +2312,20 @@ struct ChannelView: View { } } + /// Re-reads the button's state from the account being browsed. + /// + /// The local store is only consulted when it *is* the active context: under + /// a server account it holds another account's channels, and reading it + /// first would show the wrong button until the provider answered. private func refreshSubscription() { // Use loaded channel's ID if available (important for external channels // where navigation channelID is the URL, not the actual channel ID) let effectiveChannelID = channel?.id.channelID ?? channelID - subscription = appEnvironment?.dataManager.subscription(for: effectiveChannelID) - isSubscribed = subscription != nil + + if appEnvironment?.dataContextValidator.effectiveContext.isLocal == true { + isSubscribed = appEnvironment?.dataManager.subscription(for: effectiveChannelID) != nil + } + Task { if let service = appEnvironment?.subscriptionService { isSubscribed = await service.isSubscribed(to: effectiveChannelID) diff --git a/Yattee/Views/Components/SubscriptionDestinationMenu.swift b/Yattee/Views/Components/SubscriptionDestinationMenu.swift new file mode 100644 index 00000000..2f6244d7 --- /dev/null +++ b/Yattee/Views/Components/SubscriptionDestinationMenu.swift @@ -0,0 +1,265 @@ +// +// SubscriptionDestinationMenu.swift +// Yattee +// +// Subscribing somewhere other than the account currently being browsed. +// + +import SwiftUI + +// MARK: - Membership + +/// What is known about a channel's subscription on each destination. +/// +/// "Not asked yet" and "asked, not subscribed" are different rows, so the +/// unknown state is kept rather than collapsed into `false`. Answers carry the +/// round they were asked under: a channel change or a second look at the menu +/// starts a new round, and a reply from the old one is dropped instead of being +/// shown against the wrong channel. +struct SubscriptionDestinationMembership: Equatable { + private var known: [DataContext: Bool] = [:] + private(set) var generation = 0 + + /// Forgets every answer and opens a new round. + mutating func invalidate() { + known.removeAll() + generation += 1 + } + + /// Records an answer from `generation`. + /// + /// A `nil` answer says nothing — the destination has no provider, or the + /// logins were replaced while it was in flight — and must not be stored as + /// "not subscribed". + mutating func record(_ isSubscribed: Bool?, for context: DataContext, generation: Int) { + guard generation == self.generation, let isSubscribed else { return } + known[context] = isSubscribed + } + + /// nil while nothing is known, which renders as neither state. + func isSubscribed(on context: DataContext) -> Bool? { known[context] } +} + +// MARK: - Destinations + +/// Where a channel can be subscribed. +enum SubscriptionDestinations { + /// Local plus every account a subscribe could actually land on. + /// + /// The same three things the write layer insists on — the backend must do + /// subscriptions, 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 write says so with a toast. Neither is the selected context's + /// availability — a row names its own target, so it keeps working when the + /// account being browsed cannot be reached. + @MainActor + static func available(in appEnvironment: AppEnvironment?) -> [DataContext] { + guard let appEnvironment else { return [.local] } + + let accounts = appEnvironment.accountsManager.accounts.filter { account in + appEnvironment.accountsManager.capabilities(for: account)?.supportsSubscriptions == true + && appEnvironment.accountsManager.instance(for: account)?.isEnabled == true + && appEnvironment.accountCredentialsManager(for: account)?.isLoggedIn(for: account) == true + } + + return [.local] + accounts.map { DataContext.account($0.id) } + } + + /// Whether naming a destination is a choice worth offering. + /// + /// With only local to offer there is normally nothing the main subscribe + /// button does not already do — unless that button is refusing, because the + /// selected account cannot be written to. Local is then exactly the + /// alternative the user is looking for. + @MainActor + static func areOffered(in appEnvironment: AppEnvironment?) -> Bool { + if available(in: appEnvironment).count > 1 { return true } + + guard let appEnvironment else { return false } + return !appEnvironment.accountsManager.currentContext.isLocal + && !appEnvironment.dataContextValidator.allowsServerMutations + } +} + +// MARK: - Rows + +/// One row per place the channel can be subscribed, with the membership known +/// there. +/// +/// Shared by the menu and the tvOS picker so a destination that is offered in +/// one is offered — and toggled — identically in the other. +struct SubscriptionDestinationRows: View { + @Environment(\.appEnvironment) private var appEnvironment + + /// The channel as it must be stored, with the id the calling view resolved. + let channel: Channel + + /// The destination that was toggled and whether it is now subscribed, for + /// the view that owns the main subscribe button. + var onToggled: ((DataContext, Bool) -> Void)? + + /// Called after any tap, for presentations that have to close themselves. + var onSelect: (() -> Void)? + + @State private var membership = SubscriptionDestinationMembership() + @State private var busy: Set = [] + + var body: some View { + ForEach(destinations, id: \.self) { context in + Button { + toggle(context) + onSelect?() + } label: { + rowLabel(for: context) + } + .disabled(busy.contains(context)) + #if os(tvOS) + .buttonStyle(TVSettingsButtonStyle()) + #endif + } + // Membership is only asked for once the rows are on screen: an unwarmed + // provider answers by fetching that account's whole subscription list, + // which is far too much to spend on a menu nobody opened. + .task(id: channel.id) { + await loadMembership() + } + } + + @ViewBuilder + private func rowLabel(for context: DataContext) -> some View { + let title = DataContextSwitcherLabels.title(for: context, in: appEnvironment) + + if membership.isSubscribed(on: context) == true { + Label(title, systemImage: "checkmark") + } else { + Text(title) + } + } + + private var destinations: [DataContext] { + SubscriptionDestinations.available(in: appEnvironment) + } + + // MARK: - Membership + + private func loadMembership() async { + guard let service = appEnvironment?.subscriptionService else { return } + + membership.invalidate() + let generation = membership.generation + let channelID = channel.id.channelID + + // Serially rather than in a group: each cold destination answers by + // fetching its account's whole list, and firing every instance at once + // is a burst the user never asked for. + for context in destinations { + let subscribed = await service.isSubscribed(to: channelID, in: context) + membership.record(subscribed, for: context, generation: generation) + } + } + + // MARK: - Toggle + + private func toggle(_ context: DataContext) { + guard let appEnvironment else { return } + + // Unknown counts as not subscribed: the row shows no checkmark, so + // subscribing is what the user just asked for. + let wasSubscribed = membership.isSubscribed(on: context) == true + let generation = membership.generation + let channelID = channel.id.channelID + + busy.insert(context) + + Task { + do { + if wasSubscribed { + try await appEnvironment.subscriptionService.unsubscribe(from: channelID, in: context) + } else { + try await appEnvironment.subscriptionService.subscribe(to: channel, in: context) + } + + membership.record(!wasSubscribed, for: context, generation: generation) + onToggled?(context, !wasSubscribed) + } catch { + appEnvironment.toastManager.showError( + String( + localized: wasSubscribed + ? "channel.unsubscribe.error.title" + : "channel.subscribe.error.title" + ), + subtitle: error.localizedDescription + ) + } + + busy.remove(context) + } + } +} + +// MARK: - Menu + +#if !os(tvOS) +/// The destination submenu, for placing inside the channel menu. +struct SubscriptionDestinationMenu: View { + @Environment(\.appEnvironment) private var appEnvironment + + let channel: Channel + var onToggled: ((DataContext, Bool) -> Void)? + + var body: some View { + if SubscriptionDestinations.areOffered(in: appEnvironment) { + Menu { + SubscriptionDestinationRows(channel: channel, onToggled: onToggled) + } label: { + Label( + String(localized: "channel.menu.subscribeOn"), + systemImage: "person.crop.circle.badge.plus" + ) + } + } + } +} +#endif + +// MARK: - tvOS picker + +#if os(tvOS) +/// The tvOS destination list, presented in a `fullScreenCover`: menus are +/// unusable there, and a sheet is a card too small to read. +struct SubscriptionDestinationPicker: View { + @Environment(\.dismiss) private var dismiss + + let channel: Channel + var onToggled: ((DataContext, Bool) -> Void)? + + var body: some View { + ZStack { + Color.black.ignoresSafeArea() + + VStack(alignment: .leading, spacing: 24) { + Text(String(localized: "channel.menu.subscribeOn")) + .font(.title2.bold()) + + ScrollView { + VStack(alignment: .leading, spacing: 12) { + // The picker stays open across a toggle: checkmarks are + // the whole point of the list, and closing it would hide + // the change the user just made. + SubscriptionDestinationRows(channel: channel, onToggled: onToggled) + } + } + + Button(String(localized: "common.close")) { + dismiss() + } + .buttonStyle(TVSettingsButtonStyle()) + } + .frame(maxWidth: 900) + .padding(.horizontal, 80) + .padding(.vertical, 60) + } + } +} +#endif diff --git a/YatteeTests/SubscriptionDestinationTests.swift b/YatteeTests/SubscriptionDestinationTests.swift new file mode 100644 index 00000000..b04a3356 --- /dev/null +++ b/YatteeTests/SubscriptionDestinationTests.swift @@ -0,0 +1,332 @@ +// +// SubscriptionDestinationTests.swift +// YatteeTests +// +// Where a subscribe lands when the user names its destination, and what the +// menu is allowed to claim about the answers it gets back. +// + +import Testing +import Foundation +@testable import Yattee + +// MARK: - Scripted provider + +/// A ``SubscriptionProvider`` that answers from memory and records every call, +/// so a test can assert which destination a write was actually routed to. +@MainActor +private final class ScriptedSubscriptionProvider: SubscriptionProvider { + let context: DataContext + + private(set) var subscribed: Set = [] + private(set) var subscribeCalls: [String] = [] + private(set) var unsubscribeCalls: [String] = [] + private(set) var membershipCalls: [String] = [] + + /// Thrown by the next write. + var nextError: Error? + + /// While true, every call suspends until `release()`. + var holdsCallsOpen = false + private var parked: [CheckedContinuation] = [] + + init(context: DataContext, subscribed: Set = []) { + self.context = context + self.subscribed = subscribed + } + + func release() { + holdsCallsOpen = false + let waiting = parked + parked = [] + waiting.forEach { $0.resume() } + } + + private func park() async { + if holdsCallsOpen { + await withCheckedContinuation { parked.append($0) } + } + } + + func fetchSubscriptions() async throws -> [Channel] { + subscribed.map { Channel(id: .global($0), name: $0) } + } + + func subscribe(to channel: Channel) async throws { + subscribeCalls.append(channel.id.channelID) + await park() + if let nextError { throw nextError } + subscribed.insert(channel.id.channelID) + } + + func unsubscribe(from channelID: String) async throws { + unsubscribeCalls.append(channelID) + await park() + if let nextError { throw nextError } + subscribed.remove(channelID) + } + + func isSubscribed(to channelID: String) async -> Bool { + membershipCalls.append(channelID) + await park() + return subscribed.contains(channelID) + } + + var cachedSubscriptionCount: Int? { subscribed.count } +} + +/// Hands out the scripted providers instead of building ones that would need a +/// server and a credential. +@MainActor +private final class ScriptedProviderFactory: SubscriptionProviderFactory { + var providers: [UUID: ScriptedSubscriptionProvider] = [:] + + func provider( + for account: Account, + instance _: Instance, + accountsManager _: AccountsManager + ) -> (any SubscriptionProvider)? { + providers[account.id] + } +} + +// MARK: - Routing + +@MainActor +@Suite("Subscription destinations", .serialized) +struct SubscriptionDestinationRoutingTests { + /// Everything one test needs: two signed-in accounts on their own + /// instances, each with a provider that records what it was asked to do. + @MainActor + private struct Fixture { + let service: SubscriptionService + let dataManager: DataManager + let accounts: AccountsManager + let instances: InstancesManager + let madeInstances: [Instance] + let factory: ScriptedProviderFactory + let first: Account + let second: Account + + var firstProvider: ScriptedSubscriptionProvider { factory.providers[first.id]! } + var secondProvider: ScriptedSubscriptionProvider { factory.providers[second.id]! } + + func tearDown() { + for instance in madeInstances { instances.remove(instance) } + } + } + + private func makeFixture() throws -> Fixture { + let instances = InstancesManager() + let made = (0 ..< 2).map { index in + Instance( + type: .invidious, + url: URL(string: "https://inv-\(index)-\(UUID().uuidString).example.com")! + ) + } + for instance in made { instances.add(instance) } + + let accounts = AccountsManager( + identityScope: CredentialIdentityScope(fixedScope: "identity-A"), + instancesManager: instances, + defaults: UserDefaults(suiteName: "yattee.tests.\(UUID().uuidString)")! + ) + let first = accounts.addAccount(instanceID: made[0].id) + let second = accounts.addAccount(instanceID: made[1].id) + + let factory = ScriptedProviderFactory() + factory.providers[first.id] = ScriptedSubscriptionProvider(context: .account(first.id)) + factory.providers[second.id] = ScriptedSubscriptionProvider(context: .account(second.id)) + + let dataManager = try DataManager(inMemory: true) + let service = SubscriptionService(dataManager: dataManager, providerFactory: factory) + service.setAccountsManager(accounts) + + return Fixture( + service: service, + dataManager: dataManager, + accounts: accounts, + instances: instances, + madeInstances: made, + factory: factory, + first: first, + second: second + ) + } + + private func channel(_ id: String = "UC-test") -> Channel { + Channel(id: .global(id), name: "Channel \(id)") + } + + @Test("A subscribe lands on the account it names, not the one selected") + func subscribeRoutesToNamedDestination() async throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + + fixture.accounts.setContext(.account(fixture.first.id)) + try await fixture.service.subscribe(to: channel(), in: .account(fixture.second.id)) + + #expect(fixture.secondProvider.subscribeCalls == ["UC-test"]) + #expect(fixture.firstProvider.subscribeCalls.isEmpty) + } + + @Test("An unsubscribe lands on the account it names") + func unsubscribeRoutesToNamedDestination() async throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + + try await fixture.service.subscribe(to: channel(), in: .account(fixture.second.id)) + try await fixture.service.unsubscribe(from: "UC-test", in: .account(fixture.second.id)) + + #expect(fixture.secondProvider.unsubscribeCalls == ["UC-test"]) + #expect(fixture.firstProvider.unsubscribeCalls.isEmpty) + #expect(await fixture.service.isSubscribed(to: "UC-test", in: .account(fixture.second.id)) == false) + } + + @Test("The local destination writes to the device's own store") + func localDestinationWritesLocally() async throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + + fixture.accounts.setContext(.account(fixture.first.id)) + try await fixture.service.subscribe(to: channel(), in: .local) + + #expect(fixture.dataManager.isSubscribed(to: "UC-test")) + #expect(fixture.firstProvider.subscribeCalls.isEmpty) + #expect(await fixture.service.isSubscribed(to: "UC-test", in: .local) == true) + } + + @Test("Membership is answered per destination") + func membershipIsPerDestination() async throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + + try await fixture.service.subscribe(to: channel(), in: .account(fixture.first.id)) + + #expect(await fixture.service.isSubscribed(to: "UC-test", in: .account(fixture.first.id)) == true) + #expect(await fixture.service.isSubscribed(to: "UC-test", in: .account(fixture.second.id)) == false) + #expect(await fixture.service.isSubscribed(to: "UC-test", in: .local) == false) + } + + @Test("Without a destination the current context is written") + func implicitCallsUseTheCurrentContext() async throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + + fixture.accounts.setContext(.account(fixture.second.id)) + try await fixture.service.subscribe(to: channel()) + + #expect(fixture.secondProvider.subscribeCalls == ["UC-test"]) + #expect(fixture.firstProvider.subscribeCalls.isEmpty) + } + + @Test("A named destination is written even while the selected account is unusable") + func namedDestinationIgnoresTheSelectedContext() async throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + + // An account with no instance and no provider: the selected context + // cannot be written to at all, which is exactly when a destination row + // is the only way to subscribe. + let unusable = fixture.accounts.addAccount(instanceID: UUID()) + fixture.accounts.setContext(.account(unusable.id)) + + await #expect(throws: Error.self) { + try await fixture.service.subscribe(to: channel()) + } + + try await fixture.service.subscribe(to: channel(), in: .account(fixture.first.id)) + #expect(fixture.firstProvider.subscribeCalls == ["UC-test"]) + } + + @Test("A destination that cannot be served says nothing rather than 'not subscribed'") + func unservableDestinationAnswersUnknown() async throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + + let unusable = fixture.accounts.addAccount(instanceID: UUID()) + + #expect(await fixture.service.isSubscribed(to: "UC-test", in: .account(unusable.id)) == nil) + } + + @Test("An answer that outlived its login is discarded") + func staleMembershipAnswerIsDiscarded() async throws { + let fixture = try makeFixture() + defer { fixture.tearDown() } + + fixture.firstProvider.holdsCallsOpen = true + + let pending = Task { await fixture.service.isSubscribed(to: "UC-test", in: .account(fixture.first.id)) } + + for _ in 0 ..< 200 where fixture.firstProvider.membershipCalls.isEmpty { + try await Task.sleep(for: .milliseconds(5)) + } + #expect(!fixture.firstProvider.membershipCalls.isEmpty) + + // Signing in as somebody else replaces every provider; the answer now + // in flight belongs to the identity that left. + fixture.service.discardAllProviders() + fixture.firstProvider.release() + + #expect(await pending.value == nil) + } +} + +// MARK: - Membership state + +@MainActor +@Suite("Subscription destination membership") +struct SubscriptionDestinationMembershipTests { + @Test("Nothing is claimed before an answer arrives") + func unknownUntilAnswered() { + let membership = SubscriptionDestinationMembership() + + #expect(membership.isSubscribed(on: .local) == nil) + } + + @Test("An answer is kept for the destination it was asked about") + func answersAreKeptPerDestination() { + let account = DataContext.account(UUID()) + var membership = SubscriptionDestinationMembership() + + membership.record(true, for: account, generation: membership.generation) + membership.record(false, for: .local, generation: membership.generation) + + #expect(membership.isSubscribed(on: account) == true) + #expect(membership.isSubscribed(on: .local) == false) + } + + @Test("An answer with nothing to say is not stored as 'not subscribed'") + func unknownAnswersAreNotStored() { + var membership = SubscriptionDestinationMembership() + + membership.record(nil, for: .local, generation: membership.generation) + + #expect(membership.isSubscribed(on: .local) == nil) + } + + @Test("A new round forgets what the last one learned") + func invalidationForgetsAnswers() { + var membership = SubscriptionDestinationMembership() + membership.record(true, for: .local, generation: membership.generation) + + membership.invalidate() + + #expect(membership.isSubscribed(on: .local) == nil) + } + + @Test("An answer from an abandoned round is dropped") + func lateAnswersAreDropped() { + var membership = SubscriptionDestinationMembership() + let stale = membership.generation + + // The channel changed while the question was in flight. + membership.invalidate() + membership.record(true, for: .local, generation: stale) + + #expect(membership.isSubscribed(on: .local) == nil) + + membership.record(false, for: .local, generation: membership.generation) + #expect(membership.isSubscribed(on: .local) == false) + } +}