Skip to content

Conversation

Louciole
Copy link
Member

@Louciole Louciole commented Sep 3, 2025

No description provided.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for running positions, content element() function and counter(page) function to the CSS styling engine. These features enable advanced page layout capabilities for print media, allowing elements to be positioned in page margins and content to be dynamically inserted based on page context.

  • Adds RunningPosition struct and updates Position type to support running() CSS function
  • Implements Content union type with support for element() and counter() functions
  • Adds RunningPositionMap for tracking elements across pages during pagination

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/vaev-engine/values/primitives.cpp Adds comparison operators and repr method to CustomIdent
src/vaev-engine/values/mod.cpp Exports new content values module
src/vaev-engine/values/insets.cpp Refactors Position from enum to Union with RunningPosition support
src/vaev-engine/values/defs/keywords.inc Adds position-related keywords (Fixed, Sticky, etc.)
src/vaev-engine/values/content.cpp New module implementing Content, ElementContent, and Counter types
src/vaev-engine/style/specified.cpp Updates content field from String to Content type
src/vaev-engine/style/props.cpp Updates ContentProp to use Content type instead of String
src/vaev-engine/style/props-impl.cpp Minor formatting fix for function call
src/vaev-engine/layout/positioned.cpp Updates position comparisons to use Keywords
src/vaev-engine/layout/paint.cpp Updates position comparisons and adds RunningPosition handling
src/vaev-engine/layout/layout-impl.cpp Adds early return for RunningPosition elements
src/vaev-engine/layout/inline.cpp Adds running position tracking during inline layout
src/vaev-engine/layout/flex.cpp Adds running position tracking during flex layout
src/vaev-engine/layout/builder.cpp Updates buildForPseudoElement to handle new Content types
src/vaev-engine/layout/block.cpp Adds running position tracking during block layout
src/vaev-engine/layout/base.cpp Implements RunningPositionMap and related structures
src/vaev-engine/driver/print.cpp Major refactor to support two-pass pagination with running position collection

@Louciole Louciole marked this pull request as ready for review September 3, 2025 09:45
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.

}

Slice<RunningPositionInfo> _searchPage(Slice<RunningPositionInfo> list, usize page) {
page++; // pages are 1-indexed
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

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

Magic number increment without clear context. The comment indicates pages are 1-indexed, but the function parameter name and usage suggest it's 0-indexed. Consider renaming the parameter to pageIndexZeroBased or adjusting the logic to accept 1-indexed pages directly for clarity.

Copilot generated this review using guidance from repository custom instructions.

Type type = Type::PAGE;

void repr(Io::Emit& e) const {
e("counter (type:'{}')", type);
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

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

Corrected spacing in debug output format string from 'counter (type' to 'counter(type'.

Suggested change
e("counter (type:'{}')", type);
e("counter(type:'{}')", type);

Copilot uses AI. Check for mistakes.


Box element = buildElement(infos.unwrap().structure);
element.style->position = Keywords::STATIC;
marginBox.add(std::move(element));
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

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

Variable name element shadows the parameter name used earlier in the function. Consider renaming to elementBox for clarity.

Copilot generated this review using guidance from repository custom instructions.

});

if (not res) {
return sub(list, 0, 1);
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

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

Magic numbers 0 and 1 are unclear in this context. Consider using a named constant or adding a comment explaining why the first element is returned when binary search fails.

Copilot generated this review using guidance from repository custom instructions.

@sleepy-monax sleepy-monax merged commit 3b446b1 into main Oct 16, 2025
2 checks passed
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.

2 participants