-
Notifications
You must be signed in to change notification settings - Fork 31
paper-muncher: Remove subcommands and improve configuration of overflow behavior. #109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 simplifies Paper Muncher's CLI by removing the print
and render
subcommands, consolidating them into a single unified command. It introduces a new --overflow
parameter to control how content that exceeds page boundaries is handled, with modes including auto
, paginate
, crop
, and visible
.
Key changes:
- Removed separate
print
andrender
subcommands in favor of a streamlined single-command interface - Added new
Overflow
enum and--overflow
flag to control pagination behavior - Renamed
--unsecure
flag to--sandboxed
with inverted logic for better clarity - Consolidated
PrintOption
andRenderOption
into a unifiedOption
struct
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/mod.cpp | New module exporting the unified run function with Overflow enum and consolidated Option struct |
src/main.cpp | Removed subcommands, consolidated CLI arguments, and integrated new overflow parameter |
readme.md | Updated basic usage examples to reflect simplified CLI |
meta/plugins/reftest.py | Updated test command to use --overflow paginate instead of print subcommand |
doc/usage.md | Removed separate command sections and consolidated documentation |
doc/install.md | Updated installation example to use simplified command |
6a36fd6
to
98bc630
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First : i agree with the technical refactoring, it's great and needed.
but I don't really know if --overflow is the best option, while it's descriptive it implies that there is a possible overflow
Wich is false. if the box grow it's not overflowing, if we paginate neither. and this behavior is only possible in the Y axis.
It's not too bad but I prefer : --pagination (auto|true|false) (maybe the crop should be at users discretion in their CSS and not a PM option)
@Louciole it’s called
edit: |
I understand that but you didn't address my concerns
Pagination false should be visible, crop is a weird use case that only make sense for images in my opinion
Mhhhh since we allow sizing in CSS having both options is REALLY odd, if the document is set to A4 and then we try to convert it to demi-raisin because it's what the users put in their CLI it should stay A4 in my opinion (more flexible and compliant with CSS's spec), documents knows their sizes (except for non paginated documents where we should listen for the CLI) |
This flag defines the policy for what happens when overflow occurs.
The Y-axis part isn’t accurate. Overflow can occur on both X and Y. Right now, pagination only applies vertically, but there are potential use cases for horizontal overflow, for example, SVG backgrounds or multi-column layouts in illustrated books with full-spread images.
It actually is overflowing the viewport. Page size, height, and width define the viewport, not the box itself. Pagination happens when the content box extends beyond the root fragmentainer established by that viewport.
That’s outside the scope of this PR. CSS always has the final say on page size. The CLI option just provides a base page size, see #90 and css-page-3. |
meh they could be handled in CSS styling the root element or the pages, if the root element is overflow scroll we should grow to match
I agree it's not too bad, I need to think a bit about other possibilities and if there's no better solution we could work with that, BUT the existing terminology is paged documents and continuous documents, there's no other concepts and thus no need for other terminology (crop and visible should be done using CSS). Since there's only two options and a magic keyword I think a boolean works the best here. (+ it's easier to remember for users)
yup but that viewport is a social construct in a paginated document, 100vw should always be the width of a page (https://drafts.csswg.org/css-values-4/#viewport-relative-lengths) the viewport size here is irrelevant and everything is based on the CSS declarations.
Granted |
Ok then let's not block the PR over bikesheeding. If one of us has a better name then we can have another PR |
I prefer not to change the CLI every other day, if we can agree on a good interface, then it will last. Since we don't NEED it to be merged right now we can wait till Monday |
This PR focuses on making Paper Muncher easier to use for first-time users and adds more flexible overflow handling. The CLI no longer requires subcommands, and new overflow modes make it possible to generate continuous single-page PDFs.
Simplified CLI
You can now just run:
Why --overflow
The parameter name comes from CSS’s own overflow property, which defines how content that exceeds its container is handled. Paper Muncher now exposes the same concept at the document level, deciding what happens when rendered content exceeds the page boundaries.
paginate - split content into multiple pages
crop - crop content to the page size
visible - render only visible content
auto - automatically choose based on output format