Opened 9 years ago

Closed 9 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)

11207.patch (3.2 KB) - added by dante 9 years ago.
a .patch version of a [slightly adjusted for style] the above code against trunk

Download all attachments as: .zip

Change History (11)

comment:1 Changed 9 years ago by Adam Peller

Owner: changed from Adam Peller to dante

comment:2 Changed 9 years ago by Eric Pasquier

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).

comment:3 Changed 9 years ago by dante

Milestone: tbd1.5
Priority: highnormal
Status: newassigned

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 9 years ago by Eric Pasquier

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:5 Changed 9 years ago by Eric Pasquier

Hi, Dante. I just sent the signed CLA by Fax.

comment:6 Changed 9 years ago by dante

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 9 years ago by dante

Attachment: 11207.patch added

a .patch version of a [slightly adjusted for style] the above code against trunk

comment:7 Changed 9 years ago by dante

@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 9 years ago by Eric Pasquier

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 9 years ago by Adam Peller

Milestone: 1.51.6

comment:10 Changed 9 years ago by dante

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.