From 7fc38ded85f4306beefe6004319aa7baa635ca67 Mon Sep 17 00:00:00 2001 From: GeneralD Date: Sun, 14 Jun 2026 22:25:08 +0900 Subject: [PATCH] refactor(#275): expose payload-less RevealPhase from HeaderPresenter `titleState` / `artistState: FetchState` carried the decode target string as public `@Published` payload, duplicating `displayTitle` / `displayArtist`. The View only gated visibility on `!= .idle` and the target was used solely for internal dedup. Replace them with payload-less `titlePhase` / `artistPhase: RevealPhase` (`.idle` / `.revealing` / `.revealed`) and move the dedup target to private `titleTarget` / `artistTarget`. Behaviour, decode animation, and artwork handling are unchanged. Tests now observe `displayTitle` / `displayArtist` + the lifecycle phase instead of the target payload. Documented why `LyricsPresenter.lyricsState` legitimately keeps its payload (consumed by columns / active-line tick). --- .claude/CLAUDE.md | 4 +- Sources/Entity/RevealPhase.swift | 17 +++++++ .../Presenters/Track/HeaderPresenter.swift | 31 ++++++++---- .../Presenters/Track/LyricsPresenter.swift | 7 +++ Sources/VersionHandler/Resources/version.txt | 2 +- Sources/Views/Header/HeaderView.swift | 2 +- .../HeaderPresenterDuplicateTests.swift | 49 ++++++++----------- .../HeaderPresenterTests.swift | 36 ++++++-------- Tests/ViewsTests/ViewRenderingTests.swift | 4 +- 9 files changed, 87 insertions(+), 65 deletions(-) create mode 100644 Sources/Entity/RevealPhase.swift diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 50848e8..4062baa 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -230,9 +230,9 @@ Presenters subscribe to Interactors via Combine. Interactors access UseCases via **Presenter / View separation**: Presenters (`ObservableObject`) own all display state via `@Published` properties. Views observe Presenters via `@ObservedObject` and are purely declarative — no business logic, no `@Dependency` references to Interactors or UseCases. Style information (fonts, colors, sizes) flows from Interactor → Presenter → View. -**FetchState\**: Generic enum (`.idle`, `.loading`, `.revealing(T)`, `.success(T)`, `.failure`) drives both data flow and UI animation. The `.revealing` → `.success` transition is timed by Presenters using `DecodeEffectState`. +**FetchState\**: Generic enum (`.idle`, `.loading`, `.revealing(T)`, `.success(T)`, `.failure`) drives both data flow and UI animation. The `.revealing` → `.success` transition is timed by Presenters using `DecodeEffectState`. Use `FetchState` only when the payload `T` is genuinely consumed downstream (e.g. `LyricsPresenter.lyricsState`, whose content feeds `columns(in:)` and `updateActiveLineTick()`). When a Presenter only needs the animation lifecycle and the View already renders the text from a separate `display…` property, expose the payload-less `RevealPhase` (`.idle` / `.revealing` / `.revealed`) instead and keep the decode target in a private field — `HeaderPresenter` does this for `titlePhase` / `artistPhase` so the public surface never duplicates `displayTitle` / `displayArtist` (#275). -**Entity types**: `AppStyle`, `TextLayout`, `TextAppearance`, `ArtworkStyle`, `RippleStyle`, `WallpaperStyle`, `WallpaperItem`, `WallpaperPlaybackMode`, `DecodeEffect`, `AIEndpoint`, `ColorStyle`, `HealthCheckResult`, `ConfigValidationResult`, `MusicBrainzMetadata`, `MediaRemotePollResult`, `LocalWallpaper`, `RemoteWallpaper`, `YouTubeWallpaper`, `TrackUpdate`, `TrackLyricsState`, `WallpaperState`, `ResolvedWallpaperItem`, `ScreenLayout`, `WallpaperConfig`, `WallpaperItemConfig`, `NowPlayingInfo`, `LyricLine`, `LyricsContent`. Config flows through Interactors, not via global `AppStyleKey`. +**Entity types**: `AppStyle`, `TextLayout`, `TextAppearance`, `ArtworkStyle`, `RippleStyle`, `WallpaperStyle`, `WallpaperItem`, `WallpaperPlaybackMode`, `DecodeEffect`, `AIEndpoint`, `ColorStyle`, `HealthCheckResult`, `ConfigValidationResult`, `MusicBrainzMetadata`, `MediaRemotePollResult`, `LocalWallpaper`, `RemoteWallpaper`, `YouTubeWallpaper`, `TrackUpdate`, `TrackLyricsState`, `WallpaperState`, `ResolvedWallpaperItem`, `ScreenLayout`, `WallpaperConfig`, `WallpaperItemConfig`, `NowPlayingInfo`, `LyricLine`, `LyricsContent`, `RevealPhase`. Config flows through Interactors, not via global `AppStyleKey`. **No AppStyleKey**: `@Dependency(\.appStyle)` was removed. All config access goes through the owning Interactor's computed properties (e.g., `trackInteractor.textLayout`, `wallpaperInteractor.rippleConfig`). This enforces the VIPER dependency rule. diff --git a/Sources/Entity/RevealPhase.swift b/Sources/Entity/RevealPhase.swift new file mode 100644 index 0000000..1a3ac2e --- /dev/null +++ b/Sources/Entity/RevealPhase.swift @@ -0,0 +1,17 @@ +/// Payload-less lifecycle of a decode-reveal text animation. +/// +/// Presenters expose this instead of `FetchState` so the public API +/// carries only the animation lifecycle, never the target string — the View +/// already reads the rendered text from `displayTitle` / `displayArtist`, and +/// the decode target belongs in a private field, not the public surface (#275). +public enum RevealPhase { + /// No content — the field is empty and should not be rendered. + case idle + /// Decode animation is in progress. + case revealing + /// Decode animation has settled on the final text. + case revealed +} + +extension RevealPhase: Sendable {} +extension RevealPhase: Equatable {} diff --git a/Sources/Presenters/Track/HeaderPresenter.swift b/Sources/Presenters/Track/HeaderPresenter.swift index 6ba3b99..8769d77 100644 --- a/Sources/Presenters/Track/HeaderPresenter.swift +++ b/Sources/Presenters/Track/HeaderPresenter.swift @@ -9,8 +9,13 @@ public final class HeaderPresenter: ObservableObject { @Published public private(set) var displayTitle: String = " " @Published public private(set) var displayArtist: String = " " @Published public private(set) var artworkImage: NSImage? - @Published public private(set) var titleState: FetchState = .idle - @Published public private(set) var artistState: FetchState = .idle + // Payload-less reveal lifecycle: the View gates header visibility on + // `titlePhase != .idle`, and tests observe the settle to `.revealed`. The + // target strings the decode aims at live in private `titleTarget` / + // `artistTarget` below — they are an internal dedup concern, not public + // state (#275). + @Published public private(set) var titlePhase: RevealPhase = .idle + @Published public private(set) var artistPhase: RevealPhase = .idle public private(set) var titleStyle: TextAppearance = .init() public private(set) var artistStyle: TextAppearance = .init() @@ -19,6 +24,8 @@ public final class HeaderPresenter: ObservableObject { private var titleEffect: DecodeEffectState? private var artistEffect: DecodeEffectState? + private var titleTarget: String? + private var artistTarget: String? private var artworkData: Data? private var cancellables: Set = [] @@ -72,35 +79,39 @@ extension HeaderPresenter { private func revealTitle(_ text: String?) { guard let text else { - titleState = .idle + titleTarget = nil + titlePhase = .idle displayTitle = " " return } guard let effect = titleEffect else { return } - guard titleState.value != text else { return } - titleState = .revealing(text) + guard titleTarget != text else { return } + titleTarget = text + titlePhase = .revealing effect.onUpdate = { [weak self] displayText in self?.displayTitle = displayText } effect.decode(to: text) { [weak self] in - self?.titleState = .success(text) + self?.titlePhase = .revealed } } private func revealArtist(_ text: String?) { guard let text else { - artistState = .idle + artistTarget = nil + artistPhase = .idle displayArtist = " " return } guard let effect = artistEffect else { return } - guard artistState.value != text else { return } - artistState = .revealing(text) + guard artistTarget != text else { return } + artistTarget = text + artistPhase = .revealing effect.onUpdate = { [weak self] displayText in self?.displayArtist = displayText } effect.decode(to: text) { [weak self] in - self?.artistState = .success(text) + self?.artistPhase = .revealed } } } diff --git a/Sources/Presenters/Track/LyricsPresenter.swift b/Sources/Presenters/Track/LyricsPresenter.swift index 76103d4..e921e67 100644 --- a/Sources/Presenters/Track/LyricsPresenter.swift +++ b/Sources/Presenters/Track/LyricsPresenter.swift @@ -5,6 +5,13 @@ import Foundation @MainActor public final class LyricsPresenter: ObservableObject { + // `lyricsState`'s payload is the source-of-truth lyrics content, not a + // display duplicate: `columns(in:)` reads `lyricsState.value` and + // `updateActiveLineTick()` matches `.success(.timed(...))` to lay out and + // highlight lines. Unlike HeaderPresenter's removed `titleState` — whose + // payload only mirrored `displayTitle` as a dedup key — this payload is + // genuinely consumed, so it stays public. A finer `lyricsContent` split is + // out of scope (#275). @Published public private(set) var lyricsState: FetchState = .idle @Published public private(set) var displayLyricLines: [String] = [] @Published public private(set) var activeLineIndex: Int? diff --git a/Sources/VersionHandler/Resources/version.txt b/Sources/VersionHandler/Resources/version.txt index de4e782..2236901 100644 --- a/Sources/VersionHandler/Resources/version.txt +++ b/Sources/VersionHandler/Resources/version.txt @@ -1 +1 @@ -2.13.18 +2.13.19 diff --git a/Sources/Views/Header/HeaderView.swift b/Sources/Views/Header/HeaderView.swift index 41d8780..1bf5d76 100644 --- a/Sources/Views/Header/HeaderView.swift +++ b/Sources/Views/Header/HeaderView.swift @@ -14,7 +14,7 @@ public struct HeaderView: View { public var body: some View { @Dependency(\.swiftUIResolver) var resolver - if presenter.titleState != .idle { + if presenter.titlePhase != .idle { HStack(spacing: presenter.artworkOpacity > 0 ? 24 : 0) { if presenter.artworkOpacity > 0 { if let image = presenter.artworkImage { diff --git a/Tests/PresentersTests/HeaderPresenterDuplicateTests.swift b/Tests/PresentersTests/HeaderPresenterDuplicateTests.swift index 85e1af4..c40b921 100644 --- a/Tests/PresentersTests/HeaderPresenterDuplicateTests.swift +++ b/Tests/PresentersTests/HeaderPresenterDuplicateTests.swift @@ -24,9 +24,9 @@ private struct StubTrackInteractor: TrackInteractor, @unchecked Sendable { // MARK: - Helpers @MainActor -private func waitForTitleSuccess(_ presenter: HeaderPresenter, timeout: Duration = .seconds(3)) async { +private func waitForReveal(_ presenter: HeaderPresenter, timeout: Duration = .seconds(3)) async { let deadline = ContinuousClock.now + timeout - while !presenter.titleState.isSuccess || !presenter.artistState.isSuccess, + while presenter.titlePhase != .revealed || presenter.artistPhase != .revealed, ContinuousClock.now < deadline { try? await Task.sleep(for: .milliseconds(10)) @@ -42,15 +42,6 @@ private func fixtureArtworkData(color: NSColor = .red) throws -> Data { return try #require(image.tiffRepresentation) } -extension FetchState { - fileprivate var isSuccess: Bool { - switch self { - case .success: true - default: false - } - } -} - // MARK: - Tests @Suite("HeaderPresenter duplicate / artwork interactions") @@ -75,17 +66,19 @@ struct HeaderPresenterDuplicateTests { // First send subject.send(update) - await waitForTitleSuccess(presenter) - #expect(presenter.titleState == .success("Same")) - #expect(presenter.artistState == .success("Artist")) + await waitForReveal(presenter) + #expect(presenter.displayTitle == "Same") + #expect(presenter.displayArtist == "Artist") // Second send with identical title/artist subject.send(update) try? await Task.sleep(for: .milliseconds(200)) - // Should remain .success, not reset to .revealing - #expect(presenter.titleState == .success("Same")) - #expect(presenter.artistState == .success("Artist")) + // Should remain revealed, not reset to .revealing + #expect(presenter.titlePhase == .revealed) + #expect(presenter.artistPhase == .revealed) + #expect(presenter.displayTitle == "Same") + #expect(presenter.displayArtist == "Artist") } } } @@ -93,7 +86,7 @@ struct HeaderPresenterDuplicateTests { @Suite("artwork stream") struct ArtworkStream { @MainActor - @Test("artwork updates without affecting titleState") + @Test("artwork updates without affecting title display") func artworkUpdatesIndependently() async throws { let trackSubject = PassthroughSubject() let artworkSubject = PassthroughSubject() @@ -111,8 +104,8 @@ struct HeaderPresenterDuplicateTests { // Set up title first trackSubject.send(update) - await waitForTitleSuccess(presenter) - #expect(presenter.titleState == .success("Song")) + await waitForReveal(presenter) + #expect(presenter.displayTitle == "Song") #expect(presenter.artworkImage == nil) // Send artwork @@ -124,9 +117,9 @@ struct HeaderPresenterDuplicateTests { } let cachedImage = try #require(presenter.artworkImage) - // Title state must remain unchanged - #expect(presenter.titleState == .success("Song")) - #expect(presenter.artistState == .success("Band")) + // Title display must remain unchanged + #expect(presenter.displayTitle == "Song") + #expect(presenter.displayArtist == "Band") artworkSubject.send(imageData) try? await Task.sleep(for: .milliseconds(200)) @@ -155,12 +148,12 @@ struct HeaderPresenterDuplicateTests { trackSubject.send(TrackUpdate(title: "New Song", artist: "New Artist")) let artData = try fixtureArtworkData() artworkSubject.send(artData) - await waitForTitleSuccess(presenter) + await waitForReveal(presenter) // Both should have settled correctly let cachedImage = try #require(presenter.artworkImage) - #expect(presenter.titleState == .success("New Song")) - #expect(presenter.artistState == .success("New Artist")) + #expect(presenter.displayTitle == "New Song") + #expect(presenter.displayArtist == "New Artist") // Now change artwork again let newArtData = try fixtureArtworkData(color: .blue) @@ -172,8 +165,8 @@ struct HeaderPresenterDuplicateTests { #expect(presenter.artworkImage != nil) #expect(presenter.artworkImage !== cachedImage) - // Title state still untouched - #expect(presenter.titleState == .success("New Song")) + // Title display still untouched + #expect(presenter.displayTitle == "New Song") } } } diff --git a/Tests/PresentersTests/HeaderPresenterTests.swift b/Tests/PresentersTests/HeaderPresenterTests.swift index 087529b..502b3be 100644 --- a/Tests/PresentersTests/HeaderPresenterTests.swift +++ b/Tests/PresentersTests/HeaderPresenterTests.swift @@ -21,19 +21,10 @@ private struct StubTrackInteractor: TrackInteractor, @unchecked Sendable { // MARK: - Helpers -extension FetchState { - fileprivate var isSuccess: Bool { - switch self { - case .success: true - default: false - } - } -} - @MainActor -private func waitForTitleSuccess(_ presenter: HeaderPresenter, timeout: Duration = .seconds(3)) async { +private func waitForReveal(_ presenter: HeaderPresenter, timeout: Duration = .seconds(3)) async { let deadline = ContinuousClock.now + timeout - while !presenter.titleState.isSuccess || !presenter.artistState.isSuccess, + while presenter.titlePhase != .revealed || presenter.artistPhase != .revealed, ContinuousClock.now < deadline { try? await Task.sleep(for: .milliseconds(10)) @@ -105,10 +96,12 @@ struct HeaderPresenterTests { presenter.start() subject.send(update) - await waitForTitleSuccess(presenter) + await waitForReveal(presenter) - #expect(presenter.titleState == .success("Hello")) - #expect(presenter.artistState == .success("World")) + #expect(presenter.displayTitle == "Hello") + #expect(presenter.displayArtist == "World") + #expect(presenter.titlePhase == .revealed) + #expect(presenter.artistPhase == .revealed) } } @@ -128,18 +121,18 @@ struct HeaderPresenterTests { // First send a valid track and wait for decode to complete subject.send(TrackUpdate(title: "Song", artist: "Artist")) - await waitForTitleSuccess(presenter) + await waitForReveal(presenter) // Then send an idle (nil) update subject.send(TrackUpdate()) // Wait for idle state let deadline = ContinuousClock.now + .seconds(3) - while !presenter.titleState.isIdle, ContinuousClock.now < deadline { + while presenter.titlePhase != .idle, ContinuousClock.now < deadline { try? await Task.sleep(for: .milliseconds(10)) } - #expect(presenter.titleState.isIdle) - #expect(presenter.artistState.isIdle) + #expect(presenter.titlePhase == .idle) + #expect(presenter.artistPhase == .idle) #expect(presenter.displayTitle == " ") #expect(presenter.displayArtist == " ") #expect(presenter.artworkImage == nil) @@ -164,15 +157,16 @@ struct HeaderPresenterTests { presenter.start() subject.send(TrackUpdate(title: "Song", artist: "Artist")) - await waitForTitleSuccess(presenter) - #expect(presenter.titleState == .success("Song")) + await waitForReveal(presenter) + #expect(presenter.displayTitle == "Song") presenter.stop() // After stop, new emissions should not change state subject.send(TrackUpdate(title: "New Song", artist: "New Artist")) try? await Task.sleep(for: .milliseconds(200)) - #expect(presenter.titleState == .success("Song"), "State should not change after stop") + #expect(presenter.displayTitle == "Song", "Display should not change after stop") + #expect(presenter.titlePhase == .revealed, "Phase should not change after stop") } } } diff --git a/Tests/ViewsTests/ViewRenderingTests.swift b/Tests/ViewsTests/ViewRenderingTests.swift index 1f86bc1..c03af58 100644 --- a/Tests/ViewsTests/ViewRenderingTests.swift +++ b/Tests/ViewsTests/ViewRenderingTests.swift @@ -104,9 +104,9 @@ struct HeaderViewRenderingTests { } operation: { HeaderPresenter() } - // Don't call start() — titleState stays .idle + // Don't call start() — titlePhase stays .idle render(HeaderView(presenter: presenter), size: CGSize(width: 600, height: 120)) - #expect(presenter.titleState == .idle) + #expect(presenter.titlePhase == .idle) } @Test("artwork hidden when opacity is 0")