58133 - Improve logic behind setting state of Constrain checkbox in Image Properties dialog

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect, P3)

Description

[sujay]

using 10/26 build of netscape

1) launch netscape
2) launch composer
3) insert any image
4) go into image dialog
5) click on Custom Size
6) uncheck Constrain option
7) click OK on image dialog panel
8) now go back into the image dialog 

notice the constrain checkbox is checked again.

Comment 1

[Kathleen :Brade]

accepting bug though I don't know that we really want to keep track of whether
the constrain checkbox is checked (for each image? last time in image dialog?)
It would seem like most users would want to constrain most of the time so always
checking the checkbox would seem to be the best thing to do.

Comment 2

[Kathleen :Brade]

hand this bug over to Charley.
I'm not sure if we should do anything about this "bug" or not.  I think the 4.x
behavior was the opposite of this bug (at least on Macintosh); we seem to never
check the constrain checkbox (no matter what state it was last left in).

I think the current behavior in NS6 is desirable and I'm not sure we should
address this bug (maybe make it a P5 bug?).

Comment 3

[Charles Manske]

I'm inclined to keep the default as "constrain" with every new launch of the 
image dialog. I think having constrain on is the most desired state, so 
remembering the previous dialog state doesn't seem the best thing to do.

Comment 4

[Charles Manske]

Anyone who feels strongly about changing current behavior can reopen this bug.

Comment 5

[sujay]

verified in 11/8 build.

Comment 6

[Greg K.]

I'm feeling inclined to reopen this bug. Though the default state of Constrain
should be checked for a given image, once the user unchecks it that action
should persist.

Let's say I decided I want an image to be 100x10 pixels (not its' intrinsic
dimensions). I then want to alter the width a few times to make it match
something a few pixels wider. If I reopen the image properties dialog and change
the width to 101 pixels, then since Constrain is now rechecked (against my
wishes) the height jumps to 68 pixels. Hardly optimal behavior.

As a comparison, Photoshop remembers the Constrain setting on the image across
dialog invocations.

Comment 7

[Kathleen :Brade]

it'd be nice but this isn't a high priority for us right now

Comment 8

[TucsonTester2]

I just recently ran across this bug and thought it was a little annoying.  I was
using a clear pixel gif to help with the layout of my page and had to keep
unchecking the constrain box in order to resize the image.  After having to do
this quite a few times with the same image I found it to be a little frustrating.

Comment 9

[Charles Manske]

Do you expect the "constrain" checkbox state to be remembered for each image
you use? It would be easy to persist the state so the next time you use the 
dialog, it would be the same, but as soon as you change it when editing 
properties for a different image, that would become the new default.
Would that be OK?

Comment 10

[Greg K.]

In truth, yes, it should be remembered on a per-image basis.

Now, I realize this will be impossible across document opens, since the contrain
state won't be saved. What we could do in that instance is check image
dimensions at open, and if they're proportional, then note it as constrained,
but if they're not, then note that it isn't.

Comment 11

[Charles Manske]

Interesting suggestion, since we should know the "original" dimensions of the 
image (unless we can't load the image, such as when user is offline) and we can 
therefore compare those numbers to the html attributes.
I'll look into it later.

Comment 12

[Greg K.]

The original dimensions of the image aren't necessarily important. It's the
dimensions in the HTML or CSS code that are important, since they can override
the intrinsic dimensions.

Comment 13

[Charles Manske]

I didn't explain fully: The original dimensions are important to compare their
ratio to the ratio of the HTML attributes. If the same, I'd probably check the
constrain checkbox, if different, I wouldn't check it.

Comment 14

[Syd Logan]

spam composer change

Comment 15

[Greg K.]

Got it, Charles.

Comment 16

[Charles Manske]

load balancing

Comment 17

[Charles Manske]

Fix for this is ready, just needs a bit more testing. Lower priority, so pushing
off that task.

Comment 18

[Greg K.]

Looking forward to testing it. What'd you do, check proportionality at document
open or at each Image dialog invocation?

Comment 19

[Charles Manske]

I can't attach a patch until I checkin other fixes in the image dialog files, but
all I do is compare the ratio of the actual height/width to the height/width in
the input fields. This is only done if "Custom" checkbox has been checked and 
height or width has been changed. This test is done when any image is loaded into
the dialog.

Comment 20

[Greg K.]

Are there other bugs this one should depend on, then?

Comment 21

[Charles Manske]

Attachment: Fix

Primary fix was very simple: move setting of radio into SetSizeRadio().
I also figure out how Constrain checkbox should be checked base on ratio of
real height/width as compared to users.
Most of the change is substituting "actual..." with "gActual..." for variable 
names.

Comment 22

[Charles Manske]

Greg: not dependencies, just multiple bug fixes in same file.

Comment 23

[[e-mail]]

Comment on attachment 68549 [details] [diff] [review]
Fix

You can't assume that the proportions are goint to be exact. If gActualHeight
== gActualWidth then that's not a problem, otherwise if gActualHeight >
gActualWidth you need to calculate Math.round( gActualWidth * height /
gActualHeight ) == width and vice versa. Otherwise the patch looks fine.

Comment 24

[Charles Manske]

For more accurate ratio calculations, I'll use:
+function  SetSizeRadio(width, height)
+{
+  if (!(width || height) || (gActualWidth && gActualHeight && width ==
gActualWidth && height == gActualHeight))
+    gDialog.actualSizeRadio.radioGroup.selectedItem = gDialog.actualSizeRadio;
+
+  if (!gDialog.actualSizeRadio.selected)
+  {
+    gDialog.actualSizeRadio.radioGroup.selectedItem = gDialog.customSizeRadio;
+    // Decide if user's sizes are in the same ratio as actual sizes
+    if (gActualWidth && gActualHeight)
+    {
+      if (gActualWidth > gActualHeight)
+        gDialog.constrainCheckbox.checked = (Math.round(gActualHeight * width /
gActualWidth) == height);
+      else
+        gDialog.constrainCheckbox.checked = (Math.round(gActualWidth * height /
gActualHeight) == width);
+    }
+}
+

Comment 25

[Charles Manske]

Comment on attachment 68549 [details] [diff] [review]
Fix

r=[e-mail]
(from discussion on IRC)

Comment 26

[Greg K.]

Editing Summary to better reflect the problem and solution.

Comment 27

[kinmoz]

Comment on attachment 68549 [details] [diff] [review]
Fix

sr=[e-mail]

I noticed that some dump statements were added. Are those to debug work in
progress? Will they be removed at a later point?

Comment 28

[Charles Manske]

Attachment: The right files changed!

Sorry! that patch had lots of irrelevant files. This attachment is exactly the
same as what was reviewed, only all of the other file diffs were deleted.

Comment 29

[Charles Manske]

checked in

Comment 30

[Greg K.]

So this should show up in tomorrow's nightly?

Comment 31

[[e-mail]]

Although the code actually checked in is the correct code that I reviewed,
neither of these attachments appears to match up :-)

Comment 32

[Greg K.]

Sujay, you can verify this by double-clicking or contextually-clicking the image.

Comment 33

[sujay]

still not fixed in 2/21 trunk build....that checkbox always gets checked even
though
you uncheck it....

Comment 34

[Greg K.]

You sure, Sujay? It works for me using Mac/2002022108. Constrain checkbox state
is set as appropriate.

Comment 35

[Greg K.]

That is, Sujay, it should be checked even if you uncheck it, as long as you
didn't change the dimensions.

Since there's no way to store checkbox state with the saved document, this is
the only way that makes sense; that is, to detect contrainedness at Image
Properties dialog invocation.

Comment 36

[sujay]

okay it works now...

Comment 37

[sujay]

v