Keep a paginated copy of a book per layout config - #62
Open
chongfun wants to merge 8 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe cache now stores book indexes and section files per layout configuration. A two-slot LRU registry tracks resident configurations. Adoption purges legacy files, performs safe eviction, and reports failures. Lookup, validation, clearing, firmware integration, documentation, and fault tests were updated. ChangesLayout-configured reader cache
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BookBuilder
participant ReaderCache
participant LayoutConfigRegistry
participant CacheFiles
BookBuilder->>ReaderCache: adopt_layout_config
ReaderCache->>LayoutConfigRegistry: promote active layout key
ReaderCache->>CacheFiles: purge legacy files and evict old files
CacheFiles-->>ReaderCache: cleanup result
ReaderCache-->>BookBuilder: LayoutConfigAdoption
BookBuilder->>ReaderCache: load or build keyed index and sections
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reader-cache/tests/publish_faults.rs`:
- Around line 804-812: Update book_index_present to return the
open_file_in_dir(...).is_ok() result directly, removing the immediately returned
present binding while preserving the existing directory and filename logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4e59d6fb-ef6c-48a6-b5cf-8679d9abef6b
📒 Files selected for processing (5)
docs/ARCHITECTURE.mdfw/src/book_build.rsproto/src/cache.rsreader-cache/src/files.rsreader-cache/tests/publish_faults.rs
chongfun
force-pushed
the
opt/b7-per-config-section-caches
branch
from
August 5, 2026 04:23
8a6eb53 to
3c71f7b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proto/src/cache.rs`:
- Around line 688-727: Update layout_key_of_book_index_file_name,
layout_key_of_section_file_name, and is_legacy_section_file_name to validate and
inspect name.as_bytes() rather than slicing the &str at fixed offsets; introduce
or reuse a byte-oriented hexadecimal parser for the key fields, preserve the
existing length, prefix, suffix, digit, and layout-range checks, and ensure
non-ASCII directory names return None or false without panicking.
In `@reader-cache/src/files.rs`:
- Around line 888-903: Update read_cache_header and book_index_names_unlisted_by
so directory-listing errors are propagated or otherwise reported to the caller
instead of being discarded. When listing the book directory fails, return
CacheHeader::Unreadable, preserving the fail-closed behavior even when stored is
None and registry_present is false.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b81d864b-c1a1-44cd-93ae-1b2cfb621cc3
📒 Files selected for processing (5)
docs/ARCHITECTURE.mdfw/src/book_build.rsproto/src/cache.rsreader-cache/src/files.rsreader-cache/tests/publish_faults.rs
chongfun
force-pushed
the
opt/b7-per-config-section-caches
branch
from
August 5, 2026 05:34
3c71f7b to
10b6a4b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
reader-cache/src/files.rs (1)
787-800: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBound
toc_text_bytesbefore computingtitle_offset.
title_offsetsums four values read from the card.header.toc_text_bytesis an uncheckedu32here, unlikeload_v2_book_labels_and_toc, which gates the same header throughv2_toc_label_bounds_okfirst. A corrupt or foreign index with a largetoc_text_bytesoverflows theu32sum: a debug build panics, and a release build seeks to a wrong offset before the UTF-8 check rejects the result.Apply the same bounds check this file already has.
🛡️ Proposed fix
if header.source_hash != source_identity.0 || header.source_size != source_identity.1 + || !v2_toc_label_bounds_ok(&header) || header.title_text_bytes == 0 || header.title_text_bytes as usize > 64 { return None; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reader-cache/src/files.rs` around lines 787 - 800, Before computing title_offset in the v2 book-index loading path, validate header.toc_text_bytes using the existing v2_toc_label_bounds_ok bounds check already used by load_v2_book_labels_and_toc. Return None when the bounds check fails, then preserve the existing offset calculation and subsequent validation for valid headers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@reader-cache/src/files.rs`:
- Around line 787-800: Before computing title_offset in the v2 book-index
loading path, validate header.toc_text_bytes using the existing
v2_toc_label_bounds_ok bounds check already used by load_v2_book_labels_and_toc.
Return None when the bounds check fails, then preserve the existing offset
calculation and subsequent validation for valid headers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b4c5f68e-d452-4679-9d29-25e53b319e74
📒 Files selected for processing (6)
docs/ARCHITECTURE.mdfw/src/book_build.rsproto/src/cache.rsreader-cache/src/files.rsreader-cache/src/publish.rsreader-cache/tests/publish_faults.rs
B6 made a settings change replay its captured content instead of re-reading the EPUB, and measured the result on X3: 24.7 s for 736 pages, 27.1 s for 1240. That is 2.4-2.6x better than the 64 s cold build, and still 24-27 s of staring at a progress screen -- because nearly all of what is left is downstream of the capture point, in the wrap and the section writes, which no zip/inflate/XML skip can touch. The waste is that the work is thrown away. A book cached one set of files, so every wrap-relevant change overwrote the last one's pagination, and flipping back re-paid the whole replay. With portrait now a first-class flow and the default, the orientation toggle pays it too: the page box is wrap-relevant, so portrait<->landscape rebuilds all sections and overwrites the other orientation's. So name the files for the config that produced them, and keep more than one. Two hex digits of the wrap-relevant bits of `reader_layout_config`: size, weight, family, and the portrait page box. Sections become `SECTIONS/S<cfg><nnn>.BIN` and the book index becomes `BK<cfg>.BIN`, both inside FAT's 8.3 budget (`CACHE_SECTION_FILE_BYTES` 8 -> 10). The roadmap asked for a page-box bit to be *added* to the config, on the grounds that `reader_layout_config` predates portrait. It does not anymore -- READER_LAYOUT_VERSION v18 put one in bit 7 -- so the key only had to include it. Two axes stay out of the name on purpose. Spacing, because a spacing change re-walks heights over the same wrap points, so both spacings share one set of files exactly as they did before. And the wrap version plus panel salt, because a bump there has to retire *every* config: leaving them out means a bump rejects each index in its own header check and rebuilds in place, rather than stranding a fresh set of filenames per bump with no one left to delete the old. No signature took a config argument. Every per-config reader and writer derives the key from the `ReaderStore` it already holds, so no caller can name a file for one config and fill it with another's pagination. The three name parsers read bytes rather than slicing `&str`. A FAT short name is raw bytes and embedded-sdmmc renders them ISO-8859-1 (`c as char`), so a directory entry carrying a byte >= 0x80 arrives as a multi-byte char; slicing at the fixed field offsets would land inside one, and on the device a panic is a reset. A foreign or corrupt name in a cache directory is not a reason to reboot. A 10-byte file per book listing the resident configs, most recently used first. It does two jobs. Eviction: section files multiply per config, so a book holds at most CACHE_CONFIG_SLOTS (2) of them -- the flip and the flip back, which is the flow that hurts -- and a third arriving deletes the least recently used one's index and section files, nothing else. Ordering by *use* rather than insertion is what keeps the config a reader has just returned to off the block. And it tells the readers that want a book's config-*independent* facts which index file is there: source identity for the orphan sweep and the clear, the title for the Library list, the TOC and labels a replay has to carry into the index it rewrites. Those three used to open one fixed BOOK.BIN. `read_cache_header` is the one that had to be careful, because its `Absent` is what licenses the clear path to delete a cache directory -- against a 28-bit key whose collisions the format admits. It works the registry's slots, then falls back to a bounded directory listing, so a lost registry cannot make a cache that is plainly there read as deletable; a registry present but undecodable, an index that opens and will not parse, *or a listing that would not run* all read as `Unreadable` instead. That last one is why the listing returns an option rather than an empty vector: a card that refused to enumerate has not said the directory is empty, and reading it that way would hand the clear a delete it had not earned. `Absent` is reached only when nothing is there at all. `read_cached_book_title` skips the listing deliberately -- it runs once per book at catalog scan, and a miss there costs a label, not a cache. The other readers of config-independent facts share that miss semantics; `read_cache_header` is the only one where a failed listing has to be told from an empty directory. `adopt_layout_config` called `delete_layout_config_artifacts_in` and then wrote the promoted registry regardless: the helper returned nothing, and both the index delete and the section sweep discarded their failures. A card that refused a delete -- an I/O fault, or no space to write back the directory entry -- got a registry that said the eviction happened. That unregisters an intact cache. The surviving files still load if the reader flips back to that config (the per-config load names `BK<cfg>.BIN` directly rather than through the registry), but nothing counts them anymore: the next eviction takes one of the two the registry does name and walks past a full section set that no later pass will ever look at again. Repeat it under intermittent failures and the two-config storage bound is gone. It is worst exactly when the delete was needed for space -- the build that follows can then fail for want of it, on a card whose older, intact cache the registry has just hidden. So the deletion path answers the question the registry write depends on. `delete_layout_config_artifacts_in` returns whether every one of that config's files is gone -- deleted here or already absent -- and reads false on anything it cannot account for: a refused delete, a `SECTIONS/` that would not open, a sweep that did not finish. `sweep_section_files` returns the same kind of answer, true only when a listing came back with nothing left matching `wanted`. The index still goes first, so an interrupted eviction leaves an unreadable set (stray sections, no index) rather than an index promising sections that are gone. On failure the adoption is abandoned and the stored registry left untouched, so it still names the config whose files are still there. That config stays counted, and the next open under this one retries the same deletes. `LayoutConfigAdoption::eviction_failed` names it; `evicted` is set only after the deletion actually finished. The alternative -- failing the open -- was not taken. Eviction only runs when the config is not resident, which is exactly when a build was going to happen anyway, so aborting would mean refusing to open the book because a delete failed. The build proceeds and writes under a config the registry does not name; that index is still found by the next open under the same config, and by the unlisted-index scan, and it becomes registered as soon as an eviction succeeds. What can no longer happen is a registry claiming an eviction that did not occur. The registry write itself can also refuse, and that one is not fatal either -- for the same reason -- but it does need repairing rather than shrugging off. `CFG.BIN` is opened create-or-truncate, so a refused write leaves not the previous registry but nothing decodable. Reading that as an empty registry is what would lose the bound: every config on the card stops being counted at once, so no eviction ever takes them and each later open is free to stack another full section set on top. One failed small write would start an accumulation that only a manual clear ends. So a `CFG.BIN` that is there and will not decode is rebuilt from the index files the directory actually holds, using the same bounded listing `read_cache_header` falls back to. Ordering cannot be recovered -- a listing does not say which config was read last -- but the count can, and the count is what eviction needs. A card already over the bound leaves the extras unnamed for one more open; the eviction that follows brings the number down, and later opens converge. A registry that is not there *at all* still starts empty, so a book being opened for the first time does not pay for a listing. `LayoutConfigAdoption::registry_write_failed` reports the refusal so the log names a card that is failing small writes. Rebasing onto #59 put a second deleter in the same directory. That change prunes the section files a completed rebuild stranded: sections are numbered by a dense `0..count` ordinal, so a rebuild deriving fewer of them leaves the tail past its new count referenced by nothing, and the publish tail deletes it once the index has landed. Per-config naming breaks the assumption underneath it. It matched `S###.BIN` and ranged over the ordinal alone, which was sound when a book had one paginated copy. Now there are two, each numbering its sections from zero, so their ordinal ranges overlap completely -- and the config whose sections sit past the publishing config's count is not stranded at all. Left as it was, the prune deletes a working paginated copy of the book, which is the one thing this whole change exists to keep. So the prune matches on both halves of the name. `section_file_name_parts` returns the key and the ordinal from one parser, `layout_key_of_section_file_name` is now expressed through it, and `prune_orphan_sections` takes the key from the same `ReaderStore` whose pagination it just published rather than from an argument -- the convention above, for the same reason. Two smaller consequences. The keyed name is ten bytes where the old one was eight, so `S###.BIN` no longer parses as a section at all: the prune leaves the pre-per-config files alone, which is right, because retiring those is the legacy purge's job and it is gated on `BOOK.BIN`. And the prune would otherwise have gone quietly dead rather than wrong -- nothing on the card would have matched its pattern any more, so #59's insurance would have been silently spent. The roadmap suggested bumping CACHE_V2_VERSION since the index name changed. It is not needed and it would cost something: nothing reads the old names, so a bump would only invalidate CONT.BIN too and turn the one-time transition from a replay into a full EPUB re-parse. Instead the first open that finds an unkeyed BOOK.BIN deletes it and the unkeyed `S<spine>.BIN` files. The check is one failed file open on every later open, since nothing writes that name again. Which makes `BOOK.BIN` the retry marker, and it is deleted last, once the section sweep reports itself finished. Taking it first would strand whatever the sweep could not: nothing reads the unkeyed names, no registry counts them, and nothing else would ever bring an open back to look. A marker left behind costs one failed file open per later open and buys the retry; nothing reads `BOOK.BIN` itself anymore, so a marker outliving its sections misleads no reader. The purge still returns only whether there was anything to purge -- no caller needs the stronger answer -- but it no longer discards the sweep result that decides when the marker may go. TOC.BIN, COVER.BIN, CONT.BIN and the reading position do not depend on the layout config and are untouched by eviction and by the purge. Flash +7,082 B text on X3 (6,704,500 -> 6,711,582): `.text` +6,802 and `.rodata` +280. X4 costs the same to within 2 bytes: +7,084 B (6,689,378 -> 6,696,462), `.text` +6,804 and `.rodata` +280. No new static RAM on either device. `data` (6,644 both) and `bss` (326,877 on X3, 322,645 on X4) are byte-identical to the baseline, as is `.dram2_prev_fb` (52,273 on X3, 48,001 on X4). Measured for the whole branch against its merge base with `llvm-size`, one device at a time in this same worktree -- both traps that manufacture fake deltas here, an X3 binary left at the shared output path and a baseline built in a different directory shifting `.rodata`, avoided. On the card, one more paginated copy of the book -- the section files are the bulk, and the 2-slot cap is what bounds it. Stack: the deepest frames are unchanged. `adopt_layout_config` peaks around 400 B, dominated by a 16x12-byte name-staging vector modelled on `empty_sections_dir`'s, and it runs and returns before the EPUB chain's 13,840 B frame rather than nesting inside it. proto: key derivation (the orientation flip must land on a different key, a version bump must not), name recognition against both schemes and near-misses, registry ordering and eviction, encode round-trip and the decodes that must be rejected -- out-of-range key, repeated key, short buffer. Both halves of a section name round-trip for every key and ordinal the prune ranges over, and the legacy eight-character form is rejected outright. Plus three crafted non-ASCII names, each the exact byte length its parser accepts and each putting a char boundary inside a field offset that parser reads: the case the byte-wise rewrite above exists for. Against the old slicing all three panic. reader-cache, on the existing FAT16 fault-injection harness: a second config does not overwrite the first and the flip back loads without a rebuild; a third config evicts the least recently used one and leaves the survivor and COVER.BIN alone; re-reading a resident config moves it off the eviction block; the legacy purge takes both old artifacts and spares the settings-independent ones; a cache whose registry was deleted still names its book; a registry that is there and unusable fails closed as `Unreadable`; and a clear with two configs resident reports success, which it can only do by reclaiming every per-config file, since it verifies by listing the directory. Two more for the eviction reporting, one for a blocked index delete and one for a blocked section delete after the index went. They block a named file by holding it open, which this embedded-sdmmc rev refuses to reopen -- the same `RemoveStatus::Failed` an I/O fault produces, aimed at one file instead of the Nth write. Each asserts that nothing is reported evicted, that the failure is named, that the files survive, and that a retry once the block clears evicts the same key, which is what proves the registry still named it. And four for the failure paths above. A refused registry write, blocked the same way, is reported rather than swallowed. A registry truncated exactly as a refused write leaves it still evicts on the next open, which an empty reading could not do. A legacy sweep blocked mid-pass keeps `BOOK.BIN`, and the retry once the block clears takes both the section and the marker. And a property test: `read_cache_header` is run once per read the pass makes, with that read failed, and none of them may answer `Absent` for a cache that is on the card -- `Unreadable` is fine, since it fails closed. Each of the three behavioural ones fails against the code before it: the sweep test on the stranded marker, the registry test on an eviction that never happens, and the property test on a concrete fault position that produced `Absent`. And one for the prune, which #59's own four do not cover because before this change there was only ever one config: a shrinking rebuild under the second config must take its own stranded tail and leave every one of the first config's sections. It fails against an ordinal-only prune on the first section it finds missing. #59's tests came across keyed -- its helper now names section files through the config the store is holding, so a drifting key cannot leave them passing against the wrong file -- and its stray-name test grew an unkeyed `S003.BIN`, whose ordinal is inside the pruned range, to pin that the prune leaves the legacy scheme to the purge. Verified with `tools/check.sh fast`, `tools/check.sh firmware` (X4 and X3) and `tools/check.sh fmt`, all green, plus clippy on `proto` and `reader-cache` with their tests. The emulator leg was not re-run: nothing here touches layout, rendering, typography or the golden frames. `publish.rs`'s provisional-publish failure path still calls `empty_cache_dir`, so a torn index write for one config now takes the other config's good cache with it. Scoping that to the failing config is the right fix; it changes a fault path with tests pinning it, and the current behaviour degrades to a rebuild rather than to anything incorrect, so it is left out of this diff. Two books whose 28-bit keys collide can now hold indexes under different configs in the same directory, where the single-file scheme could only let one overwrite the other. `read_cache_header` answers with the first index that reads, so it may report the wrong book's header, and a clear that does match deletes the colliding book's files along with its own. The identity stamp still stops the clear from deleting against a header it cannot match, and the collision was already destructive before this change, so this is a pre-existing limitation of the key width rather than a regression. Fixing it properly means passing source identity into adoption and purging derived files whose index belongs to another source, which is its own change.
chongfun
force-pushed
the
opt/b7-per-config-section-caches
branch
from
August 6, 2026 05:06
10b6a4b to
3c3e238
Compare
…G.BIN preservation - Gate new cache build/replay output on successful layout config adoption (adoption.succeeded()), preventing unadopted per-layout files from accumulating on disk. - Make layout_registry_from_index_files fallible and explicitly delete excess non-surviving configurations when >2 indexes are found during reconstruction. - Defer CFG.BIN deletion in empty_cache_dir until per-config indexes and sections are deleted and cleared == true. - Validate v2_toc_label_bounds_ok before computing title_offset in read_v2_book_title. - Remove orphan conflict marker from docs/ARCHITECTURE.md. - Add integration tests for CFG.BIN preservation on failed clear and excess config deletion during reconstruction.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
- Gate identity marker (TOC.BIN, CFG.BIN) removal in purge_colliding_cache_artifacts_in on successful deletion of all non-marker artifacts (SECTIONS/, BOOK.BIN, COVER.BIN, CONT.BIN) and per-config index files (BK*.BIN). - Re-structure book_dir_ownership to iterate all entries using String::<SHORT_NAME_BYTES> to prevent format errors on 9+ byte filenames (e.g. COVER.BIN) and separate entry collection from file opening to avoid file-locking conflicts during iteration. - Enforce strict ownership return precedence: Mismatch -> Unreadable -> Match. - Add integration tests covering mixed unreadable index handling, 5-index unlisted collision inspection, and collision purge failure retries.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reader-cache/src/files.rs`:
- Around line 2017-2018: Update the logging in the book-open flow around
book_dir_ownership and the corresponding registry-read log to remove the
DEBUG-prefixed messages or change them to the existing “cache: ...” convention.
Apply the same treatment to the logs at the referenced ownership and
registry-read points, preserving only useful operational logging.
In `@reader-cache/tests/publish_faults.rs`:
- Line 2191: Replace the hardcoded "SECTIONS" argument in the book.open_dir call
with the existing CACHE_SECTIONS_DIR constant, preserving the current expect
handling.
- Around line 2255-2274: Assert that k1, k2, k3, k4, and k5 are pairwise
distinct immediately after creating them, following the pattern used by the
sibling test. Preserve the existing layout changes and key creation flow while
ensuring any collision causes the test to fail.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 11d78435-752d-4e86-83f2-f03bf784d5e9
📒 Files selected for processing (3)
proto/src/cache.rsreader-cache/src/files.rsreader-cache/tests/publish_faults.rs
…very - Update book_dir_ownership and read_cache_header to inspect legacy BOOK.BIN for source identity when present. - A mismatching BOOK.BIN in book_dir_ownership returns Mismatch, triggering collision purge which clears BOOK.BIN, legacy section files, and COVER.BIN. - read_cache_header discovers BookV2Header identity from legacy BOOK.BIN so clear and orphan cleanup routines can verify source identity before authorizing deletion. - Add integration tests for adopting and clearing legacy-only caches belonging to colliding sources.
…ions reliably - Defer legacy BOOK.BIN deletion in purge_colliding_cache_artifacts_in until after content files succeed so failed purges retain durable identity markers for retry. - Scan complete directory inventory in read_cache_header before returning Present, failing closed as Unreadable if any artifact is unreadable or mismatches identity. - Include position files (POS.BIN, POSA/B.BIN) in collision purge so displaced owner positions are not inherited by colliding books. - Add integration tests covering legacy cover failure retries, multi-marker unreadability precedence, and position file clearing on collision adoption.
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
1 participant
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.
Pull Request
Verification
Before requesting review, please confirm you have run the appropriate checks:
tools/check.sh fmtpassedtools/check.sh fastpassed (host Clippy and tests)tools/check.sh emulatorpassed (emulator tests and goldens)tools/check.sh firmwarepassed (firmware Clippy and release builds)tools/check.sh allpassed (required before the pull request is considered ready)Skipped checks
If you skipped any checks, please list them below and explain why:
Description
B6 made a settings change replay its captured content instead of re-reading the EPUB, and measured the result on X3: 24.7 s for 736 pages, 27.1 s for 1240. That is 2.4-2.6x better than the 64 s cold build, and still 24-27 s of staring at a progress screen because nearly all of what is left is downstream of the capture point, in the wrap and the section writes, which no zip/inflate/XML skip can touch.
The waste is that the work is thrown away. A book cached one set of files, so every wrap-relevant change overwrote the last one's pagination, and flipping back re-paid the whole replay. With portrait now a first-class flow and the default, the orientation toggle pays it too: the page box is wrap-relevant, so portrait<->landscape rebuilds all sections and overwrites the other orientation's.
So name the files for the config that produced them, and keep more than one.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation