fix: VipsDriver#resize_image_to resizes to the requested dimensions - #205
Conversation
Reviewer's GuideThis PR fixes the Vips driver’s resize logic to use proper horizontal and vertical scale factors so images are resized to the requested dimensions, and adds focused tests to enforce the contract across drivers and along the production retina-screenshot path, plus a strengthened load_images contract. Sequence diagram for retina screenshot resizingsequenceDiagram
participant Screenshoter
participant VipsDriver
participant VipsImage
Screenshoter->>Screenshoter: process_screenshot
Screenshoter->>Screenshoter: resize_if_needed
Screenshoter->>VipsDriver: resize_image_to(image, 1280, 800)
VipsDriver->>VipsImage: resize(1280.0 / image.width, vscale: 800.0 / image.height)
VipsImage-->>VipsDriver: image sized 1280x800
VipsDriver-->>Screenshoter: resized image
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe Vips driver now uses independent horizontal and vertical scaling factors. Tests cover exact resizing of non-square images, retina screenshot resizing, and image slot identity. ChangesVips image resizing
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
VipsDriver#resize_image_to passed image.resize() the aspect ratio (new_width / new_height) instead of a scale factor, which is what Vips::Image#resize expects. An 80x80 source resized to [40, 30] returned [107, 107] instead of [40, 30]. On the production path (Screenshoter#resize_if_needed, macOS + Selenium + retina), a 2560x1600 screenshot with expected width 1280 computed the buggy factor 1280/800 = 1.6, enlarging to [4096, 2560] instead of halving to [1280, 800]. Fix uses libvips resize's documented vscale option to set width and height scale factors independently: image.resize(scale, vscale:). Adds a strict driver-contract test (resize_image_to on a non-square source to a non-square target returns exactly [new_width, new_height]) that reds on Vips and passes on ChunkyPNG, plus a focused Screenshoter test exercising resize_if_needed's formula through VipsDriver on a synthetic 2560x1600 retina image, and strengthens the load_images ordering contract test to use fixtures with different dimensions so a swapped return goes red.
f28cb05 to
56f2cc7
Compare
Bug
VipsDriver#resize_image_tocalledimage.resize(new_width.to_f / new_height).Vips::Image#resizetakes a uniform scale factor, not target dimensions — the code was computing the target aspect ratio and passing that as the scale.Reproduction (
test/fixtures/images/a.png, 80x80):Production call chain
Screenshoter#process_screenshot(screenshoter.rb:54) →#resize_if_needed(screenshoter.rb:114-122), triggered only on macOS + Selenium + a retina display (selenium_with_retina_screen?). For a 2560x1600 screenshot with an expected window width of 1280:The crop step that follows then operates on a misaligned, wrongly-sized image, so retina baselines get saved at the wrong dimensions.
Fix
Vips::Image#resizeaccepts an independentvscale(vertical scale factor) option alongside the required horizontalscaleargument — confirmed against the installedruby-vips2.3.0 / libvipsresizeoperation:This resizes to the exact requested
[new_width, new_height]in one call, with no aspect-preserving surprises — the minimal change that keeps the method's existing shape (stillimage.resize(...), still returns aVips::Image). ConsideredVips::Image#thumbnail_image(width, height:, size: :force)as an alternative;resizewithvscale:was chosen as the smaller diff since the method already calledresize.Tests
test/support/driver_contract_tests.rb: added[contract] resize_image_to resizes a non-square source to the exact requested non-square dimensions, run via the sharedDriverContractTestsmodule against both drivers (6 instances: 5ChunkyPNGDriverTestnested classes + 1VipsDriverTest). Before the fix: 1 failure (Vips) —Expected: [40, 30] Actual: [4, 8]on a 3x6portrait.pngsource; ChunkyPNG'sresample_bilinear(new_width, new_height)already passed.test/unit/screenshoter_test.rb: added#resize_if_needed halves a non-square retina screenshot to the expected window size via VipsDriver, driving a synthetic 2560x1600Vips::Imagethrough the realresize_if_neededformula (Screenshot.window_sizestubbed to[1280, 1024]) and asserting[1280, 800]. Verified this reds against the pre-fix code with the exact reported numbers (Expected: [1280, 800] Actual: [4096, 2560]), then confirmed green after the fix.[contract] load_images returns [old_image, new_image] without swapping slotsto usea.png(80x80) anda_cropped.png(80x60) instead of same-dimension fixtures, so a swap is actually detectable.Gate-checks
#resize_if_neededscreenshoter test → red (Actual: [4096, 2560]), matching the bug report's numbers exactly. Reapplied the fix → green.ChunkyPNGDriver#_load_images's return order → the strengthened contract test reds in all 5 nested test classes (Expected: [80, 80] Actual: [80, 60]). Reverted → green.Verification
mise x ruby@4.0.6 -- bundle exec rake test:unit— 280 runs, 825 assertions, 0 failures, 0 errors, 0 skips (baseline was 267 runs/0 failures before this change; +13 new: 2 new contract tests × 6 driver-test instances + 1 screenshoter test).mise x ruby@4.0.6 -- bundle exec rake test— 313 runs, 870 assertions, 0 failures, 0 errors, 6 skips (baseline 300 runs/0 failures/6 skips).mise x ruby@4.0.6 -- bundle exec standardrb— 97 files inspected, no offenses.CHANGELOG.mdnot touched.🤖 Generated with Claude Code
Summary by Sourcery
Correct VipsDriver resizing to use independent horizontal and vertical scale factors while strengthening regression coverage for resizing and image ordering.
Bug Fixes:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests