-
Notifications
You must be signed in to change notification settings - Fork 0
perf(#257): elapsed を Presenter 側で補間し helper Timer を 3 秒に延長 #264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
8ce1f44
perf(#257): elapsed を Presenter 側で補間し helper Timer を 3 秒に延長
GeneralD 090761d
chore: bump version to 2.13.7
GeneralD 2b7f7d9
fix(#257): timestamp 欠落時の補間と新規テストの flaky sleep を解消
GeneralD fa8a663
test(#257): TrackInteractor playbackPosition publisher のテストを追加
GeneralD 1250bdf
test(#257): TrackInteractor 残カバレッジを 92.94% → 97.49% に引き上げ
GeneralD 78acf8b
test(#257): 固定 Task.sleep を deadline polling に置換、count 検証を try #requi…
GeneralD File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,20 @@ | ||
| import Foundation | ||
|
|
||
| public struct PlaybackPosition { | ||
| public let elapsed: TimeInterval? | ||
| public let rawElapsed: TimeInterval? | ||
| public let timestamp: Date? | ||
| public let playbackRate: Double | ||
|
|
||
| public init(elapsed: TimeInterval? = nil, playbackRate: Double = 1.0) { | ||
| self.elapsed = elapsed | ||
| public init( | ||
| rawElapsed: TimeInterval? = nil, | ||
| timestamp: Date? = nil, | ||
| playbackRate: Double = 1.0 | ||
| ) { | ||
| self.rawElapsed = rawElapsed | ||
| self.timestamp = timestamp | ||
| self.playbackRate = playbackRate | ||
| } | ||
| } | ||
|
|
||
| extension PlaybackPosition: Sendable {} | ||
| extension PlaybackPosition: Equatable {} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 2.13.6 | ||
| 2.13.7 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -192,13 +192,20 @@ struct LyricsPresenterTests { | |
| trackSubject.send(TrackUpdate(lyrics: content, lyricsState: .resolved)) | ||
| await waitForLyricsSuccess(presenter) | ||
|
|
||
| // Send a paused playback position (rate = 0) | ||
| positionSubject.send(PlaybackPosition(elapsed: 6, playbackRate: 0)) | ||
| // Allow Combine to deliver the position update | ||
| try? await Task.sleep(for: .milliseconds(50)) | ||
|
|
||
| presenter.updateActiveLineTick() | ||
| // activeLineIndex should remain nil because playback is paused | ||
| // Send a paused playback position (rate = 0). | ||
| positionSubject.send(PlaybackPosition(rawElapsed: 6, playbackRate: 0)) | ||
|
|
||
| // activeLineIndex must remain nil — both before sink delivery | ||
| // (latestRawElapsed still nil → interpolatedElapsed nil → no index | ||
| // change) and after delivery (latestPlaybackRate == 0 → early | ||
| // return). Poll the tick to confirm it never flips during a | ||
| // short observation window. | ||
| let deadline = ContinuousClock.now + .seconds(1) | ||
| while ContinuousClock.now < deadline { | ||
| presenter.updateActiveLineTick() | ||
| if presenter.activeLineIndex != nil { break } | ||
| try? await Task.sleep(for: .milliseconds(10)) | ||
| } | ||
| #expect(presenter.activeLineIndex == nil) | ||
| } | ||
| } | ||
|
|
@@ -229,11 +236,97 @@ struct LyricsPresenterTests { | |
| await waitForLyricsSuccess(presenter) | ||
|
|
||
| // Send position at 6s — should highlight Line B (time=5) | ||
| positionSubject.send(PlaybackPosition(elapsed: 6, playbackRate: 1.0)) | ||
| // Allow Combine to deliver the position update | ||
| try? await Task.sleep(for: .milliseconds(50)) | ||
| positionSubject.send(PlaybackPosition(rawElapsed: 6, playbackRate: 1.0)) | ||
|
|
||
|
Comment on lines
238
to
240
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. deadline polling に置換しました (78acf8b)。3 秒以内に |
||
| let deadline = ContinuousClock.now + .seconds(3) | ||
| while ContinuousClock.now < deadline { | ||
| presenter.updateActiveLineTick() | ||
| if presenter.activeLineIndex == 1 { break } | ||
| try? await Task.sleep(for: .milliseconds(10)) | ||
| } | ||
| #expect(presenter.activeLineIndex == 1) | ||
| } | ||
| } | ||
|
|
||
| @MainActor | ||
| @Test("interpolates elapsed from snapshot (rawElapsed + timestamp + playbackRate)") | ||
| func interpolatesFromSnapshot() async throws { | ||
| let fixedNow = Date(timeIntervalSinceReferenceDate: 1_000_000) | ||
| let trackSubject = PassthroughSubject<TrackUpdate, Never>() | ||
| let positionSubject = PassthroughSubject<PlaybackPosition, Never>() | ||
| let lines: [LyricLine] = [ | ||
| .init(time: 0, text: "Line A"), | ||
| .init(time: 5, text: "Line B"), | ||
| .init(time: 10, text: "Line C"), | ||
| ] | ||
| let content = LyricsContent.timed(lines) | ||
|
|
||
| await withDependencies { | ||
| $0.trackInteractor = StubTrackInteractor( | ||
| trackChangePublisher: trackSubject.eraseToAnyPublisher(), | ||
| playbackPositionPublisher: positionSubject.eraseToAnyPublisher(), | ||
| textLayout: TextLayout(decodeEffect: .init(duration: 0)) | ||
| ) | ||
| $0.date = .constant(fixedNow) | ||
| } operation: { | ||
| let presenter = LyricsPresenter() | ||
| presenter.start() | ||
|
|
||
| trackSubject.send(TrackUpdate(lyrics: content, lyricsState: .resolved)) | ||
| await waitForLyricsSuccess(presenter) | ||
|
|
||
| presenter.updateActiveLineTick() | ||
| // Snapshot from 7s ago: rawElapsed=0, rate=1.0 | ||
| // Interpolated at fixedNow: 0 + 1.0 * 7.0 = 7.0 → Line B (time=5) | ||
| positionSubject.send( | ||
| PlaybackPosition( | ||
| rawElapsed: 0, | ||
| timestamp: fixedNow.addingTimeInterval(-7), | ||
| playbackRate: 1.0)) | ||
|
|
||
| let deadline = ContinuousClock.now + .seconds(3) | ||
| while ContinuousClock.now < deadline { | ||
| presenter.updateActiveLineTick() | ||
| if presenter.activeLineIndex == 1 { break } | ||
| try? await Task.sleep(for: .milliseconds(10)) | ||
| } | ||
| #expect(presenter.activeLineIndex == 1) | ||
| } | ||
| } | ||
|
|
||
| @MainActor | ||
| @Test("falls back to rawElapsed when timestamp is nil") | ||
| func fallsBackWhenTimestampMissing() async throws { | ||
| let trackSubject = PassthroughSubject<TrackUpdate, Never>() | ||
| let positionSubject = PassthroughSubject<PlaybackPosition, Never>() | ||
| let lines: [LyricLine] = [ | ||
| .init(time: 0, text: "A"), | ||
| .init(time: 5, text: "B"), | ||
| .init(time: 10, text: "C"), | ||
| ] | ||
| let content = LyricsContent.timed(lines) | ||
|
|
||
| await withDependencies { | ||
| $0.trackInteractor = StubTrackInteractor( | ||
| trackChangePublisher: trackSubject.eraseToAnyPublisher(), | ||
| playbackPositionPublisher: positionSubject.eraseToAnyPublisher(), | ||
| textLayout: TextLayout(decodeEffect: .init(duration: 0)) | ||
| ) | ||
| } operation: { | ||
| let presenter = LyricsPresenter() | ||
| presenter.start() | ||
|
|
||
| trackSubject.send(TrackUpdate(lyrics: content, lyricsState: .resolved)) | ||
| await waitForLyricsSuccess(presenter) | ||
|
|
||
| // timestamp nil → rawElapsed used verbatim (no interpolation) | ||
| positionSubject.send(PlaybackPosition(rawElapsed: 7, timestamp: nil, playbackRate: 1.0)) | ||
|
|
||
| let deadline = ContinuousClock.now + .seconds(3) | ||
| while ContinuousClock.now < deadline { | ||
| presenter.updateActiveLineTick() | ||
| if presenter.activeLineIndex == 1 { break } | ||
| try? await Task.sleep(for: .milliseconds(10)) | ||
| } | ||
| #expect(presenter.activeLineIndex == 1) | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With MediaRemote sources that omit
kMRMediaRemoteNowPlayingInfoTimestamp, the presenter explicitly falls back torawElapsedwithout interpolation (LyricsPresenter.interpolatedElapsedreturnsbasewhen the timestamp is nil), so increasing the helper fallback to 3 seconds makes lyric highlighting update only once every 3 seconds for those sources. The previous 1-second fallback was the only mechanism advancing elapsed time in this scenario; either keep a shorter interval for timestamp-less snapshots or synthesize/update a timestamp before relying on the 3-second poll.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
指摘の懸念に対し、helper 側で fetch 時刻を timestamp として
する形に修正しました (2b7f7d9)。
kMRMediaRemoteNowPlayingInfoTimestampがDate()を代替値として使用し、client 側の補間が常に有効になるようにしています。これにより timestamp-less ソースでも 3 秒 polling 間隔のままフレーム単位精度をplaybackRate == 0(pause) 時は LyricsPresenter 側で補間自体をupdateActiveLineTickのガード) ため、停止中に誤って前進する副作用はありません。