From 1d85b19bb669047e5aeb0f329f8f847782e5d458 Mon Sep 17 00:00:00 2001 From: Gianni Carlo Date: Sun, 7 Jun 2026 08:30:49 -0500 Subject: [PATCH 1/3] Add chapter threshold logic for prev chapter button --- BookPlayer/Player/PlayerManager.swift | 18 ++++++++++++++---- .../Lightweight-Models/PlayableItem.swift | 16 +++++++++++++--- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/BookPlayer/Player/PlayerManager.swift b/BookPlayer/Player/PlayerManager.swift index 4c4c0d59b..d0fc7722d 100755 --- a/BookPlayer/Player/PlayerManager.swift +++ b/BookPlayer/Player/PlayerManager.swift @@ -791,14 +791,24 @@ extension PlayerManager { } func skipToPreviousChapter() { - if let currentChapter = currentItem?.currentChapter, - let previousChapter = currentItem?.previousChapter(before: currentChapter) - { + defer { NotificationCenter.default.post(name: .listeningProgressChanged, object: nil) } + + guard let currentItem, + let currentChapter = currentItem.currentChapter + else { + playPreviousItem() + return + } + + if !currentItem.isNearChapterStart { + /// First press while into the chapter goes back to its start + jumpToChapter(currentChapter) + } else if let previousChapter = currentItem.previousChapter(before: currentChapter) { + /// Already near the chapter start, so step back to the previous chapter jumpToChapter(previousChapter) } else { playPreviousItem() } - NotificationCenter.default.post(name: .listeningProgressChanged, object: nil) } func skip(_ interval: TimeInterval) { diff --git a/Shared/CoreData/Lightweight-Models/PlayableItem.swift b/Shared/CoreData/Lightweight-Models/PlayableItem.swift index 4b0397177..5abe20a2b 100644 --- a/Shared/CoreData/Lightweight-Models/PlayableItem.swift +++ b/Shared/CoreData/Lightweight-Models/PlayableItem.swift @@ -128,6 +128,18 @@ public final class PlayableItem: NSObject, Identifiable { : self.duration } + /// Seconds from a chapter's start within which the playhead is still considered to be "at the start". + /// Shared by rewind clamping and the previous-chapter button so they stay in lockstep. + public static let chapterStartThreshold: TimeInterval = 3 + + /// Whether the playhead is close enough to the current chapter's start that a backward action + /// should step into the previous chapter rather than restart the current one. + public var isNearChapterStart: Bool { + guard let chapter = self.currentChapter else { return false } + + return self.currentTime < chapter.start + Self.chapterStartThreshold + } + public func getInterval(from proposedInterval: TimeInterval) -> TimeInterval { let interval = proposedInterval > 0 @@ -144,9 +156,7 @@ public final class PlayableItem: NSObject, Identifiable { return proposedInterval } - let chapterThreshold: TimeInterval = 3 - - if chapter.start + chapterThreshold > currentTime { + if self.isNearChapterStart { return proposedInterval } From cbb4c4cc496bc90d242674b4ae053c728796f110 Mon Sep 17 00:00:00 2001 From: Gianni Carlo Date: Sun, 7 Jun 2026 09:04:19 -0500 Subject: [PATCH 2/3] Address feedback --- BookPlayerTests/PlayableItemTests.swift | 31 +++++++ BookPlayerTests/PlayerManagerTests.swift | 90 +++++++++++++++++++ .../LocalPlayback/Player/PlayerManager.swift | 18 +++- .../Lightweight-Models/PlayableItem.swift | 2 +- 4 files changed, 136 insertions(+), 5 deletions(-) diff --git a/BookPlayerTests/PlayableItemTests.swift b/BookPlayerTests/PlayableItemTests.swift index ed7090c0a..419b113f1 100644 --- a/BookPlayerTests/PlayableItemTests.swift +++ b/BookPlayerTests/PlayableItemTests.swift @@ -84,4 +84,35 @@ class PlayableItemTests: XCTestCase { XCTAssert(remainingTimeInBook == -50) } + + // MARK: - isNearChapterStart + + func testIsNearChapterStartWithinThreshold() { + sut.currentChapter = sut.chapters[0] // starts at 0 + sut.currentTime = 2 + + XCTAssertTrue(sut.isNearChapterStart) + } + + func testIsNearChapterStartAtChapterStart() { + sut.currentChapter = sut.chapters[1] // starts at 51 + sut.currentTime = 51 + + XCTAssertTrue(sut.isNearChapterStart) + } + + func testIsNotNearChapterStartAtThresholdBoundary() { + /// Exactly `chapterStartThreshold` (3s) past the start is NOT near the start + sut.currentChapter = sut.chapters[1] // starts at 51 + sut.currentTime = 54 + + XCTAssertFalse(sut.isNearChapterStart) + } + + func testIsNotNearChapterStartMidChapter() { + sut.currentChapter = sut.chapters[1] // starts at 51 + sut.currentTime = 100 + + XCTAssertFalse(sut.isNearChapterStart) + } } diff --git a/BookPlayerTests/PlayerManagerTests.swift b/BookPlayerTests/PlayerManagerTests.swift index d2bd5ff21..70dc9a7e5 100644 --- a/BookPlayerTests/PlayerManagerTests.swift +++ b/BookPlayerTests/PlayerManagerTests.swift @@ -69,6 +69,43 @@ class PlayerManagerTests: XCTestCase { ) } + /// Two-chapter item using production's 1-based chapter indexing, so + /// `nextChapter`/`previousChapter` navigation resolves correctly. + private func generateChapteredItem() -> PlayableItem { + let chapter1 = PlayableChapter( + title: "chapter 1", + author: "author", + start: 0, + duration: 50, + relativePath: "", + remoteURL: nil, + index: 1 + ) + let chapter2 = PlayableChapter( + title: "chapter 2", + author: "author", + start: 51, + duration: 100, + relativePath: "", + remoteURL: nil, + index: 2 + ) + return PlayableItem( + title: "test book", + author: "test author", + chapters: [chapter1, chapter2], + currentTime: 0, + duration: 151, + relativePath: "", + uuid: "LEGACY_UUID", + parentFolder: nil, + percentCompleted: 10, + lastPlayDate: nil, + isFinished: false, + isBoundBook: false + ) + } + func testUpdatingEmptyNowPlayingBookTime() { self.sut.setNowPlayingBookTime() @@ -198,4 +235,57 @@ class PlayerManagerTests: XCTestCase { XCTAssertNil(nextItem) XCTAssertTrue(playbackServiceMock.getPlayableItemAfterParentFolderAutoplayedRestartFinishedCallsCount == 2) } + + // MARK: - skipToPreviousChapter + + func testSkipToPreviousChapterMidChapterRestartsCurrentChapter() { + let item = generateChapteredItem() + item.currentChapter = item.chapters[1] // chapter 2, starts at 51 + item.currentTime = 100 // well past the start threshold + sut.currentItem = item + + sut.skipToPreviousChapter() + + // Restarts the current chapter rather than stepping back + XCTAssertEqual(sut.currentItem?.currentChapter?.index, 2) + XCTAssertEqual(playbackServiceMock.getPlayableItemBeforeParentFolderCallsCount, 0) + } + + func testSkipToPreviousChapterNearStartStepsToPreviousChapter() { + let item = generateChapteredItem() + item.currentChapter = item.chapters[1] // chapter 2, starts at 51 + item.currentTime = 52 // within the start threshold + sut.currentItem = item + + sut.skipToPreviousChapter() + + // Steps back to the previous chapter + XCTAssertEqual(sut.currentItem?.currentChapter?.index, 1) + XCTAssertEqual(playbackServiceMock.getPlayableItemBeforeParentFolderCallsCount, 0) + } + + func testSkipToPreviousChapterFirstChapterMidChapterRestartsInstead() { + let item = generateChapteredItem() + item.currentChapter = item.chapters[0] // chapter 1, starts at 0 + item.currentTime = 30 // mid-chapter, past the threshold + sut.currentItem = item + + sut.skipToPreviousChapter() + + // Restarts chapter 1 instead of jumping to the previous item + XCTAssertEqual(sut.currentItem?.currentChapter?.index, 1) + XCTAssertEqual(playbackServiceMock.getPlayableItemBeforeParentFolderCallsCount, 0) + } + + func testSkipToPreviousChapterFirstChapterNearStartPlaysPreviousItem() { + let item = generateChapteredItem() + item.currentChapter = item.chapters[0] // chapter 1, starts at 0 + item.currentTime = 1 // within the start threshold, no previous chapter + sut.currentItem = item + + sut.skipToPreviousChapter() + + // Falls back to the previous item + XCTAssertEqual(playbackServiceMock.getPlayableItemBeforeParentFolderCallsCount, 1) + } } diff --git a/BookPlayerWatch/LocalPlayback/Player/PlayerManager.swift b/BookPlayerWatch/LocalPlayback/Player/PlayerManager.swift index b26ba4944..3d522d618 100644 --- a/BookPlayerWatch/LocalPlayback/Player/PlayerManager.swift +++ b/BookPlayerWatch/LocalPlayback/Player/PlayerManager.swift @@ -745,14 +745,24 @@ extension PlayerManager { } func skipToPreviousChapter() { - if let currentChapter = currentItem?.currentChapter, - let previousChapter = currentItem?.previousChapter(before: currentChapter) - { + defer { NotificationCenter.default.post(name: .listeningProgressChanged, object: nil) } + + guard let currentItem, + let currentChapter = currentItem.currentChapter + else { + playPreviousItem() + return + } + + if !currentItem.isNearChapterStart { + /// First press while into the chapter goes back to its start + jumpToChapter(currentChapter) + } else if let previousChapter = currentItem.previousChapter(before: currentChapter) { + /// Already near the chapter start, so step back to the previous chapter jumpToChapter(previousChapter) } else { playPreviousItem() } - NotificationCenter.default.post(name: .listeningProgressChanged, object: nil) } func skip(_ interval: TimeInterval) { diff --git a/Shared/CoreData/Lightweight-Models/PlayableItem.swift b/Shared/CoreData/Lightweight-Models/PlayableItem.swift index 5abe20a2b..717cb641e 100644 --- a/Shared/CoreData/Lightweight-Models/PlayableItem.swift +++ b/Shared/CoreData/Lightweight-Models/PlayableItem.swift @@ -130,7 +130,7 @@ public final class PlayableItem: NSObject, Identifiable { /// Seconds from a chapter's start within which the playhead is still considered to be "at the start". /// Shared by rewind clamping and the previous-chapter button so they stay in lockstep. - public static let chapterStartThreshold: TimeInterval = 3 + private static let chapterStartThreshold: TimeInterval = 3 /// Whether the playhead is close enough to the current chapter's start that a backward action /// should step into the previous chapter rather than restart the current one. From ee6b386e895222e4c3fd3c4abcffff0f0dc0b51a Mon Sep 17 00:00:00 2001 From: Gianni Carlo Date: Sun, 7 Jun 2026 09:12:47 -0500 Subject: [PATCH 3/3] fix tests --- BookPlayerTests/PlayerManagerTests.swift | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/BookPlayerTests/PlayerManagerTests.swift b/BookPlayerTests/PlayerManagerTests.swift index 70dc9a7e5..e9191b628 100644 --- a/BookPlayerTests/PlayerManagerTests.swift +++ b/BookPlayerTests/PlayerManagerTests.swift @@ -24,6 +24,11 @@ class PlayerManagerTests: XCTestCase { UserDefaults.sharedDefaults.removeObject(forKey: Constants.UserDefaults.remainingTimeEnabled) self.playbackServiceMock = PlaybackServiceProtocolMock() + /// Mirror production `PlaybackService.updatePlaybackTime(item:time:)`, which advances the + /// playhead, so seek targets are observable in tests instead of staying frozen. + self.playbackServiceMock.updatePlaybackTimeItemTimeClosure = { item, time in + item.currentTime = time + } self.sut = PlayerManager( libraryService: LibraryServiceProtocolMock(), playbackService: playbackServiceMock, @@ -246,8 +251,9 @@ class PlayerManagerTests: XCTestCase { sut.skipToPreviousChapter() - // Restarts the current chapter rather than stepping back + // Restarts the current chapter (seeks to its start) rather than stepping back XCTAssertEqual(sut.currentItem?.currentChapter?.index, 2) + XCTAssertEqual(sut.currentItem?.currentTime ?? 0, 51.1, accuracy: 0.0001) XCTAssertEqual(playbackServiceMock.getPlayableItemBeforeParentFolderCallsCount, 0) } @@ -259,8 +265,9 @@ class PlayerManagerTests: XCTestCase { sut.skipToPreviousChapter() - // Steps back to the previous chapter + // Steps back to the previous chapter's start XCTAssertEqual(sut.currentItem?.currentChapter?.index, 1) + XCTAssertEqual(sut.currentItem?.currentTime ?? -1, 0.1, accuracy: 0.0001) XCTAssertEqual(playbackServiceMock.getPlayableItemBeforeParentFolderCallsCount, 0) } @@ -272,8 +279,9 @@ class PlayerManagerTests: XCTestCase { sut.skipToPreviousChapter() - // Restarts chapter 1 instead of jumping to the previous item + // Restarts chapter 1 (seeks to its start) instead of jumping to the previous item XCTAssertEqual(sut.currentItem?.currentChapter?.index, 1) + XCTAssertEqual(sut.currentItem?.currentTime ?? -1, 0.1, accuracy: 0.0001) XCTAssertEqual(playbackServiceMock.getPlayableItemBeforeParentFolderCallsCount, 0) } @@ -285,7 +293,8 @@ class PlayerManagerTests: XCTestCase { sut.skipToPreviousChapter() - // Falls back to the previous item + // Falls back to the previous item without seeking within the current one XCTAssertEqual(playbackServiceMock.getPlayableItemBeforeParentFolderCallsCount, 1) + XCTAssertEqual(sut.currentItem?.currentTime ?? -1, 1, accuracy: 0.0001) } }