feat(epub): basic table support for row/colspan and for solid/none border style#1889
feat(epub): basic table support for row/colspan and for solid/none border style#1889WuTofu wants to merge 14 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds complete table support to the EPUB reader: TableBlock data structures with rendering, binary serialization, and rowspan/colspan logic; PageTable page element integration; CSS per-side border model with parsing and cache format bump (v4→v5); and HTML table parsing with deferred layout, multi-page emission, and cell-spanning semantics. ChangesTable Support Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR introduces a substantial, multi-layered feature: table data structures (3 new types), rendering with complex rowspan/colspan/phantom-cell logic (238 lines in TableBlock.cpp), CSS border parsing and cache format changes affecting 12 new flag fields and two size constants (275 lines across CssParser/CssStyle), and HTML table parsing with deferred layout and multi-page emission (553 lines, including rowspan distribution and line-by-line splitting). Changes span 12 files across multiple architectural layers (data model, CSS, parsing, page layout) with dense logic in table rendering and HTML parsing, requiring careful verification of border logic, span handling, and page-splitting semantics. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@lib/Epub/Epub/blocks/TableBlock.cpp`:
- Around line 215-217: The code reads an untrusted uint16_t numLines via
serialization::readPod and immediately calls cell.lines.reserve(numLines) and
later deserializes that many entries; add a sanity check that validates numLines
against a reasonable maximum (e.g., a constant MAX_CELL_LINES or derived from
file/record size) and reject/log/throw if it exceeds that cap before calling
cell.lines.reserve or entering the deserialize loop; update the logic around
numLines, serialization::readPod, and cell.lines.reserve in TableBlock.cpp (and
any loop that consumes numLines) to enforce this bound.
- Around line 103-110: In TableBlock.cpp inside the loop that scans the next row
(the block setting phantomAt in the code surrounding rows[ri + 1].cells), the
index variable lc is advanced by 1 for every next-row cell which misaligns when
cells have colspan > 1; change the advancement so lc is incremented by the
cell's logical width (use nc.colspan or treat colspan of 0/1 as 1) after
handling nc.rowspan == 0, and ensure you still guard phantomAt with the existing
64-limit check; update the loop that references lc so phantomAt marks the
correct logical columns and row separators are placed correctly.
In `@lib/Epub/Epub/css/CssStyle.h`:
- Around line 81-90: The current CssStyle bitfields only track
borderTopWidthZero/borderBottomWidthZero/borderLeftWidthZero/borderRightWidthZero
which cannot distinguish "width was explicitly set to 0" from "width was
unspecified"; add per-side "width defined" bits (e.g. borderTopWidthDefined,
borderBottomWidthDefined, borderLeftWidthDefined, borderRightWidthDefined)
alongside the existing border*WidthZero flags in class/struct CssStyle, set them
wherever widths are parsed/cached (the parser that currently sets
border*WidthZero), and update the style merge logic (the method that
merges/composes styles) to prefer a higher-priority rule that has widthDefined
(even if non-zero) over a lower-priority widthZero, and to clear/restore
visibility appropriately when a higher-priority rule defines a non-zero width or
border style; ensure these flags are serialized through any cache/save/load
paths you have so cascade semantics are preserved.
In `@lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp`:
- Around line 1143-1148: The parser currently allows numCols up to 255 which can
overflow TableBlock's 64-column buffers; clamp the parsed column count to the
64-column invariant instead of 255 by capping logicalColCount at 64 when
computing numCols and then update self->tableTotalCols with that clamped value.
Locate the computation around logicalColCount/numCols in
ChapterHtmlSlimParser::parse (the code that sets numCols and
self->tableTotalCols) and replace the 255 cap with the TableBlock column limit
(use the literal 64 or the TableBlock max constant if available) so the runtime
path and serialized entries never exceed the 64-column limit referenced by
TableBlock::render() and TableBlock::deserialize().
🪄 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: CHILL
Plan: Pro
Run ID: 8e38e0e9-68ef-4cf3-ad4f-dbbbbadfe519
📒 Files selected for processing (11)
lib/Epub/Epub/Page.cpplib/Epub/Epub/Page.hlib/Epub/Epub/Section.cpplib/Epub/Epub/blocks/Block.hlib/Epub/Epub/blocks/TableBlock.cpplib/Epub/Epub/blocks/TableBlock.hlib/Epub/Epub/css/CssParser.cpplib/Epub/Epub/css/CssParser.hlib/Epub/Epub/css/CssStyle.hlib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpplib/Epub/Epub/parsers/ChapterHtmlSlimParser.h
📜 Review details
⏰ 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). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-04-12T12:28:33.205Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1629
File: src/activities/home/HomeActivity.cpp:119-120
Timestamp: 2026-04-12T12:28:33.205Z
Learning: When reviewing code in this repository (C/C++ sources), set review comment severity according to this policy:
- Use **Major** (🟠) only for defects with realistic risk of crash, out-of-memory (OOM), invalid pointer dereference, data corruption, or other severe issues that are unlikely to be caught in casual/manual device testing.
- Use **Minor** or informational for UX gaps, logic edge-cases, style issues, or missing feature completeness that the author can verify (or has verified) through normal device use.
- Do **not** escalate severity to Major based on behavioral/UX observations alone; assume the author has already tested the feature on their own device and only treat issues as Major if they match the high-risk defect categories above.
Applied to files:
lib/Epub/Epub/blocks/Block.hlib/Epub/Epub/css/CssParser.hlib/Epub/Epub/Page.hlib/Epub/Epub/Section.cpplib/Epub/Epub/Page.cpplib/Epub/Epub/blocks/TableBlock.hlib/Epub/Epub/blocks/TableBlock.cpplib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpplib/Epub/Epub/css/CssParser.cpplib/Epub/Epub/css/CssStyle.hlib/Epub/Epub/parsers/ChapterHtmlSlimParser.h
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
lib/Epub/Epub/Section.cpplib/Epub/Epub/Page.cpplib/Epub/Epub/blocks/TableBlock.cpplib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpplib/Epub/Epub/css/CssParser.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
lib/Epub/Epub/Section.cpplib/Epub/Epub/Page.cpplib/Epub/Epub/blocks/TableBlock.cpplib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpplib/Epub/Epub/css/CssParser.cpp
📚 Learning: 2026-03-28T11:06:29.611Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:92-95
Timestamp: 2026-03-28T11:06:29.611Z
Learning: When reviewing crosspoint-reader code, avoid flagging a missing `renderer.displayBuffer()` call immediately after `GUI.drawPopup()` / `BaseTheme::drawPopup()`: `BaseTheme::drawPopup()` already calls `renderer.displayBuffer()` before returning, so the popup is guaranteed to be flushed to the e-ink panel before subsequent blocking work begins. Conversely, do not require a `renderer.displayBuffer()` call after `fillPopupProgress()`; it intentionally does not flush, so intermediate progress-bar updates may not appear unless the update granularity warrants an explicit flush elsewhere.
Applied to files:
lib/Epub/Epub/Section.cpplib/Epub/Epub/Page.cpplib/Epub/Epub/blocks/TableBlock.cpplib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpplib/Epub/Epub/css/CssParser.cpp
🔇 Additional comments (2)
lib/Epub/Epub/Section.cpp (1)
13-13: Good cache-version bump.This keeps older section caches from being deserialized against the new page/table payload shape.
lib/Epub/Epub/Page.cpp (1)
44-55: Nice fail-fast on corrupt table payloads.Bailing out when
TableBlock::deserialize()fails is the right boundary here; it avoids returning a partially reconstructed page.
bac852f to
d2ceec0
Compare
|
wow, amazing job @WuTofu!! I will test it and let you know! ;) |
|
It should probably wait until after release 1.3. |
|
Oh, wait. Just checked my all screenshots again, and saw my test case 5 and 8 didn't actually pass. I probably broke them when I was fixing other cases.🤦 Will take a look soon |
@pablohc since I just found not all my tests pissing, I actually want to turn this into draft now. And I don't want to waste your time and make you test it over and over. I personally don't expect and also not want to rush this into v1.3.0. So, I didn't ask anyone to review it And on second thought, I probably won't fix this soon(another reason to make this draft lol). I want to do something that I can polish the v1.3.0 release first. Like improving the experience of font downloader ui |
Totally understandable, @WuTofu. My intention wasn't to rush things, but to give you feedback on what's been done. When you feel confident about the PR status, let me know and I'll take another look. |
d2ceec0 to
279241d
Compare
Summary
What is the goal of this PR?
Implements basic EPUB table rendering support in the reader, including row/colspan handling and border rendering for
solid/noneborder styles.What changes are included?
TableBlockandPageTablesupport for table rendering and page serialization.ChapterHtmlSlimParserto parse<table>,<tr>,<td>, and<th>with cell padding, width, colspan, rowspan, and border style accumulation.border-style,border-width, and shorthandborder/border-top/border-bottom/border-left/border-right.CssBorderStylesupport and visibility checks inCssStyle.Page.h.Additional Context
Note: colspan must be a positive number, only rowspan can be zero or a positive number.
AI Usage
While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.
Did you use AI tools to help write this code? PARTIALLY