feat: slim footnotes support#988
feat: slim footnotes support#988Uri-Tauber wants to merge 6 commits intocrosspoint-reader:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds footnote support across the EPUB stack: new FootnoteEntry type, per-page footnote collection and I/O, HTML parser detection/collection, reader UI for listing/navigating footnotes, href→spine resolution, menu/localization entries, and navigation state save/restore for footnote jumps. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Parser as ChapterHtmlSlimParser
participant Page
participant Reader as EpubReaderActivity
participant FootnotesView as EpubReaderFootnotesActivity
User->>Parser: Provide chapter HTML
activate Parser
Parser->>Parser: Detect internal <a href> and collect display text & href
Parser->>Page: Transfer pending footnotes when word threshold met
deactivate Parser
User->>Reader: Open menu -> Select FOOTNOTES
Reader->>Page: Read current page footnotes
Reader->>FootnotesView: Launch with footnote list
activate FootnotesView
FootnotesView->>User: Render list / handle selection
User->>FootnotesView: Select footnote
FootnotesView->>Reader: onSelectFootnote(href)
deactivate FootnotesView
activate Reader
Reader->>Reader: save current position
Reader->>Reader: resolveHrefToSpineIndex(href)
Reader->>Reader: navigateToHref -> load target section/page
Reader->>User: Display target page
deactivate Reader
User->>Reader: Press BACK
activate Reader
Reader->>Reader: restoreSavedPosition()
Reader->>User: Return to original spine/page
deactivate Reader
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp (1)
325-367: Consider implementing the data- href extraction TODO.*Right now
javascript:void(0)links are dropped, so EPUBs that store hrefs in data attributes will be missed. Want me to sketch a parser for the common data-* patterns?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
lib/Epub/Epub.cpplib/Epub/Epub.hlib/Epub/Epub/FootnoteEntry.hlib/Epub/Epub/Page.cpplib/Epub/Epub/Page.hlib/Epub/Epub/Section.cpplib/Epub/Epub/blocks/TextBlock.hlib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpplib/Epub/Epub/parsers/ChapterHtmlSlimParser.hlib/I18n/I18nKeys.hlib/I18n/translations/english.yamlsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderActivity.hsrc/activities/reader/EpubReaderFootnotesActivity.cppsrc/activities/reader/EpubReaderFootnotesActivity.hsrc/activities/reader/EpubReaderMenuActivity.h
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-02-16T20:43:19.339Z
Learnt from: Levrk
Repo: crosspoint-reader/crosspoint-reader PR: 909
File: src/activities/reader/EpubReaderActivity.cpp:461-461
Timestamp: 2026-02-16T20:43:19.339Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, when using ConfirmationActivity with enterNewActivity(), it's not necessary to call exitActivity() beforehand. The ConfirmationActivity operates as a modal dialog and cleanup is handled via the pendingSubactivityExit flag in the callback lambda. This differs from other activities like EpubReaderChapterSelectionActivity or KOReaderSyncActivity which do require exitActivity() before enterNewActivity().
Applied to files:
src/activities/reader/EpubReaderMenuActivity.hsrc/activities/reader/EpubReaderFootnotesActivity.cppsrc/activities/reader/EpubReaderFootnotesActivity.hsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderActivity.h
📚 Learning: 2026-02-18T15:43:07.515Z
Learnt from: GavinHigham
Repo: crosspoint-reader/crosspoint-reader PR: 966
File: src/activities/ActivityWithSubactivity.h:20-21
Timestamp: 2026-02-18T15:43:07.515Z
Learning: In the crosspoint-reader activity architecture, when using ActivityWithSubactivity, the currentActivity global variable remains at the top-level activity (e.g., ReaderActivity) even when nested subactivities are active. The supportsOrientation() delegation chain starts from the top-level activity and stops when it reaches a subactivity that overrides the method (e.g., EpubReaderActivity returns true). Deeper nested subactivities (like EpubReaderChapterSelectionActivity or EpubReaderPercentSelectionActivity) do not need to override supportsOrientation() because they never become the currentActivity and the delegation chain doesn't reach them.
Applied to files:
src/activities/reader/EpubReaderMenuActivity.hsrc/activities/reader/EpubReaderFootnotesActivity.cppsrc/activities/reader/EpubReaderFootnotesActivity.hsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderActivity.h
📚 Learning: 2026-02-12T06:57:35.955Z
Learnt from: CaptainFrito
Repo: crosspoint-reader/crosspoint-reader PR: 725
File: src/activities/network/CalibreConnectActivity.cpp:218-264
Timestamp: 2026-02-12T06:57:35.955Z
Learning: In src/activities/network/CalibreConnectActivity.cpp, button hints (« Exit) are intentionally omitted from the SERVER_STARTING and ERROR states—only the SERVER_RUNNING state displays navigation hints. This is a deliberate design decision by the author.
Applied to files:
src/activities/reader/EpubReaderActivity.cpp
🧬 Code graph analysis (6)
src/activities/reader/EpubReaderFootnotesActivity.cpp (2)
src/activities/reader/EpubReaderActivity.cpp (4)
loop(121-273)loop(121-121)render(510-645)render(510-510)lib/Epub/Epub/Page.cpp (6)
render(6-8)render(6-6)render(28-31)render(28-28)render(51-55)render(51-51)
lib/Epub/Epub.h (1)
lib/Epub/Epub.cpp (2)
resolveHrefToSpineIndex(850-875)resolveHrefToSpineIndex(850-850)
src/activities/reader/EpubReaderFootnotesActivity.h (2)
src/activities/ActivityWithSubactivity.h (1)
ActivityWithSubactivity(6-21)src/activities/reader/EpubReaderFootnotesActivity.cpp (8)
onEnter(12-16)onEnter(12-12)onExit(18-18)onExit(18-18)loop(20-51)loop(20-20)render(53-95)render(53-53)
src/activities/reader/EpubReaderActivity.cpp (3)
src/main.cpp (4)
exitActivity(136-142)exitActivity(136-136)enterNewActivity(144-147)enterNewActivity(144-144)src/activities/ActivityWithSubactivity.cpp (6)
exitActivity(20-27)exitActivity(20-20)enterNewActivity(29-34)enterNewActivity(29-29)requestUpdate(42-47)requestUpdate(42-42)src/activities/Activity.cpp (2)
requestUpdate(42-48)requestUpdate(42-42)
lib/Epub/Epub/Page.cpp (1)
lib/Serialization/Serialization.h (1)
writePod(8-10)
src/activities/reader/EpubReaderActivity.h (1)
src/activities/reader/EpubReaderActivity.cpp (4)
navigateToHref(816-856)navigateToHref(816-816)restoreSavedPosition(858-873)restoreSavedPosition(858-858)
🔇 Additional comments (24)
lib/Epub/Epub/Section.cpp (1)
12-12: LGTM!Version bump to 14 correctly invalidates existing section caches, forcing re-parsing with the new footnote serialization format.
lib/Epub/Epub/FootnoteEntry.h (1)
5-13: LGTM!Fixed-size buffers are appropriate for this embedded context, avoiding heap allocations in the hot path. The buffer sizes (24 for number, 64 for href) align with the parser's internal buffers.
lib/Epub/Epub/blocks/TextBlock.h (1)
31-31: LGTM!Clean accessor method enabling word count tracking for footnote page assignment.
lib/I18n/translations/english.yaml (1)
19-19: LGTM!Translation entry correctly added and positioned to match the enum ordering in I18nKeys.h.
src/activities/reader/EpubReaderActivity.h (1)
28-46: LGTM!Well-structured footnote navigation state. The save/restore pattern using
-1sentinels forsavedSpineIndex/savedPageNumbercorrectly handles the "no saved position" case, as confirmed by the implementation innavigateToHref()andrestoreSavedPosition().lib/Epub/Epub.h (1)
75-75: LGTM!Clean API addition for resolving hrefs to spine indices. The implementation correctly handles anchor stripping and provides both exact and filename-only matching for robustness across varying EPUB path conventions.
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.h (1)
68-75: LGTM!Footnote link tracking state is well-designed. Fixed buffer sizes match
FootnoteEntrystruct (24/64 bytes), and pairing footnotes with word indices enables accurate page assignment during the single-pass parse.lib/I18n/I18nKeys.h (1)
53-53: LGTM!Enum value correctly positioned to match the translation file ordering.
lib/Epub/Epub/Page.cpp (1)
106-123: Footnote deserialization guards look solid.Count validation, read checks, and explicit null-termination are all good.
src/activities/reader/EpubReaderMenuActivity.h (2)
15-15: MenuAction updated cleanly with FOOTNOTES.
42-46: Menu list integration for FOOTNOTES looks good.lib/Epub/Epub.cpp (1)
850-874: Href-to-spine resolution logic looks consistent with intended navigation.src/activities/reader/EpubReaderFootnotesActivity.cpp (1)
12-51: Input handling and selection bounds are clean.src/activities/reader/EpubReaderActivity.cpp (4)
190-195: Back-press handling for footnote restore is well-contained.
386-402: Footnotes menu action wiring looks correct.
636-638: Capturing page footnotes during render looks good.
816-873: navigateToHref/restoreSavedPosition flow is clear and safe.src/activities/reader/EpubReaderFootnotesActivity.h (1)
12-34: Footnotes activity interface is tidy and self-contained.lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp (5)
52-68: Attribute helper and internal-link filter are straightforward.
138-139: ResettingwordsExtractedInBlockon new blocks makes sense.
516-527: Footnote label normalization logic looks good.
801-811: Pending-footnote transfer inaddLineToPageis clear.
850-858: Fallback footnote transfer is a good safety net.lib/Epub/Epub/Page.h (1)
59-69: The null-pointer guard is unnecessary;numberandhrefare char array members, not pointers.
addFootnote()is called only withFootnoteEntrystruct members (it->second.number,it->second.href, andfn.number,fn.href). These are addresses of char arrays within the struct, not pointer members. SinceFootnoteEntry's constructor initializes both fields as empty strings and entries are only added after safestrncpywith null-termination, null pointers cannot reachaddFootnote(). The suggested guard is redundant.Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/Epub/Epub/Page.cpp`:
- Around line 70-79: serialize() writes the full footnotes.size() which can
exceed the 16-entry limit assumed by deserialize() and addFootnote(); fix
serialize() to clamp the written count to 16 (e.g., write uint16_t count =
std::min<uint16_t>(footnotes.size(), 16) instead of fnCount) and iterate only up
to that clamped count when writing fn.number and fn.href (the loop that writes
each footnote), optionally logging a warning when footnotes.size() > 16; refer
to the footnotes member, serialize()/deserialize() methods, the fnCount
variable, and the code that writes fn.number/fn.href to locate the change.
In `@lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp`:
- Around line 649-660: When closing a footnote link in the block that checks
insideFootnoteLink and footnoteLinkDepth, include the cumulative
wordsExtractedInBlock offset when computing wordIndex so the pendingFootnotes
entry uses a global word index; currently wordIndex only uses
currentTextBlock->size() which is relative. Update the calculation that assigns
wordIndex (used when pushing into pendingFootnotes) to add wordsExtractedInBlock
to the current block size (respecting the existing null checks and casts), so
FootnoteEntry indices (entry.number/entry.href) are compared correctly against
wordsExtractedInBlock later.
In `@src/activities/reader/EpubReaderFootnotesActivity.cpp`:
- Around line 53-94: In EpubReaderFootnotesActivity::render replace the two
hardcoded UI strings with localized string IDs: add new StrId entries (e.g.
STR_NO_FOOTNOTES_ON_PAGE and STR_LINK_PLACEHOLDER) in the i18n resources and use
tr(STR_NO_FOOTNOTES_ON_PAGE) instead of "No footnotes on this page" and
tr(STR_LINK_PLACEHOLDER) instead of "[link]" where label is set; keep the
existing logic for empty label fallback and ensure any tests/resources are
updated to include the new StrId keys.
Uri-Tauber
force-pushed
the
feat/slim-footnotes-support
branch
from
d6150f1 to
d9acb21
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements basic footnote support for EPUB files, allowing users to navigate from footnote links within the text to their corresponding content and back. The implementation is intentionally simplified compared to PR #553, removing complex features like virtual spine items, two-pass parsing, and extracted footnote content in favor of a single-pass approach with fixed-size buffers.
Changes:
- Added
FootnoteEntrystruct with fixed-size buffers (24 bytes for display text, 64 bytes for href) to track footnote links - Enhanced HTML parser to detect internal
<a href>links, collect them as footnotes, and assign them to pages based on word-count tracking - Added footnotes menu UI (
EpubReaderFootnotesActivity) accessible from the reader menu to display and navigate footnotes on the current page - Implemented navigation state management to save/restore reading position when jumping to footnotes via the Back button
- Updated Page serialization to include footnotes and bumped section cache version to 14
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
lib/Epub/Epub/FootnoteEntry.h |
New POD struct defining fixed-size footnote entry (number[24], href[64]) |
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.h |
Added state tracking for footnote link parsing and word counting |
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp |
Implemented link detection, text normalization, and page assignment logic in single-pass parser |
lib/Epub/Epub/blocks/TextBlock.h |
Added wordCount() accessor for word-based footnote tracking |
lib/Epub/Epub/Page.h |
Added footnotes vector and addFootnote() method with 16-entry limit |
lib/Epub/Epub/Page.cpp |
Implemented footnote serialization/deserialization with validation |
lib/Epub/Epub/Section.cpp |
Bumped SECTION_FILE_VERSION to 14 to invalidate old cache |
lib/Epub/Epub.h |
Added resolveHrefToSpineIndex() method declaration |
lib/Epub/Epub.cpp |
Implemented href-to-spine resolution with filename matching fallback |
src/activities/reader/EpubReaderMenuActivity.h |
Added FOOTNOTES menu action to reader menu |
src/activities/reader/EpubReaderFootnotesActivity.h |
New activity header for footnotes list UI |
src/activities/reader/EpubReaderFootnotesActivity.cpp |
Implemented scrollable footnote list with selection handling |
src/activities/reader/EpubReaderActivity.h |
Added footnote state tracking (saved position, viewing flag) |
src/activities/reader/EpubReaderActivity.cpp |
Implemented footnote navigation, position save/restore, and Back button integration |
lib/I18n/translations/english.yaml |
Added STR_FOOTNOTES, STR_NO_FOOTNOTES, STR_LINK strings |
lib/I18n/I18nKeys.h |
Added corresponding enum entries for new strings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Deserialize footnotes | ||
| uint16_t fnCount; | ||
| serialization::readPod(file, fnCount); | ||
| if (fnCount > 16) { |
There was a problem hiding this comment.
The constant 16 is used in three places (addFootnote limit check, serialize clamp, deserialize validation) to cap the number of footnotes per page. Consider defining a named constant like 'MAX_FOOTNOTES_PER_PAGE' in FootnoteEntry.h or as a static constexpr in the Page class to avoid magic numbers and ensure consistency if this limit needs to be changed in the future.
|
Works in my testing. And code looks good to me |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/activities/reader/EpubReaderActivity.cpp (1)
386-402: Minor: RedundantrequestUpdate()call.The navigation callback calls
navigateToHref(href, true)which already invokesrequestUpdate()at the end of its implementation (line 851). The subsequent call on line 399 is redundant. While harmless due toeIncrementsemantics, it could be removed for clarity.♻️ Optional cleanup
[this](const char* href) { // Navigate to selected footnote navigateToHref(href, true); exitActivity(); - requestUpdate(); }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/reader/EpubReaderActivity.cpp` around lines 386 - 402, In the EpubReaderMenuActivity::MenuAction::FOOTNOTES case remove the redundant requestUpdate() inside the footnote navigation lambda because navigateToHref(href, true) already calls requestUpdate(); locate the lambda passed to the EpubReaderFootnotesActivity constructor (the [this](const char* href) callback), delete the trailing requestUpdate() call while keeping the call to navigateToHref(href, true) and the exitActivity() invocation to preserve flow.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/Epub/Epub/Page.cpplib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpplib/I18n/I18nKeys.hlib/I18n/translations/english.yamlsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderActivity.hsrc/activities/reader/EpubReaderFootnotesActivity.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/activities/reader/EpubReaderFootnotesActivity.cpp
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 988
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:649-661
Timestamp: 2026-02-19T12:17:00.962Z
Learning: In ChapterHtmlSlimParser.cpp, when computing footnote word indices in endElement() for footnote links, the wordIndex must be cumulative across the 750-word mid-paragraph flush boundary. The correct calculation is: `wordIndex = wordsExtractedInBlock + currentTextBlock->size()`, not just `currentTextBlock->size()`. This ensures footnotes attach to the page containing their actual anchor word, even after layoutAndExtractLines(false) has extracted and removed earlier words from the block.
📚 Learning: 2026-02-16T22:25:35.674Z
Learnt from: whyte-j
Repo: crosspoint-reader/crosspoint-reader PR: 733
File: lib/I18n/I18nKeys.h:271-272
Timestamp: 2026-02-16T22:25:35.674Z
Learning: In the crosspoint-reader i18n system (lib/I18n/), missing translation keys in non-English YAML files are automatically filled with English strings at build time by the gen_i18n.py script. All generated string arrays (STRINGS_EN, STRINGS_ES, etc.) have identical lengths, so runtime array indexing is safe without per-string fallback logic. The system prints "INFO: '{key}' missing in {lang_code}, using English fallback" during code generation when this occurs.
Applied to files:
lib/I18n/translations/english.yamllib/I18n/I18nKeys.h
📚 Learning: 2026-02-19T12:17:00.962Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 988
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:649-661
Timestamp: 2026-02-19T12:17:00.962Z
Learning: In ChapterHtmlSlimParser.cpp, when computing footnote word indices in endElement() for footnote links, the wordIndex must be cumulative across the 750-word mid-paragraph flush boundary. The correct calculation is: `wordIndex = wordsExtractedInBlock + currentTextBlock->size()`, not just `currentTextBlock->size()`. This ensures footnotes attach to the page containing their actual anchor word, even after layoutAndExtractLines(false) has extracted and removed earlier words from the block.
Applied to files:
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cppsrc/activities/reader/EpubReaderActivity.hlib/Epub/Epub/Page.cppsrc/activities/reader/EpubReaderActivity.cpp
📚 Learning: 2026-02-16T20:43:19.339Z
Learnt from: Levrk
Repo: crosspoint-reader/crosspoint-reader PR: 909
File: src/activities/reader/EpubReaderActivity.cpp:461-461
Timestamp: 2026-02-16T20:43:19.339Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, when using ConfirmationActivity with enterNewActivity(), it's not necessary to call exitActivity() beforehand. The ConfirmationActivity operates as a modal dialog and cleanup is handled via the pendingSubactivityExit flag in the callback lambda. This differs from other activities like EpubReaderChapterSelectionActivity or KOReaderSyncActivity which do require exitActivity() before enterNewActivity().
Applied to files:
src/activities/reader/EpubReaderActivity.hsrc/activities/reader/EpubReaderActivity.cpp
📚 Learning: 2026-02-18T15:43:07.515Z
Learnt from: GavinHigham
Repo: crosspoint-reader/crosspoint-reader PR: 966
File: src/activities/ActivityWithSubactivity.h:20-21
Timestamp: 2026-02-18T15:43:07.515Z
Learning: In the crosspoint-reader activity architecture, when using ActivityWithSubactivity, the currentActivity global variable remains at the top-level activity (e.g., ReaderActivity) even when nested subactivities are active. The supportsOrientation() delegation chain starts from the top-level activity and stops when it reaches a subactivity that overrides the method (e.g., EpubReaderActivity returns true). Deeper nested subactivities (like EpubReaderChapterSelectionActivity or EpubReaderPercentSelectionActivity) do not need to override supportsOrientation() because they never become the currentActivity and the delegation chain doesn't reach them.
Applied to files:
src/activities/reader/EpubReaderActivity.hsrc/activities/reader/EpubReaderActivity.cpp
📚 Learning: 2026-02-19T17:46:26.871Z
Learnt from: Tritlo
Repo: crosspoint-reader/crosspoint-reader PR: 1003
File: src/activities/reader/EpubReaderActivity.cpp:657-674
Timestamp: 2026-02-19T17:46:26.871Z
Learning: In EpubReaderActivity.cpp, renderContents() behavior is intentional: if uncached images exist, Phase 1 should call displayBuffer(forceFullRefresh) to perform a HALF_REFRESH and clear ghosting; Phase 2 should call renderer.displayBuffer() without forceFullRefresh for a faster refresh. Grayscale anti-aliasing is applied separately after renderContents() via displayGrayBuffer(). This guidance applies specifically to this file and situation.
Applied to files:
src/activities/reader/EpubReaderActivity.cpp
📚 Learning: 2026-02-12T06:57:35.955Z
Learnt from: CaptainFrito
Repo: crosspoint-reader/crosspoint-reader PR: 725
File: src/activities/network/CalibreConnectActivity.cpp:218-264
Timestamp: 2026-02-12T06:57:35.955Z
Learning: In src/activities/network/CalibreConnectActivity.cpp, button hints (« Exit) are intentionally omitted from the SERVER_STARTING and ERROR states—only the SERVER_RUNNING state displays navigation hints. This is a deliberate design decision by the author.
Applied to files:
src/activities/reader/EpubReaderActivity.cpp
🧬 Code graph analysis (3)
src/activities/reader/EpubReaderActivity.h (1)
src/activities/reader/EpubReaderActivity.cpp (4)
navigateToHref(816-853)navigateToHref(816-816)restoreSavedPosition(855-868)restoreSavedPosition(855-855)
lib/Epub/Epub/Page.cpp (2)
lib/Serialization/Serialization.h (1)
writePod(8-10)src/CrossPointSettings.cpp (2)
file(94-100)file(94-94)
src/activities/reader/EpubReaderActivity.cpp (3)
src/main.cpp (4)
exitActivity(136-142)exitActivity(136-136)enterNewActivity(144-147)enterNewActivity(144-144)src/activities/ActivityWithSubactivity.cpp (6)
exitActivity(20-27)exitActivity(20-20)enterNewActivity(29-34)enterNewActivity(29-29)requestUpdate(42-47)requestUpdate(42-42)src/activities/Activity.cpp (2)
requestUpdate(42-48)requestUpdate(42-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (18)
src/activities/reader/EpubReaderActivity.h (2)
28-36: LGTM!Well-designed footnote navigation state with bounded memory usage. The fixed-size array with
MAX_FOOTNOTE_DEPTH = 3is appropriate for embedded constraints and prevents unbounded stack growth during nested footnote navigation.
48-50: LGTM!Clean method declarations with a sensible default for
savePosition.src/activities/reader/EpubReaderActivity.cpp (4)
190-198: LGTM!Clean integration of footnote position restoration into the back button flow. The guard
footnoteDepth > 0correctly distinguishes between normal back navigation and returning from footnote jumps.
635-638: LGTM!Properly captures footnotes from the temporary page object via copy assignment, ensuring availability for the footnotes UI.
816-853: LGTM!Solid implementation with proper error handling:
- Null guards on entry
- Bounded position stack with
MAX_FOOTNOTE_DEPTHcheck- Correct undo of push on resolution failure
RenderLockproperly protects state mutations- String copy ensures
hrefsurvives any subsequent memory changes
855-868: LGTM!Clean stack-pop implementation with proper boundary check and lock-protected state restoration.
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp (8)
52-58: LGTM!Clean helper to retrieve XML attribute values. Null checks are properly handled.
60-68: LGTM!The protocol exclusion list covers the common external link types. Internal EPUB links (relative paths, fragment-only
#anchor) will correctly returntrue.
325-367: LGTM!Internal link detection and footnote tracking setup is well-structured. The buffer flush before style change ensures correct styling, and the underline styling via both depth tracking and style stack is consistent with other inline elements.
516-527: LGTM!The label normalization correctly strips brackets and whitespace. Buffer overflow is prevented with bounds checking, and null-termination is maintained after each character addition.
649-662: LGTM!The wordIndex calculation correctly uses the cumulative approach (
wordsExtractedInBlock + currentTextBlock->size()), ensuring footnotes attach to the correct page even after mid-paragraph flushes. Based on learnings: "the wordIndex must be cumulative across the 750-word mid-paragraph flush boundary."
138-138: LGTM!Correctly resets the cumulative word counter when starting a new text block/paragraph.
802-812: LGTM!The footnote-to-page assignment correctly uses cumulative word tracking. The iterator-based erase pattern handles invalidation properly.
851-859: The codebase explicitly enables C++20 support (-std=gnu++2ain platformio.ini, line 31) and removes the default C++11 flag. The structured binding syntax at line 855 is fully compatible with C++20 and requires no changes.lib/I18n/I18nKeys.h (1)
354-356: LGTM!New i18n keys for footnotes feature are correctly positioned before the
_COUNTsentinel. Based on learnings, missing translations in non-English YAML files will be auto-filled with English strings at build time bygen_i18n.py.lib/Epub/Epub/Page.cpp (2)
70-80: LGTM!Serialization correctly clamps footnote count to 16 and uses indexed iteration to match. This prevents serialization/deserialization mismatch if
footnotesis modified directly.
107-124: LGTM!Deserialization validates the footnote count before reading and defensively null-terminates strings. The count validation ensures compatibility with the serialization limit.
lib/I18n/translations/english.yaml (1)
320-322: LGTM!Clear and appropriate translation strings for the footnotes feature. The order matches the enum in
I18nKeys.h. Based on learnings, missing translations in other language files will be auto-filled with these English strings at build time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/activities/reader/EpubReaderActivity.cpp`:
- Around line 386-402: In the EpubReaderMenuActivity::MenuAction::FOOTNOTES case
remove the redundant requestUpdate() inside the footnote navigation lambda
because navigateToHref(href, true) already calls requestUpdate(); locate the
lambda passed to the EpubReaderFootnotesActivity constructor (the [this](const
char* href) callback), delete the trailing requestUpdate() call while keeping
the call to navigateToHref(href, true) and the exitActivity() invocation to
preserve flow.
|
Added a nested positions tracking (Array of 3 |
Uri-Tauber
force-pushed
the
feat/slim-footnotes-support
branch
from
6a96afd to
e3fd6b4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/activities/reader/EpubReaderFootnotesActivity.cpp (1)
12-16: Consider resettingscrollOffsetinonEnter.
selectedIndexis reset to 0, butscrollOffsetis not explicitly reset. If the activity is re-entered (e.g., user opens footnotes, navigates, returns, opens footnotes again), a stalescrollOffsetcould cause the list to render from an unexpected position.Proposed fix
void EpubReaderFootnotesActivity::onEnter() { ActivityWithSubactivity::onEnter(); selectedIndex = 0; + scrollOffset = 0; requestUpdate(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/reader/EpubReaderFootnotesActivity.cpp` around lines 12 - 16, EpubReaderFootnotesActivity::onEnter resets selectedIndex but not scrollOffset, so re-entering can leave a stale scroll position; update onEnter (alongside ActivityWithSubactivity::onEnter) to explicitly reset scrollOffset (e.g., set scrollOffset = 0 or its default) before calling requestUpdate so the footnotes list always starts at the top when the activity is entered.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/I18n/I18nKeys.hlib/I18n/translations/english.yamlsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderActivity.hsrc/activities/reader/EpubReaderFootnotesActivity.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/I18n/translations/english.yaml
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 988
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:649-661
Timestamp: 2026-02-19T12:17:00.962Z
Learning: In ChapterHtmlSlimParser.cpp, when computing footnote word indices in endElement() for footnote links, the wordIndex must be cumulative across the 750-word mid-paragraph flush boundary. The correct calculation is: `wordIndex = wordsExtractedInBlock + currentTextBlock->size()`, not just `currentTextBlock->size()`. This ensures footnotes attach to the page containing their actual anchor word, even after layoutAndExtractLines(false) has extracted and removed earlier words from the block.
📚 Learning: 2026-02-16T20:43:19.339Z
Learnt from: Levrk
Repo: crosspoint-reader/crosspoint-reader PR: 909
File: src/activities/reader/EpubReaderActivity.cpp:461-461
Timestamp: 2026-02-16T20:43:19.339Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, when using ConfirmationActivity with enterNewActivity(), it's not necessary to call exitActivity() beforehand. The ConfirmationActivity operates as a modal dialog and cleanup is handled via the pendingSubactivityExit flag in the callback lambda. This differs from other activities like EpubReaderChapterSelectionActivity or KOReaderSyncActivity which do require exitActivity() before enterNewActivity().
Applied to files:
src/activities/reader/EpubReaderActivity.hsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderFootnotesActivity.cpp
📚 Learning: 2026-02-18T15:43:07.515Z
Learnt from: GavinHigham
Repo: crosspoint-reader/crosspoint-reader PR: 966
File: src/activities/ActivityWithSubactivity.h:20-21
Timestamp: 2026-02-18T15:43:07.515Z
Learning: In the crosspoint-reader activity architecture, when using ActivityWithSubactivity, the currentActivity global variable remains at the top-level activity (e.g., ReaderActivity) even when nested subactivities are active. The supportsOrientation() delegation chain starts from the top-level activity and stops when it reaches a subactivity that overrides the method (e.g., EpubReaderActivity returns true). Deeper nested subactivities (like EpubReaderChapterSelectionActivity or EpubReaderPercentSelectionActivity) do not need to override supportsOrientation() because they never become the currentActivity and the delegation chain doesn't reach them.
Applied to files:
src/activities/reader/EpubReaderActivity.hsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderFootnotesActivity.cpp
📚 Learning: 2026-02-19T12:17:00.962Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 988
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:649-661
Timestamp: 2026-02-19T12:17:00.962Z
Learning: In ChapterHtmlSlimParser.cpp, when computing footnote word indices in endElement() for footnote links, the wordIndex must be cumulative across the 750-word mid-paragraph flush boundary. The correct calculation is: `wordIndex = wordsExtractedInBlock + currentTextBlock->size()`, not just `currentTextBlock->size()`. This ensures footnotes attach to the page containing their actual anchor word, even after layoutAndExtractLines(false) has extracted and removed earlier words from the block.
Applied to files:
src/activities/reader/EpubReaderActivity.hsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderFootnotesActivity.cpp
📚 Learning: 2026-02-16T22:25:35.674Z
Learnt from: whyte-j
Repo: crosspoint-reader/crosspoint-reader PR: 733
File: lib/I18n/I18nKeys.h:271-272
Timestamp: 2026-02-16T22:25:35.674Z
Learning: In the crosspoint-reader i18n system (lib/I18n/), missing translation keys in non-English YAML files are automatically filled with English strings at build time by the gen_i18n.py script. All generated string arrays (STRINGS_EN, STRINGS_ES, etc.) have identical lengths, so runtime array indexing is safe without per-string fallback logic. The system prints "INFO: '{key}' missing in {lang_code}, using English fallback" during code generation when this occurs.
Applied to files:
lib/I18n/I18nKeys.h
📚 Learning: 2026-02-19T17:46:26.871Z
Learnt from: Tritlo
Repo: crosspoint-reader/crosspoint-reader PR: 1003
File: src/activities/reader/EpubReaderActivity.cpp:657-674
Timestamp: 2026-02-19T17:46:26.871Z
Learning: In EpubReaderActivity.cpp, renderContents() behavior is intentional: if uncached images exist, Phase 1 should call displayBuffer(forceFullRefresh) to perform a HALF_REFRESH and clear ghosting; Phase 2 should call renderer.displayBuffer() without forceFullRefresh for a faster refresh. Grayscale anti-aliasing is applied separately after renderContents() via displayGrayBuffer(). This guidance applies specifically to this file and situation.
Applied to files:
src/activities/reader/EpubReaderActivity.cpp
📚 Learning: 2026-02-12T06:57:35.955Z
Learnt from: CaptainFrito
Repo: crosspoint-reader/crosspoint-reader PR: 725
File: src/activities/network/CalibreConnectActivity.cpp:218-264
Timestamp: 2026-02-12T06:57:35.955Z
Learning: In src/activities/network/CalibreConnectActivity.cpp, button hints (« Exit) are intentionally omitted from the SERVER_STARTING and ERROR states—only the SERVER_RUNNING state displays navigation hints. This is a deliberate design decision by the author.
Applied to files:
src/activities/reader/EpubReaderActivity.cppsrc/activities/reader/EpubReaderFootnotesActivity.cpp
📚 Learning: 2026-02-19T17:46:26.871Z
Learnt from: Tritlo
Repo: crosspoint-reader/crosspoint-reader PR: 1003
File: src/activities/reader/EpubReaderActivity.cpp:657-674
Timestamp: 2026-02-19T17:46:26.871Z
Learning: In src/activities/reader/EpubReaderActivity.cpp's renderContents() method, when uncached images exist, Phase 1 intentionally calls displayBuffer(forceFullRefresh) to perform a HALF_REFRESH (if needed), while Phase 2 intentionally calls renderer.displayBuffer() directly without forceFullRefresh. This is by design: Phase 1's refresh clears the screen properly to prevent ghosting, so Phase 2 can use a faster refresh mode for better performance. The grayscale anti-aliasing is handled separately after renderContents() via displayGrayBuffer().
Applied to files:
src/activities/reader/EpubReaderFootnotesActivity.cpp
🧬 Code graph analysis (2)
src/activities/reader/EpubReaderActivity.h (1)
src/activities/reader/EpubReaderActivity.cpp (4)
navigateToHref(816-853)navigateToHref(816-816)restoreSavedPosition(855-868)restoreSavedPosition(855-855)
src/activities/reader/EpubReaderActivity.cpp (3)
src/main.cpp (4)
exitActivity(136-142)exitActivity(136-136)enterNewActivity(144-147)enterNewActivity(144-144)src/activities/ActivityWithSubactivity.cpp (6)
exitActivity(20-27)exitActivity(20-20)enterNewActivity(29-34)enterNewActivity(29-29)requestUpdate(42-47)requestUpdate(42-42)src/activities/Activity.cpp (2)
requestUpdate(42-48)requestUpdate(42-42)
🔇 Additional comments (8)
lib/I18n/I18nKeys.h (1)
353-356: LGTM — new StrId entries look consistent with the existing enum pattern.src/activities/reader/EpubReaderActivity.h (1)
28-50: LGTM! Clean footnote navigation scaffolding.The state management design is sound:
- Fixed-size
savedPositions[3]avoids heap allocation on the hot path per PR objectivesSavedPositionPOD struct is appropriately minimal- Default parameter
savePosition = falsefornavigateToHrefallows flexible usagesrc/activities/reader/EpubReaderFootnotesActivity.cpp (1)
53-95: LGTM! Render logic is solid.
- Visible count calculation safely handles edge cases with
std::max(1, ...)- Scroll offset adjustment keeps selection in view
- Localization properly uses
tr()with string IDssrc/activities/reader/EpubReaderActivity.cpp (5)
190-198: LGTM! Back button logic correctly prioritizes footnote restoration.The conditional check for
footnoteDepth > 0beforeonGoHome()ensures users can navigate back through footnote history before exiting the reader.
386-402: VerifycurrentPageFootnotesreference lifetime.
EpubReaderFootnotesActivityreceivescurrentPageFootnotesby const reference. SincecurrentPageFootnotesis a member ofEpubReaderActivityand the parent activity remains alive while the subactivity runs, this should be safe. However, ifrender()were called and updatedcurrentPageFootnoteswhile the footnotes activity is open, it could cause inconsistency.Given that the subactivity blocks render updates (per
ActivityWithSubactivity::requestUpdate()behavior), this appears safe.
635-638: LGTM! Footnote collection is straightforward.Copying footnotes from the loaded page ensures
currentPageFootnotesalways reflects the currently displayed page.
816-853: LGTM! Navigation logic handles edge cases well.Key observations:
- Null checks for
epubandhrefprevent crashes- Position only saved when
sectionexists (valid state to restore)- Failed resolution correctly undoes the stack push
RenderLockproperly protects state mutations
855-868: LGTM! Position restoration is correctly implemented.The decrement-before-access pattern (
footnoteDepth--thensavedPositions[footnoteDepth]) correctly maps depth to array index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/activities/reader/EpubReaderFootnotesActivity.cpp`:
- Around line 12-16: EpubReaderFootnotesActivity::onEnter resets selectedIndex
but not scrollOffset, so re-entering can leave a stale scroll position; update
onEnter (alongside ActivityWithSubactivity::onEnter) to explicitly reset
scrollOffset (e.g., set scrollOffset = 0 or its default) before calling
requestUpdate so the footnotes list always starts at the top when the activity
is entered.
|
@Uri-Tauber will you bring back the PR? |
|
I'm sorry. I accidently deleted my fork of crosspoint. |
Summary
What is the goal of this PR? Implement support for footnotes in epub files.
It is based on #553, but simplified — removed the parts which complicated the code and burden the CPU/RAM. This version supports basic footnotes and lets the user jump from location to location inside the epub.
What changes are included?
-
FootnoteEntrystruct — A small POD struct (number[24], href[64]) shared between parser, page storage, and UI.- Parser:
<a href>detection (ChapterHtmlSlimParser) — During a single parsing pass, internal epub links are detected and collected as footnotes. The link text is underlined to hint navigability. Bracket/whitespace normalization is applied to the display label (e.g. [1] → 1).- Footnote-to-page assignment (
ChapterHtmlSlimParser,Page) — Footnotes are attached to the exact page where their anchor word appears, tracked via a cumulative word counter during layout, surviving paragraph splits and the 750-word mid-paragraph safety flush.- Page serialization (
Page,Section) — Footnotes are serialized/deserialized per page (max 16 per page). Section cache version bumped to 14 to force a clean rebuild.- Href → spine resolution (
Epub) —resolveHrefToSpineIndex()maps an href (e.g.chapter2.xhtml#note1) to its spine index by filename matching.- Footnotes menu + activity (
EpubReaderMenuActivity,EpubReaderFootnotesActivity) — A new "Footnotes" entry in the reader menu lists all footnote links found on the current page. The user scrolls and selects to navigate.- Navigate & restore (
EpubReaderActivity) —navigateToHref()saves the current spine index and page number, then jumps to the target. The Back button restores the saved position when the user is done reading the footnote.Additional Context
What was removed vs #553: virtual spine items (
addVirtualSpineItem,isVirtualSpineItem), two-pass parsing,<aside>content extraction to temp HTML files,<p class="note">paragraph note extraction,replaceHtmlEntities(master already haslookupHtmlEntity),footnotePages/buildFilteredChapterList,noterefCallback/Noterefstruct, and the stack size increase from 8 KB to 24 KB (not needed without two-pass parsing and virtual file I/O on the render task).Performance: Single-pass parsing. No new heap allocations in the hot path — footnote text is collected into fixed stack buffers (char[24], char[64]). Active runtime memory is ~2.8 KB worst-case (one page × 16 footnotes × 88 bytes, mirrored in
currentPageFootnotes). Flash usage is unchanged at 97.4%; RAM stays at 31%.Known limitations: When clicking a footnote, it jumps to the start of the HTML file instead of the specific anchor. This could be problematic for books that don't have separate files for each footnote. (no element-id-to-page mapping yet - will be another PR soon).
AI Usage
Did you use AI tools to help write this code? < PARTIALLY>
Claude Opus 4.6 was used to do most of the migration, I checked manually its work, and fixed some stuff, but I haven't review all the changes yet, so feedback is welcomed.