Make Rectangle and MutableRectangle constructors handle negative lengths.

Make Rectangle and MutableRectangle constructors handle negative lengths.

Now handles negative width/height so that it's equivalent to
   new Rectangle.fromPoints(new Point(left, top),
                            new Point(left + width, top + height))

BUG= http://dartbug.com/16167
R=efortuna@google.com

Committed: https://code.google.com/p/dart/source/detail?r=32736

[Lasse Reichstein Nielsen, 2014-01-28 15:35:12]

Are there any HTML rectangles that needs adapting too?

[srdjan, 2014-01-28 16:47:41]

https://codereview.chromium.org/135273009/diff/1/sdk/lib/math/rectangle.dart
File sdk/lib/math/rectangle.dart (right):

https://codereview.chromium.org/135273009/diff/1/sdk/lib/math/rectangle.dart#...
sdk/lib/math/rectangle.dart:141: height = (height >= 0) ? height : -height;
This looks very unreadable to me (same names for arguments and instance
variables). Shouldn't the arguments be typed? (see style guide)

[Lasse Reichstein Nielsen, 2014-01-28 17:49:59]

https://codereview.chromium.org/135273009/diff/1/sdk/lib/math/rectangle.dart
File sdk/lib/math/rectangle.dart (right):

https://codereview.chromium.org/135273009/diff/1/sdk/lib/math/rectangle.dart#...
sdk/lib/math/rectangle.dart:141: height = (height >= 0) ? height : -height;
Yes, they should be typed "T", and I should write "this.left" on the left-hand
side of the '='.

[Emily Fortuna, 2014-01-29 19:07:01]

Do we really want to silently modify and return the absolute value of the
negative number in the constructor? I'd advocate for throwing some sort of
ArgumentError instead. Or is this because the constructors are const? :-P

[Lasse Reichstein Nielsen, 2014-01-30 06:55:47]

On 2014/01/29 19:07:01, Emily Fortuna wrote:
> Do we really want to silently modify and return the absolute value of the
> negative number in the constructor? I'd advocate for throwing some sort of
> ArgumentError instead. Or is this because the constructors are const? :-P

It is because the constructor in Rectangle is const, yes.
The initializers of const constructors must be potential compile-time constant
expressions, so we can't use a throw on one of the condiational branches. (We
could lobby for that, but I don't think a language change at this point is
likely :)

I wanted it to do something reasonable, so flipping the rectangle coordinates
while maintaining the same set of spanned points seemed the simplest reasonable
choice.
Another option is to cap them at zero, so width < 0 becomes width == 0. That's
also kind of reasonable, and possibly even a better choice in practice (less
surprise - only the incorrect parameter is affected).

The constructor in MutableRectangle isn't const, but I wanted them to act the
same.
That might just be needless insisting to consistency.

[Lasse Reichstein Nielsen, 2014-01-30 13:32:47]

I'm starting to like truncating negative lengths to zero instead of trying to
reverse the rectangle.
It also handles -Infinity better.
I'll upload a version doing that instead, and you can say which one you prefer.

[Lasse Reichstein Nielsen, 2014-01-30 13:42:34]

PTAL if this is better than the previous version.

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
File sdk/lib/math/rectangle.dart (right):

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
sdk/lib/math/rectangle.dart:215: : this._width = (width >= 0) ? width : -width *
0,
-width * 0  is integer zero if width is int and double 0.0 if width is double.
The '-' is to avoid -0.0.

[srdjan, 2014-01-30 15:28:44]

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
File sdk/lib/math/rectangle.dart (right):

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
sdk/lib/math/rectangle.dart:145: * If `width` is negative, it is truncated to
zero.
I think clamped to zero is a better term than truncated to zero.

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
sdk/lib/math/rectangle.dart:146: * Similar for `height`.
Please repeat the comment, instead of saying similar.

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
sdk/lib/math/rectangle.dart:208: * Similar for `height`.
dito

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
sdk/lib/math/rectangle.dart:215: : this._width = (width >= 0) ? width : -width *
0,
On 2014/01/30 13:42:34, Lasse Reichstein Nielsen wrote:
> -width * 0  is integer zero if width is int and double 0.0 if width is double.
> The '-' is to avoid -0.0.

This is not as evident from code itself, please add it as comment. Why do you
need to avoid -0.0?

[Emily Fortuna, 2014-01-30 18:35:13]

looks good with adjustments.

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
File sdk/lib/math/rectangle.dart (right):

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
sdk/lib/math/rectangle.dart:152: : this.width = (width >= 0) ? width : -width *
0,
Why not:
this.width = (width >= 0) ? width : 0,  ...

instead?

I do like the clamped at zero idea better though. It makes it a bit more obvious
that the data you passed is "invalid"

[srdjan, 2014-01-30 18:46:40]

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
File sdk/lib/math/rectangle.dart (right):

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
sdk/lib/math/rectangle.dart:152: : this.width = (width >= 0) ? width : -width *
0,
On 2014/01/30 18:35:13, Emily Fortuna wrote:
> Why not:
> this.width = (width >= 0) ? width : 0,  ...
> 
> instead?
> 
> I do like the clamped at zero idea better though. It makes it a bit more
obvious
> that the data you passed is "invalid"

The problem is that width is of 'T extends num', and 0 is only integer. I.e., if
T is double, 0 is not assignable to width. Tricky. Only removing generics or
clamping seems to be cleaner.

[Emily Fortuna, 2014-01-30 18:56:23]

lgtm.

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
File sdk/lib/math/rectangle.dart (right):

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
sdk/lib/math/rectangle.dart:152: : this.width = (width >= 0) ? width : -width *
0,
On 2014/01/30 18:46:40, srdjan wrote:
> On 2014/01/30 18:35:13, Emily Fortuna wrote:
> > Why not:
> > this.width = (width >= 0) ? width : 0,  ...
> > 
> > instead?
> > 
> > I do like the clamped at zero idea better though. It makes it a bit more
> obvious
> > that the data you passed is "invalid"
> 
> The problem is that width is of 'T extends num', and 0 is only integer. I.e.,
if
> T is double, 0 is not assignable to width. Tricky. Only removing generics or
> clamping seems to be cleaner.

Ah yes. Touché. (can we maybe at a little comment here mentioning that for
someone like me that was scanning it quickly and didn't realize that subtlety
the first time?)

[Lasse Reichstein Nielsen, 2014-01-31 08:51:53]

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
File sdk/lib/math/rectangle.dart (right):

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
sdk/lib/math/rectangle.dart:145: * If `width` is negative, it is truncated to
zero.
True, and it matches the naming for Uint8ClampedList.
Will change.

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
sdk/lib/math/rectangle.dart:146: * Similar for `height`.
On 2014/01/30 15:28:45, srdjan wrote:
> Please repeat the comment, instead of saying similar.

Done.

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
sdk/lib/math/rectangle.dart:152: : this.width = (width >= 0) ? width : -width *
0,
I'll add a statuc "clampToZero" function and use that, and then document the
function (there isn't room to document it here anyway).

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
sdk/lib/math/rectangle.dart:208: * Similar for `height`.
On 2014/01/30 15:28:45, srdjan wrote:
> dito

Done.

https://codereview.chromium.org/135273009/diff/120001/sdk/lib/math/rectangle....
sdk/lib/math/rectangle.dart:215: : this._width = (width >= 0) ? width : -width *
0,
I don't *need* to avoid -0.0, but I want to if I can (which I can, so I do :).
Here I get to choose between picking -0.0 and 0.0 as the value, so I just pick
0.0, even if it costs one extra operation.

[Lasse Reichstein Nielsen, 2014-02-18 10:12:33]

Committed patchset #8 manually as r32736 (presubmit successful).