Opened 12 years ago
Closed 12 years ago
#11207 closed defect (fixed)
dojox.image.lightbox window resize erroneous algorithm
Reported by: | Eric Pasquier | Owned by: | dante |
---|---|---|---|
Priority: | high | Milestone: | 1.6 |
Component: | Dojox | Version: | 1.4.2 |
Keywords: | lightbox | Cc: | |
Blocked By: | Blocking: |
Description
The algoritm used in _scaleToFit is completly erroneous to resize an image to a viewport. Please find below a correction :
_scaleToFit: function(/* Object */size){ // summary: resize an image to fit within the bounds of the viewport // size: Object // The 'size' object passed around for this image var ns = {}; // New size var nvp = {}; // New viewport // Take borders/title into account nvp.w = this._vp.w - 80; nvp.h = this._vp.h - 60 - this._lastTitleSize.h; // Calculate aspect ratio var viewportAspect = nvp.w / nvp.h; var imageAspect = size.w / size.h; // Calculate new image size if (imageAspect >= viewportAspect) { ns.h = nvp.w / imageAspect; ns.w = nvp.w; } else { ns.w = imageAspect * nvp.h; ns.h = nvp.h; } // we actually have to style this image, it's too big this._wasStyled = true; this._setImageSize(ns); ns.duration = size.duration; return ns; // Object }
Eric.
Attachments (1)
Change History (11)
comment:1 Changed 12 years ago by
Owner: | changed from Adam Peller to dante |
---|
comment:2 Changed 12 years ago by
comment:3 Changed 12 years ago by
Milestone: | tbd → 1.5 |
---|---|
Priority: | high → normal |
Status: | new → assigned |
sorry -- my comment was lost somehow:
@Eric - do you have a CLA on file?
Also, not a super large deal, but the preferred way for patch submission is via unified diff against trunk attached with a .diff or .patch extension for easy review.
Do any of the tests show this resizing error? I have the one large 1:1 aspect ratio test that seems fine, but I presume this comes from images with odd/excessive aspects?
comment:4 Changed 12 years ago by
Hi, Dante, The first test I made was to display a 2590x1920 image (taken with my 10-years old camera): the image displayed is bigger than the screen. Then, I built 2590*1000 and 1000x1920 images that confirm the problem.
The algorithm implemented surprised me, and contrast a lot with the work done for the entire control: testing the "biggest edge of the viewport" to resize an image is really something innovative.
Anyway, feel free to use my suggestion. I have been using this algorithm for years in a personal VB application developped by myself (a control to display & print images !).
I don't know anything about CLA, unified diff and so on. If you are the developper of the control, it should be easy for you to correct it. Otherwise, tell me how can I help. I can supply the complete new js file.
Eric.
comment:6 Changed 12 years ago by
Awesome! I've created tall and wide test images, and am only seeing the error in really wide images. Will apply your patch and see if that resolves it.
Changed 12 years ago by
Attachment: | 11207.patch added |
---|
a .patch version of a [slightly adjusted for style] the above code against trunk
comment:7 Changed 12 years ago by
@Eric - everything looks great. the above attachment is a diff from svn diff
after applying [and slightly modifying] your patch. Once I get the confirmation of the CLA I'll go ahead and commit the fix. Thanks for fixing my sloppy algo! I see know how very 'creative' my solution was. I may have been drunk. :)
comment:8 Changed 12 years ago by
Yes, now your control is very smooth to use, even if you resize the browser during display. I didn't do it, you can add a test when the browser's window is very small: the nvp values become negatives. In this case, define a fixed size (just to be perfect !)
comment:9 Changed 12 years ago by
Milestone: | 1.5 → 1.6 |
---|
I'd like to add that this defect is also present on 1.5.0b3: large images are not correctly resized with the actual function and can be larger than the screen, with no possibility to close the popup (button not visible).