diff --git a/Yattee/Core/AccountsManager.swift b/Yattee/Core/AccountsManager.swift index 13d81c97..fe92db8c 100644 --- a/Yattee/Core/AccountsManager.swift +++ b/Yattee/Core/AccountsManager.swift @@ -230,8 +230,16 @@ final class AccountsManager { pendingMarkerUploads.insert(instanceID) } + /// Stores the account's user-set label, or clears it with `nil`. + /// + /// A save that changes nothing is dropped rather than written. The rename + /// field is prefilled, so "Save" without an edit is the ordinary way out of + /// the alert — and every write here stamps `updatedAt` and queues an + /// upload, which under last-write-wins would let that idle tap overwrite a + /// peer's genuine rename that has not reached this device yet. func updateDisplayName(_ displayName: String?, for accountID: UUID) { guard let index = accounts.firstIndex(where: { $0.id == accountID }) else { return } + guard accounts[index].displayName != displayName else { return } accounts[index].displayName = displayName accounts[index].updatedAt = Date() pendingUploads.insert(accountID) diff --git a/Yattee/Services/Subscriptions/DataContextValidator.swift b/Yattee/Services/Subscriptions/DataContextValidator.swift index 9be93de0..2f07d215 100644 --- a/Yattee/Services/Subscriptions/DataContextValidator.swift +++ b/Yattee/Services/Subscriptions/DataContextValidator.swift @@ -239,14 +239,32 @@ final class DataContextValidator { /// What a rename field's contents mean once the user is done typing. /// /// `nil` clears the user-set label and hands the account back to - /// `defaultDisplayName`. An empty field says that outright — and so does - /// typing exactly the name the account already falls back to. The field - /// starts prefilled with the label the row is showing, so saving it - /// untouched must not turn a *device-local* fallback into a synced label - /// that every other device is then made to display. - static func renamedDisplayName(_ input: String, fallingBackTo defaultName: String) -> String? { + /// `defaultDisplayName`; anything else is stored as typed. + /// + /// The question the field cannot answer on its own is what an *untouched* + /// save means. The field starts prefilled with the label the row shows, and + /// on an account with no user-set label that label is device-local — the + /// login identifier only exists where the credential does. Saving it + /// unedited must not turn one device's fallback into a synced label every + /// other device is then made to display, so that case clears instead. + /// + /// The test is editedness, never equality with the default: on a second + /// device the string that device happens to fall back to is a perfectly + /// ordinary name to *choose*, and typing it has to store and sync like any + /// other. An unedited save on an account that already carries a label + /// re-states that label, which the manager then drops as a no-op — the name + /// stays, rather than being silently cleared by a tap that changed nothing. + static func renamedDisplayName( + _ input: String, + prefilledWith prefill: String, + hasUserSetLabel: Bool + ) -> String? { let trimmed = input.trimmingCharacters(in: .whitespacesAndNewlines) - guard !trimmed.isEmpty, trimmed != defaultName else { return nil } + guard !trimmed.isEmpty else { return nil } + + let wasEdited = trimmed != prefill.trimmingCharacters(in: .whitespacesAndNewlines) + guard wasEdited || hasUserSetLabel else { return nil } + return trimmed } } diff --git a/Yattee/Views/Settings/AccountRenameAlert.swift b/Yattee/Views/Settings/AccountRenameAlert.swift index 6aaf6039..6cebdb41 100644 --- a/Yattee/Views/Settings/AccountRenameAlert.swift +++ b/Yattee/Views/Settings/AccountRenameAlert.swift @@ -22,6 +22,51 @@ struct AccountRenameRequest: Identifiable { /// What the field is prefilled with: the label the row is showing now. let initialText: String + + /// Whether that prefill is a name the user chose, as opposed to the label + /// the account falls back to on this device. It is what tells an untouched + /// save to keep the name from one that must not publish a device-local + /// fallback — see `DataContextValidator.renamedDisplayName`. + let hasUserSetLabel: Bool +} + +extension AccountRenameRequest { + /// The request for an account that still resolves through a source. + /// + /// The single place the alert's strings are derived, so both surfaces that + /// offer Rename ask the validator the same two questions: what the row is + /// showing, and what clearing the field goes back to. + @MainActor + static func forAccount(_ account: Account, resolvedBy validator: DataContextValidator) -> Self { + Self( + id: account.id, + defaultName: validator.defaultDisplayName(for: account), + initialText: validator.displayName(for: account), + hasUserSetLabel: account.hasUserSetDisplayName + ) + } + + /// The request for an account with no source to resolve through. + /// + /// The fallback is the caller's to name: the validator's last resort for an + /// account whose instance is gone is the words "Source missing", which is a + /// diagnosis and not a name to offer the field. + static func forAccount(_ account: Account, fallingBackTo defaultName: String) -> Self { + Self( + id: account.id, + defaultName: defaultName, + initialText: account.displayName ?? "", + hasUserSetLabel: account.hasUserSetDisplayName + ) + } +} + +extension Account { + /// Whether the account carries a label the user chose. An empty string is + /// no more a name than `nil` is, and both arrive here from old records. + var hasUserSetDisplayName: Bool { + !(displayName ?? "").isEmpty + } } /// The rename alert, shared by every screen that lists accounts. @@ -40,8 +85,14 @@ private struct AccountRenameAlertModifier: ViewModifier { // Keyed on the ID rather than on the request: the prefill has to be // in place before the alert's field is built, and re-entering the // same row is the only time the text should be reset. + // + // Seeded only when a request appears, never blanked when one goes + // away. Dismissal sets the request to nil while the alert's buttons + // are still live, and Save reads this text when it runs — blanking + // here would let the two race, with an emptied field committing as + // "clear the name". .onChange(of: request?.id) { _, _ in - text = request?.initialText ?? "" + if let request { text = request.initialText } } .alert( String(localized: "accounts.rename.title"), @@ -56,7 +107,11 @@ private struct AccountRenameAlertModifier: ViewModifier { Button(String(localized: "common.save")) { onCommit( pending.id, - DataContextValidator.renamedDisplayName(text, fallingBackTo: pending.defaultName) + DataContextValidator.renamedDisplayName( + text, + prefilledWith: pending.initialText, + hasUserSetLabel: pending.hasUserSetLabel + ) ) } diff --git a/Yattee/Views/Settings/AccountsListView.swift b/Yattee/Views/Settings/AccountsListView.swift index 66f20af9..f9206e0b 100644 --- a/Yattee/Views/Settings/AccountsListView.swift +++ b/Yattee/Views/Settings/AccountsListView.swift @@ -349,18 +349,10 @@ struct AccountsListView: View { /// prefill a field with. private func makeRenameRequest(for account: Account, instance: Instance?) -> AccountRenameRequest { guard instance != nil, let validator = appEnvironment?.dataContextValidator else { - return AccountRenameRequest( - id: account.id, - defaultName: String(localized: "accounts.orphan.unnamed"), - initialText: account.displayName ?? "" - ) + return .forAccount(account, fallingBackTo: String(localized: "accounts.orphan.unnamed")) } - return AccountRenameRequest( - id: account.id, - defaultName: validator.defaultDisplayName(for: account), - initialText: validator.displayName(for: account) - ) + return .forAccount(account, resolvedBy: validator) } private func status(for account: Account) -> AccountStatus { diff --git a/Yattee/Views/Settings/InstanceAccountsSection.swift b/Yattee/Views/Settings/InstanceAccountsSection.swift index c7c0ff9b..b480e39f 100644 --- a/Yattee/Views/Settings/InstanceAccountsSection.swift +++ b/Yattee/Views/Settings/InstanceAccountsSection.swift @@ -195,13 +195,16 @@ struct InstanceAccountsSection: View { } /// What the rename field starts on, and what clearing it goes back to. + /// + /// The section always has its instance, so the only reason to name the + /// fallback here is a missing environment, where the row's own label is + /// already the instance's. private func makeRenameRequest(for account: Account) -> AccountRenameRequest { - AccountRenameRequest( - id: account.id, - defaultName: appEnvironment?.dataContextValidator.defaultDisplayName(for: account) - ?? instance.displayName, - initialText: displayName(for: account) - ) + guard let validator = appEnvironment?.dataContextValidator else { + return .forAccount(account, fallingBackTo: instance.displayName) + } + + return .forAccount(account, resolvedBy: validator) } private func status(for account: Account) -> AccountStatus { diff --git a/YatteeTests/AccountsScreenTests.swift b/YatteeTests/AccountsScreenTests.swift index f4a19401..c7423534 100644 --- a/YatteeTests/AccountsScreenTests.swift +++ b/YatteeTests/AccountsScreenTests.swift @@ -490,29 +490,166 @@ struct AccountRenameTests { @Test("A typed name replaces the fallback") func typedNameIsStored() { #expect( - DataContextValidator.renamedDisplayName(" Main i01 ", fallingBackTo: "i01") == "Main i01" + DataContextValidator.renamedDisplayName( + " Main i01 ", + prefilledWith: "i01", + hasUserSetLabel: false + ) == "Main i01" ) } @Test("An empty field clears the label") func emptyClearsTheLabel() { - #expect(DataContextValidator.renamedDisplayName("", fallingBackTo: "i01") == nil) - #expect(DataContextValidator.renamedDisplayName(" \n ", fallingBackTo: "i01") == nil) + for hasLabel in [true, false] { + #expect( + DataContextValidator.renamedDisplayName( + "", + prefilledWith: "i01", + hasUserSetLabel: hasLabel + ) == nil + ) + #expect( + DataContextValidator.renamedDisplayName( + " \n ", + prefilledWith: "Main i01", + hasUserSetLabel: hasLabel + ) == nil + ) + } } - @Test("Saving the prefilled fallback untouched sets no name at all") + @Test("Saving an unnamed account's prefill untouched sets no name at all") func unchangedFallbackIsNotStored() { - // The field starts on the label the row is showing, and that label can - // be device-local — the login identifier only exists where the - // credential does. Storing it would publish one device's fallback to - // every other device as if the user had chosen it. - #expect(DataContextValidator.renamedDisplayName("i01", fallingBackTo: "i01") == nil) + // The field starts on the label the row is showing, and on an account + // with no user-set label that is device-local — the login identifier + // only exists where the credential does. Storing it would publish one + // device's fallback to every other device as if the user had chosen it. #expect( DataContextValidator.renamedDisplayName( - "utest1@i01.v.yattee.stream", - fallingBackTo: "utest1@i01.v.yattee.stream" + "i01", + prefilledWith: "i01", + hasUserSetLabel: false ) == nil ) + #expect( + DataContextValidator.renamedDisplayName( + " utest1@i01.v.yattee.stream ", + prefilledWith: "utest1@i01.v.yattee.stream", + hasUserSetLabel: false + ) == nil + ) + } + + @Test("Typing a name that happens to match this device's fallback still stores it") + func editedNameEqualToTheDefaultIsStored() { + // The rule is editedness, not equality with the default. On a second + // device the string that device falls back to is an ordinary name to + // choose — and the prefill there is the *synced* label, so typing over + // it is a real edit that has to store and sync like any other. + #expect( + DataContextValidator.renamedDisplayName( + "i01", + prefilledWith: "Main i01", + hasUserSetLabel: true + ) == "i01" + ) + // Same on an account with no label yet: the prefill was the fallback + // "utest1@…", and the user replaced it with something else entirely. + #expect( + DataContextValidator.renamedDisplayName( + "i01", + prefilledWith: "utest1@i01.v.yattee.stream", + hasUserSetLabel: false + ) == "i01" + ) + } + + @Test("Saving a named account untouched keeps its name rather than clearing it") + func unchangedNameIsKept() { + // Decided semantics for "Save with no edit" on an account that already + // carries a label: re-state the label. The manager drops the write as a + // no-op, so the name survives and nothing is queued — an idle tap must + // never read as "clear the name". + #expect( + DataContextValidator.renamedDisplayName( + "Main i01", + prefilledWith: "Main i01", + hasUserSetLabel: true + ) == "Main i01" + ) + } + + @Test("Both rename surfaces offer the shown label and fall back to the default") + func renameRequestIsBuiltFromTheValidator() { + let (accounts, instances, validator) = makeFixture() + var instance = Instance(type: .invidious, url: URL(string: "https://inv.example.com")!) + instance.name = "My Invidious" + instances.add(instance) + + let account = accounts.addAccount(instanceID: instance.id) + + let unnamed = AccountRenameRequest.forAccount(account, resolvedBy: validator) + #expect(unnamed.id == account.id) + #expect(unnamed.defaultName == validator.defaultDisplayName(for: account)) + #expect(unnamed.initialText == validator.displayName(for: account)) + #expect(unnamed.hasUserSetLabel == false) + + accounts.updateDisplayName("Main i01", for: account.id) + let renamed = accounts.account(id: account.id)! + + let named = AccountRenameRequest.forAccount(renamed, resolvedBy: validator) + // The placeholder stays the *default*, never the label being edited: + // clearing the field has to advertise where it lands. + #expect(named.defaultName == "My Invidious") + #expect(named.initialText == "Main i01") + #expect(named.hasUserSetLabel) + } + + @Test("An orphan's rename request offers only its own label") + func orphanRenameRequestSkipsTheValidator() { + let (accounts, instances, _) = makeFixture() + let instance = Instance(type: .invidious, url: URL(string: "https://inv.example.com")!) + instances.add(instance) + + let account = accounts.addAccount(instanceID: instance.id) + let unnamed = AccountRenameRequest.forAccount(account, fallingBackTo: "Unnamed") + #expect(unnamed.defaultName == "Unnamed") + #expect(unnamed.initialText.isEmpty) + #expect(unnamed.hasUserSetLabel == false) + + accounts.updateDisplayName("Abandoned login", for: account.id) + let named = AccountRenameRequest.forAccount( + accounts.account(id: account.id)!, + fallingBackTo: "Unnamed" + ) + #expect(named.initialText == "Abandoned login") + #expect(named.hasUserSetLabel) + } + + @Test("Re-saving the same name writes nothing") + func noOpSaveIsNotWritten() { + let (accounts, instances, _) = makeFixture() + let instance = Instance(type: .invidious, url: URL(string: "https://inv.example.com")!) + instances.add(instance) + + let account = accounts.addAccount(instanceID: instance.id) + + // An untouched save on an account with no label: nil over nil. + accounts.confirmUpload(account.id) + let untouchedAt = accounts.account(id: account.id)?.updatedAt + accounts.updateDisplayName(nil, for: account.id) + #expect(!accounts.pendingUploads.contains(account.id)) + #expect(accounts.account(id: account.id)?.updatedAt == untouchedAt) + + accounts.updateDisplayName("Main i01", for: account.id) + accounts.confirmUpload(account.id) + let renamedAt = accounts.account(id: account.id)?.updatedAt + + // And on one that carries a label: the same string over itself. + accounts.updateDisplayName("Main i01", for: account.id) + #expect(!accounts.pendingUploads.contains(account.id)) + #expect(accounts.account(id: account.id)?.updatedAt == renamedAt) + #expect(accounts.account(id: account.id)?.displayName == "Main i01") } @Test("The default name ignores the user-set one, and the shown name prefers it")