Skip to content

feat(epub): basic table support for row/colspan and for solid/none border style#1889

Draft
WuTofu wants to merge 14 commits into
crosspoint-reader:masterfrom
WuTofu:feat/basic-epub-table
Draft

feat(epub): basic table support for row/colspan and for solid/none border style#1889
WuTofu wants to merge 14 commits into
crosspoint-reader:masterfrom
WuTofu:feat/basic-epub-table

Conversation

@WuTofu
Copy link
Copy Markdown
Contributor

@WuTofu WuTofu commented May 9, 2026

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/none border styles.

  • What changes are included?

    • Added TableBlock and PageTable support for table rendering and page serialization.
    • Extended ChapterHtmlSlimParser to parse <table>, <tr>, <td>, and <th> with cell padding, width, colspan, rowspan, and border style accumulation.
    • Added CSS parsing for border-style, border-width, and shorthand border/border-top/border-bottom/border-left/border-right.
    • Added CssBorderStyle support and visibility checks in CssStyle.
    • Added table serialization/deserialization and page element tagging in Page.cpp / Page.h.

Additional Context

  1. Basic two-column table
image
Screenshot 1 Screenshot 2
image image
  1. Basic three-column table
Table Screenshot
image image
  1. Center-aligned table with no border
image
Screenshot 1 Screenshot 2
image image
  1. Table with no border
Table Screenshot
image image
  1. Table with border-{top,bottom} style
Table Screenshot
image image
  1. Table with border-{top,bottom} style and center-aligned text
image
Screenshot 1 Screenshot 2
image image
  1. Table with row/colspan
Table Screenshot
image image
  1. Table with border shorthands style
image
Screenshot 1 Screenshot 2
image image
Screenshot 3 Screenshot 4
image image
  1. Table with border shorthands style (postive pixels + none)
image
Screenshot 1 Screenshot 2
image image
Screenshot 3 Screenshot 4
image image
  1. Table with border shorthands style (none + postive pixels)
image
Screenshot 1 Screenshot 2
image image
Screenshot 3 Screenshot 4
image image
  1. Table with special case of rowspan="0"

Note: colspan must be a positive number, only rowspan can be zero or a positive number.

image
Screenshot 1 Screenshot 2 Screenshot 3
image image image

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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 9, 2026

Review Change Stack

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7a98f5de-ceb8-4fcd-942e-c5ae428544b2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This 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.

Changes

Table Support Implementation

Layer / File(s) Summary
Block Types and Table Data Structures
lib/Epub/Epub/blocks/Block.h, lib/Epub/Epub/blocks/TableBlock.h
Adds TABLE_BLOCK enum, introduces TableCell with text lines and per-cell padding/span metadata, TableRow with cells and row layout metrics, and TableBlock class with column widths, borders, and methods for height/render/serialize/deserialize.
PageTable Page Element
lib/Epub/Epub/Page.h
Adds TAG_PageTable enum and PageTable class derived from PageElement, wrapping a TableBlock with precomputed height, includes blocks/TableBlock.h header.
TableBlock Rendering and Persistence
lib/Epub/Epub/blocks/TableBlock.cpp
Implements totalHeight() computing dimensions from borders/padding/row content, render() drawing borders/dividers with phantom-cell rowspan logic, serialize() writing table metadata and block content, and deserialize() reconstructing with sanity checks and span normalization.
PageTable Integration
lib/Epub/Epub/Page.cpp
Implements PageTable::render, serialize, deserialize delegating to TableBlock, updates Page::deserialize to recognize TAG_PageTable and handle deserialization failures.
CSS Border Style Model
lib/Epub/Epub/css/CssStyle.h
Adds CssBorderStyle enum, extends CssPropertyFlags to uint32_t with per-side style/width-zero/width-set flags, adds per-side borderTopStyle/bottomStyle/leftStyle/rightStyle members to CssStyle.
CSS Border Querying
lib/Epub/Epub/css/CssStyle.h
Updates applyOver() to propagate border definitions and width flags, adds per-side hasBorder*Style() and isBorder*Visible() methods and aggregate hasBorderStyle(), updates reset() to initialize border styles.
CSS Border Parsing
lib/Epub/Epub/css/CssParser.h, lib/Epub/Epub/css/CssParser.cpp
Bumps CSS_CACHE_VERSION to 5, adds isBorderStyleKeyword() and interpretBorderStyle() helpers, extends parseDeclarationIntoStyle() for per-side and shorthand border properties with width zero/set tracking, updates cache format to serialize four border-style bytes and 32-bit definedBits.
HTML Parser Table State
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.h, lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp
Adds TableBlock.h include, defines TableCellAccum and TableRowAccum accumulation types, replaces simple indices with multi-row table state, per-column width tracking, rowspan progress, and border flags; adds parseHtmlWidthAttr() helper.
HTML Table Parsing and Layout
lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp
Implements <table> flush/init, <tr> row creation, <td>/<th> cell accumulation with width/span/padding parsing, </td>/</th> text move and cell-mode exit, </tr> row layout with phantom-cell interleaving and column-width resolution, </table> rowspan distribution and multi-page PageTable emission with line-by-line splitting; disables long-text optimization in table-cell mode.

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

  • jpirnay
  • znelson
  • Uri-Tauber
  • jdk2pq
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main feature: basic table support for row/colspan and border style handling (solid/none).
Description check ✅ Passed The description comprehensively documents the PR goals, changes made, and includes multiple test case screenshots demonstrating the implementation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between e64155e and 6654530.

