Merge develop into fix-issue-579 - #893
Closed
mauricecarrier7 wants to merge 40 commits into
Closed
Conversation
Co-authored-by: Mickaël Menu <mickael.menu@gmail.com>
Brings the readium#579 series up to date with develop (39 commits), resolving the four conflicts introduced by the 4.0 concurrency work. - ZIPFoundationContainer: kept develop's @sendable consume + native Task.isCancelled guard; the out-of-range clamp merged cleanly on top. - ReadError: kept both develop's Sendable conformance on AccessError and the series' isCancellation. wrap(_:) normalises at the boundary; isCancellation is a predicate over an existing error that also unwraps .decoding(...). - AudioNavigator: kept develop's timeObserverToken/notificationTask, which supersede the series' own timeObserver/playerItemEndObserver, and carried the item-status and likely-to-keep-up observers, which develop lacks. AudioNavigator is @mainactor on develop and KVO callbacks arrive off it, so both handlers now hop, and the publication/resourceIndex read moved inside the hop. - LCPDecryptor: took develop's file wholesale and re-applied the chunking around its arithmetic, so develop's ranged decryption is preserved byte-for-byte and only the chunking loop is new.
Member
|
FYI it's at the top of my list after an LCP CRL fix, I'll make sure it's merged in |
Author
Thanks, good to know. No rush on our side; we're on the 3.11.0 rebase and unaffected until we move to the 4.0 line. Happy to re-test on develop once it's in. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Hi @mickael-menu — this brings
fix-issue-579up to date withdevelopso #880can merge again. It adds no new behaviour — the only code changes are the ones
the conflicts forced, and they're listed below.
The branch is 39 commits behind
developand #880 currently shows asconflicting. We ship your series in Palace (rebased onto 3.11.0), so we needed
this merge for ourselves anyway — sharing it in case it saves you the time.
What conflicted, and how it's resolved
Four files, all from the 4.0 concurrency work:
develop's@Sendable consumeand nativeTask.isCancelledguard; your out-of-range clamp applied cleanly on top.develop'sSendableconformance onAccessErrorand your
isCancellation. They do different jobs:wrap(_:)normalises at theboundary, while
isCancellationis a predicate over an existing error thatalso unwraps
.decoding(...).develop'stimeObserverToken/notificationTask,which supersede the series' own
timeObserver/playerItemEndObserver, andcarried over the item-status and
isPlaybackLikelyToKeepUpobservers, whichdevelopdoesn't have. The class is@MainActornow and KVO callbacks arriveoff it, so both handlers hop, and the
publication/resourceIndexread movedinside the hop.
develop's file wholesale and re-applied the chunkingaround its arithmetic, so its ranged decryption is preserved byte-for-byte and
only the chunking loop is new.
The net diff against
developis the same 12 files as #880 — the merge brings innothing else.
Verification
port of this series onto the
4.0.0-alpha.1tag (which differs fromdevelopby two commits, neither touching LCP): playback starts from the licence alone,
with only the 2.5 KB
.lcplon disk and no.lcpafetched. The same title onthe same build with streaming switched off downloads the full 778 MB package
first.
A couple of notes
Steven's
getOrNilsuggestion lands on a line this merge moved: thereadingOrder[resourceIndex]read now sits inside the@MainActorhop(inside
observe(currentItem:)). I left the behaviour alone so this stays a pure merge — happyto apply it if you'd like it folded in.
Two of the patches have no test coverage — the resource caching in
PublicationMediaLoader, and the.loadingstate correction inAudioNavigator. We checked by reverting each one: removing the caching patchleaves all 1,534 remaining tests green.
Tests/LCPTestsalso isn't a target inPackage.swift, soLCPDecryptionTestsnever runs. Happy to write tests for anyof these if it would help.
Thanks for the fix — it's the reason audiobook streaming works for our users.