-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: margin parameter for trim method #4484
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
| } else { | ||
| background.resize(image.bands()); | ||
| } | ||
| auto applyMargin = [&](int left, int top, int width, int height) { |
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.
debatable, could be moved to a separate utility method, let me know what you think.
I did it to avoid duplicating those 4 variables 3 times later on.
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 adds a margin parameter to the trim() method, allowing users to expand the trimmed area by a specified number of pixels while clamping to image boundaries. This addresses feature request #4480 and provides more control over trim operations by leaving extra space around the detected content.
Key Changes:
- Added
marginparameter to trim method that expands the detected content area by the specified pixel amount - Implemented proper boundary clamping to prevent overflow beyond original image dimensions
- Added comprehensive test coverage including edge cases for boundary overflow
Reviewed changes
Copilot reviewed 10 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/resize.js | Added margin parameter validation and documentation to trim function |
| lib/index.d.ts | Added TypeScript definition for the new margin option |
| lib/constructor.js | Initialized default trimMargin value to 0 |
| src/operations.h | Updated Trim function signature to accept margin parameter |
| src/operations.cc | Implemented margin logic using lambda to expand bounding boxes with proper clamping |
| src/pipeline.h | Added trimMargin field to PipelineBaton struct |
| src/pipeline.cc | Extracted and passed trimMargin parameter from JavaScript to C++ |
| test/unit/trim.js | Added tests for margin validation, complex/simple gradients, and boundary overflow scenarios |
| test/types/sharp.test-d.ts | Added TypeScript test for margin parameter usage and removed trailing whitespace |
| test/fixtures/index.js | Added new test fixture for gradient border testing |
| package.json | Added contributor credit |
| .gitignore | Added IDE-related files to ignore list |
Comments suppressed due to low confidence (1)
lib/resize.js:578
- Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
this.options.trimMargin = options.margin
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // https://github.com/lovell/sharp/pull/4048 | ||
| sharp(input).composite([ | ||
| { | ||
| input: 'image.gif', |
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.
VSCode insisted on removing these extra spaces when opening the file. Let me know if I should revert these.
| .astro | ||
| docs/dist | ||
| release-notes.md | ||
| .cache |
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.
I use clang for suggestions and type checking:
- it creates a
.cachefolder by default - needs
compile_commands.jsonflags to know where to pull 3rd-party header files from.
I thought future contributors may benefit from this change.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
@lovell Hi, there we go! Let me know what you think :-) |
Co-authored-by: Copilot <[email protected]>
Prerequisites
This PR implements the feature requested here #4480
Implementation
Basically, I expand the extracted area by the specified margin clamping the resulting bounding box by image's own boundaries.
Coverage
Tests pass as expected.