📒 Files selected for processing (11)
  • lib/Epub/Epub/Page.cpp
  • lib/Epub/Epub/Page.h
  • lib/Epub/Epub/Section.cpp
  • lib/Epub/Epub/blocks/Block.h
  • lib/Epub/Epub/blocks/TableBlock.cpp
  • lib/Epub/Epub/blocks/TableBlock.h
  • lib/Epub/Epub/css/CssParser.cpp
  • lib/Epub/Epub/css/CssParser.h
  • lib/Epub/Epub/css/CssStyle.h
  • lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp
  • lib/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.h
  • lib/Epub/Epub/css/CssParser.h
  • lib/Epub/Epub/Page.h
  • lib/Epub/Epub/Section.cpp
  • lib/Epub/Epub/Page.cpp
  • lib/Epub/Epub/blocks/TableBlock.h
  • lib/Epub/Epub/blocks/TableBlock.cpp
  • lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp
  • lib/Epub/Epub/css/CssParser.cpp
  • lib/Epub/Epub/css/CssStyle.h
  • lib/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.cpp
  • lib/Epub/Epub/Page.cpp
  • lib/Epub/Epub/blocks/TableBlock.cpp
  • lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp
  • lib/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.cpp
  • lib/Epub/Epub/Page.cpp
  • lib/Epub/Epub/blocks/TableBlock.cpp
  • lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp
  • lib/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.cpp
  • lib/Epub/Epub/Page.cpp
  • lib/Epub/Epub/blocks/TableBlock.cpp
  • lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp
  • lib/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.

Comment thread lib/Epub/Epub/blocks/TableBlock.cpp
Comment thread lib/Epub/Epub/blocks/TableBlock.cpp
Comment thread lib/Epub/Epub/css/CssStyle.h
Comment thread lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp
@WuTofu WuTofu force-pushed the feat/basic-epub-table branch from bac852f to d2ceec0 Compare May 9, 2026 19:01
@pablohc
Copy link
Copy Markdown
Contributor

pablohc commented May 10, 2026

wow, amazing job @WuTofu!! I will test it and let you know! ;)

@pablohc pablohc requested review from a team and pablohc May 10, 2026 18:49
@Uri-Tauber
Copy link
Copy Markdown
Member

It should probably wait until after release 1.3.

@WuTofu
Copy link
Copy Markdown
Contributor Author

WuTofu commented May 10, 2026

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

@WuTofu
Copy link
Copy Markdown
Contributor Author

WuTofu commented May 10, 2026

wow, amazing job @WuTofu!! I will test it and let you know! ;)

@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

@pablohc
Copy link
Copy Markdown
Contributor

pablohc commented May 10, 2026

wow, amazing job @WuTofu!! I will test it and let you know! ;)

@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.

@pablohc pablohc removed the request for review from a team May 10, 2026 19:46
@WuTofu WuTofu marked this pull request as draft May 11, 2026 03:54
@WuTofu WuTofu force-pushed the feat/basic-epub-table branch from d2ceec0 to 279241d Compare May 11, 2026 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants