Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the pagination system in the Vaev driver. The main changes involve consolidating pagination logic into a new PaginationContext class and renaming identifiers for better clarity regarding page indexing.
- Renamed
pageNumbertopageIndexthroughout the codebase to reflect 0-based indexing - Changed
Font::Databasefrom a reference to a value member inComputerto support move semantics - Introduced
PaginationContextclass to encapsulate all pagination-related state and operations - Renamed
RunningPositionMaptoRunningsfor consistency
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/vaev-engine/style/computer.cpp | Changed fontBook to _database (value instead of reference) and updated all references |
| src/vaev-engine/layout/positioned.cpp | Updated to use pageIndex instead of pageNumber |
| src/vaev-engine/layout/inline.cpp | Updated to use pageIndex instead of pageNumber |
| src/vaev-engine/layout/flex.cpp | Updated to use pageIndex instead of pageNumber |
| src/vaev-engine/layout/builder/mod.cpp | Changed parameter type from RunningPositionMap to Runnings |
| src/vaev-engine/layout/block.cpp | Updated to use pageIndex instead of pageNumber |
| src/vaev-engine/layout/base/running-position.cpp | Renamed struct to Runnings, removed page increment logic, added documentation |
| src/vaev-engine/layout/base/input.cpp | Updated field names: pageNumber → pageIndex, RunningPositionMap → Runnings |
| src/vaev-engine/layout/base/breaks.cpp | Added static constants for document boundaries, changed MutCursor to Cursor for breakpoint traversal |
| src/vaev-engine/driver/render.cpp | Updated to use move semantics for Font::Database |
| src/vaev-engine/driver/print.cpp | Complete refactor with new PaginationContext class and Page struct |
| src/vaev-engine/dom/window.cpp | Updated to use PaginationContext for printing and changed print method to non-const |
|
|
||
| // FIXME: use attrs from style::FontFace | ||
| if (fontBook.load(resolvedUrl.unwrap())) | ||
| if (_database.load(resolvedUrl.unwrap())) |
There was a problem hiding this comment.
idk about "database" it's not clear that we're talking about fonts
| .containingBlock = {inlineSize, input.knownSize.y.unwrapOr(0_au)}, | ||
| .runningPosition = input.runningPosition, | ||
| .pageNumber = input.pageNumber, | ||
| .pageIndex = input.pageIndex, |
| } | ||
| }; | ||
|
|
||
| Breakpoint const Breakpoint::START_OF_DOCUMENT = { |
| .name = ""s, | ||
| .number = pageNumber++, | ||
| .blank = false, | ||
| export struct PaginationContext { |
There was a problem hiding this comment.
Why not, a bit java core but make sense
There was a problem hiding this comment.
Well you are the one who named it like this ;)
There was a problem hiding this comment.
I was talkin about the whole Object/it's architecture 😄
| Print::Settings _settings; | ||
| Style::Computer _computer; | ||
| Style::SpecifiedValues _style; | ||
| Layout::Runnings _runnings = {}; |
There was a problem hiding this comment.
maybe add a comment saying that we're talking about running pos, they're a shady corner of the specs so we cant be sure the one reading the code knows about it (or keep the old name)
| page.breakpoint = std::move(breakpoint); | ||
| _pages.pushBack(page); | ||
| if (done) | ||
| break; |
Louciole
left a comment
Louciole
left a comment
There was a problem hiding this comment.
the structure is clearer, really OO but eager to see the final version
No description provided.