Move Rectangle and Point into dart:math.

Move Rectangle and Point into dart:math.

BUG=
R=floitsch@google.com

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

[Emily Fortuna, 2013-10-02 23:13:35]

Florian,
This is the same patch as before with a few modifications. The first patchset is
the original one I committed, and Patch Set 2 shows my new changes. There is
still something wrong with the generics or there is something wrong with the
dart2js analyzer, though.... if you patch in my change, and run "python
tools/test.py -mrelease --use-sdk --write-debug-log --checked -t60
dart2js/analyze_api_test" you get the error listed below. 

I've looked over my generics and it's possible I'm missing something obvious (if
so, please enlighten me!). It would appear that the dart2js analyzer isn't using
the fact that T extends from num when it checks if "*" and "+" are valid
operations. Can you verify that dart2js is doing the correct behavior in this
case by throwing an error? 
Thanks.

Here's the error:

FAILED: none-vm-checked release_ia32 dart2js/analyze_api_test
Expected: Pass Slow 
Actual: RuntimeError
CommandOutput[vm]:

stdout:
unittest-suite-wait-for-done
sdk/lib/math/rectangle.dart:95:12: Warning: No operator '<=' in class 'T'.
    return left <= another.left &&
           ^^^^^^^^^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:96:12: Warning: No operator '+' in class 'T'.
           left + width >= another.left + another.width &&
           ^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:97:12: Warning: No operator '<=' in class 'T'.
           top <= another.top &&
           ^^^^^^^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:98:12: Warning: No operator '+' in class 'T'.
           top + height >= another.top + another.height;
           ^^^^^^^^^^^^
sdk/lib/math/point.dart:42:25: Warning: No operator '*' in class 'T'.
    return new Point<T>(x * factor, y * factor);
                        ^^^^^^^^^^
sdk/lib/math/point.dart:42:37: Warning: No operator '*' in class 'T'.
    return new Point<T>(x * factor, y * factor);
                                    ^^^^^^^^^^
sdk/lib/math/point.dart:29:25: Warning: No operator '-' in class 'T'.
    return new Point<T>(x - other.x, y - other.y);
                        ^^^^^^^^^^^
sdk/lib/math/point.dart:29:38: Warning: No operator '-' in class 'T'.
    return new Point<T>(x - other.x, y - other.y);
                                     ^^^^^^^^^^^
sdk/lib/math/rectangle.dart:29:18: Warning: No operator '+' in class 'T'.
  T get right => left + width;
                 ^^^^^^^^^^^^
sdk/lib/math/point.dart:25:25: Warning: No operator '+' in class 'T'.
    return new Point<T>(x + other.x, y + other.y);
                        ^^^^^^^^^^^
sdk/lib/math/point.dart:25:38: Warning: No operator '+' in class 'T'.
    return new Point<T>(x + other.x, y + other.y);
                                     ^^^^^^^^^^^
sdk/lib/math/point.dart:67:14: Warning: No operator '-' in class 'T'.
    var dx = x - other.x;
             ^^^^^^^^^^^
sdk/lib/math/point.dart:68:14: Warning: No operator '-' in class 'T'.
    var dy = y - other.y;
             ^^^^^^^^^^^
sdk/lib/math/point.dart:55:14: Warning: No operator '-' in class 'T'.
    var dx = x - other.x;
             ^^^^^^^^^^^
sdk/lib/math/point.dart:56:14: Warning: No operator '-' in class 'T'.
    var dy = y - other.y;
             ^^^^^^^^^^^
sdk/lib/math/point.dart:49:32: Warning: No operator '*' in class 'T'.
  double get magnitude => sqrt(x * x + y * y);
                               ^^^^^
sdk/lib/math/point.dart:49:40: Warning: No operator '*' in class 'T'.
  double get magnitude => sqrt(x * x + y * y);
                                       ^^^^^
sdk/lib/math/rectangle.dart:126:44: Warning: No operator '+' in class 'T'.
  Point<T> get bottomRight => new Point<T>(this.left + this.width,
                                           ^^^^^^^^^^^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:127:7: Warning: No operator '+' in class 'T'.
      this.top + this.height);
      ^^^^^^^^^^^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:106:25: Warning: No operator '+' in class 'T'.
           another.x <= left + width &&
                        ^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:108:25: Warning: No operator '+' in class 'T'.
           another.y <= top + height;
                        ^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:82:21: Warning: No operator '+' in class 'T'.
    var right = max(this.left + this.width, other.left + other.width);
                    ^^^^^^^^^^^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:82:45: Warning: No operator '+' in class 'T'.
    var right = max(this.left + this.width, other.left + other.width);
                                            ^^^^^^^^^^^^^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:83:22: Warning: No operator '+' in class 'T'.
    var bottom = max(this.top + this.height, other.top + other.height);
                     ^^^^^^^^^^^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:83:46: Warning: No operator '+' in class 'T'.
    var bottom = max(this.top + this.height, other.top + other.height);
                                             ^^^^^^^^^^^^^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:74:13: Warning: No operator '<=' in class 'T'.
    return (left <= other.left + other.width && other.left <= left + width &&
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:74:63: Warning: No operator '+' in class 'T'.
    return (left <= other.left + other.width && other.left <= left + width &&
                                                              ^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:75:9: Warning: No operator '<=' in class 'T'.
        top <= other.top + other.height && other.top <= top + height);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:75:57: Warning: No operator '+' in class 'T'.
        top <= other.top + other.height && other.top <= top + height);
                                                        ^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:56:18: Warning: No operator '+' in class 'T'.
    var x1 = min(left + width, other.left + other.width);
                 ^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:56:32: Warning: No operator '+' in class 'T'.
    var x1 = min(left + width, other.left + other.width);
                               ^^^^^^^^^^^^^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:60:20: Warning: No operator '+' in class 'T'.
      var y1 = min(top + height, other.top + other.height);
                   ^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:60:34: Warning: No operator '+' in class 'T'.
      var y1 = min(top + height, other.top + other.height);
                                 ^^^^^^^^^^^^^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:34:19: Warning: No operator '+' in class 'T'.
  T get bottom => top + height;
                  ^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:157:28: Warning: 'T' is not a subtype of bound 'num'
for type variable 'T' of type 'RectangleBase<T>'.
Try to change or remove the type argument.
class Rectangle<T> extends RectangleBase<T> {
                           ^^^^^^^^^^^^^
sdk/lib/math/rectangle.dart:165:32: Warning: 'T' is not a subtype of bound 'num'
for type variable 'T' of type 'Point<T>'.
Try to change or remove the type argument.
  factory Rectangle.fromPoints(Point<T> a, Point<T> b) {
                               ^^^^^
sdk/lib/math/rectangle.dart:165:44: Warning: 'T' is not a subtype of bound 'num'
for type variable 'T' of type 'Point<T>'.
Try to change or remove the type argument.
  factory Rectangle.fromPoints(Point<T> a, Point<T> b) {
                                           ^^^^^
Uncaught Error: Expect.isFalse(true) fails.
Stack Trace:
#0      Expect._fail (package:expect/expect.dart:368:5)
#1      Expect.isFalse (package:expect/expect.dart:120:10)
#2      CollectingDiagnosticHandler.checkResults
(file:///Users/efortuna/git-dart/dart/tests/compiler/dart2js/analyze_helper.dart:50:19)
#3      analyze.<anonymous closure>
(file:///Users/efortuna/git-dart/dart/tests/compiler/dart2js/analyze_helper.dart:139:25)
#4      _Future._propagateToListeners.<anonymous closure>
(dart:async/future_impl.dart:449)
#6      _rootRun (dart:async/zone.dart:531)
#7      _ZoneDelegate.run (dart:async/zone.dart:344)
#8      _CustomizedZone.run (dart:async/zone.dart:489)
#9      _Future._propagateToListeners (dart:async/future_impl.dart:441)
#10     _Future._complete (dart:async/future_impl.dart:298)
#11     Future.forEach.nextElement (dart:async/future.dart:246)
#12     _Future._propagateToListeners.<anonymous closure>
(dart:async/future_impl.dart:449)
#13     _rootRun (dart:async/zone.dart:531)
#14     _ZoneDelegate.run (dart:async/zone.dart:344)
#15     _CustomizedZone.run (dart:async/zone.dart:489)
#16     _Future._propagateToListeners (dart:async/future_impl.dart:441)
#17     _Future._addListener.<anonymous closure>
(dart:async/future_impl.dart:237)
#18     _asyncRunCallback (dart:async/event_loop.dart:18)
#19     _createTimer.<anonymous closure> (dart:async-patch/timer_patch.dart:11)
#20     _Timer._createTimerHandler._handleTimeout (timer_impl.dart:151)
#21     _Timer._createTimerHandler.<anonymous closure> (timer_impl.dart:168)
#22     _ReceivePortImpl._handleMessage
(dart:isolate-patch/isolate_patch.dart:113)




stderr:
Unhandled exception:
Expect.isFalse(true) fails.
#0      _rootHandleUncaughtError.<anonymous closure> (dart:async/zone.dart:526)
#1      _asyncRunCallback (dart:async/event_loop.dart:18)
#2      _asyncRunCallback (dart:async/event_loop.dart:21)
#3      _createTimer.<anonymous closure> (dart:async-patch/timer_patch.dart:11)
#4      _Timer._createTimerHandler._handleTimeout (timer_impl.dart:151)
#5      _Timer._createTimerHandler._handleTimeout (timer_impl.dart:159)
#6      _Timer._createTimerHandler._handleTimeout (timer_impl.dart:159)
#7      _Timer._createTimerHandler.<anonymous closure> (timer_impl.dart:168)
#8      _ReceivePortImpl._handleMessage
(dart:isolate-patch/isolate_patch.dart:113)

[Lasse Reichstein Nielsen, 2013-10-03 06:24:03]

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart
File sdk/lib/math/point.dart (right):

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:15: String toString() => '($x, $y)';
I wouldn't mind it saying "Point($x, $y)".
Just the "pretty" point syntax is indistinguishable from a two-element iterable.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:24: Point<T> operator +(Point<T> other) {
These are really Vector operations, not Point operations (it's not just
multiplication).
Needs documentation.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:36: * that you can scale Point<double> objects by an
`int` factor. Because the
I don't particularly like this, but I guess it is inevitable.
The alternative is to do:
  if (factor is int && T == int) return new Point<int>(x*factor,y*factor)) 
  return new Point<double>(x * factor, y * factor);

Then we would have to do it everywhere, so operator+ would be
Point<num> operator+(Point<num> other) {
  num newX = x + other.x;
  num newY = y + other.y;
  if (T == int && newX is int && newY is int) {
    return new Point<int>(newX, newY);
  }
  return new Point<double>(newX, newY)
}

That is - the num double-is-contagious behavior extended to Point.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:38: * it, passing in a double [factor] on a Point<int>
_will_ _cause_ _a_
`Point<int>`

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:71: 
Needs documentation.

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

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:26: * The number of units `right` of the origin
where this rectangle's bottom
The x-coordinate of the right edge.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:31: * The number of units below the origin where
this rectangle's bottom
below?
How about "y-coordinate of this rectangle's bottom edge.".

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:50: * Computes the intersection of `this` and
[other].
*
* The intersection of two rectangles, if any, is always a rectangle.
* 
* Returns the intersection of this and `other`, or `null` if they don't
intersect.
*/

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:75: top <= other.top + other.height && other.top <=
top + height);
Personally I'd prefer this indentation:

  return left <= other.left + other.width && 
         other.left <= left + width &&
         top <= other.top + other.height && 
         other.top <= top + height);

It also matches the contains-methods below.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:79: * Returns a new rectangle which completely
contains `this` and [other].
"union" is a surprising term (to me).
How about "boundingRectangle"? Or "boundingBox"?

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:102: * Tests whether `this` contains a point.
Mention that the edges of the rectangle are considered part of the rectangle.
It's not important for the other, purely rectangular, methods, but this method
makes it possible to talk about individual points of the rectangle.

For integer rectangles it's pretty much a given that the edge is included, but
if you only think of doubles, it's not as obvious.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:111: Rectangle<int> ceil() => new
Rectangle<int>(left.ceil(), top.ceil(),
Needs documentation.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:127: this.top + this.height);
Add topRight and bottomLeft getters too, just for consistency.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:138: }
How about:
 num left = min(a.x, b.x);
 num width = a.x + b.x - 2 * left;
It's shorter, but perhaps a little too clever.

It's also so short that you might not need an extra helper method for it.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:157: class Rectangle<T extends num> extends
RectangleBase<T> {
Why have RectangleBase? That as: What is the expected use of RectangleBase?

Since extending RectangleBase won't make something a Rectangle, I don't see much
use for it. 
Also, it diverges from our other naming, where Rectangle would be the interface
and RectangleBase the skeleton implementation of Rectangle. This RectangleBase
is not a Rectangle.

[Lasse Reichstein Nielsen, 2013-10-03 08:41:07]

I think the problem is with the dart2js analyzer.
The static type of 'x' in
  class C<T extends num> { 
   T x;
  }
should be num. Can't find it in the spec, though.

[karlklose, 2013-10-03 09:29:22]

On 2013/10/03 08:41:07, Lasse Reichstein Nielsen wrote:
> I think the problem is with the dart2js analyzer.
> The static type of 'x' in
>   class C<T extends num> { 
>    T x;
>   }
> should be num. Can't find it in the spec, though.

It is a dart2js bug and I fixed it in https://codereview.chromium.org/25698003

[floitsch, 2013-10-03 09:33:49]

Still LGTM (and again with minor nits/comments).

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart
File sdk/lib/math/point.dart (right):

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:33: * Scale this point by [factor] as if it were a
vector.
if it was a vector

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:38: * it, passing in a double [factor] on a Point<int>
_will_ _cause_ _a_
No use for future: _causes a runtime error_ in checked mode.

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

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:111: Rectangle<int> ceil() => new
Rectangle<int>(left.ceil(), top.ceil(),
I don't like ceil/floor/round/truncate on Points, but even less on Rectangle.
If nobody is using them I would prefer removing them.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:157: class Rectangle<T extends num> extends
RectangleBase<T> {
Agreed with Lasse, merge the two classes.

[Emily Fortuna, 2013-10-03 21:29:12]

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart
File sdk/lib/math/point.dart (right):

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:15: String toString() => '($x, $y)';
On 2013/10/03 06:24:03, Lasse Reichstein Nielsen wrote:
> I wouldn't mind it saying "Point($x, $y)".
> Just the "pretty" point syntax is indistinguishable from a two-element
iterable.

Done.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:24: Point<T> operator +(Point<T> other) {
On 2013/10/03 06:24:03, Lasse Reichstein Nielsen wrote:
> These are really Vector operations, not Point operations (it's not just
> multiplication).
> Needs documentation.

Done.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:33: * Scale this point by [factor] as if it were a
vector.
On 2013/10/03 09:33:50, floitsch wrote:
> if it was a vector

Done.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:36: * that you can scale Point<double> objects by an
`int` factor. Because the
On 2013/10/03 06:24:03, Lasse Reichstein Nielsen wrote:
> I don't particularly like this, but I guess it is inevitable.
> The alternative is to do:
>   if (factor is int && T == int) return new Point<int>(x*factor,y*factor)) 
>   return new Point<double>(x * factor, y * factor);
> 
> Then we would have to do it everywhere, so operator+ would be
> Point<num> operator+(Point<num> other) {
>   num newX = x + other.x;
>   num newY = y + other.y;
>   if (T == int && newX is int && newY is int) {
>     return new Point<int>(newX, newY);
>   }
>   return new Point<double>(newX, newY)
> }
> 
> That is - the num double-is-contagious behavior extended to Point.

No Florian and I don't weren't thrilled with it either. :-( 

Why would we want to do that with +, too? I thought the point of distinguishing
Point<int> and Point<double> was so that you *couldn't* add a Point<double> to a
Point<int>?
(I can address whatever we decide in a follow up CL)

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:38: * it, passing in a double [factor] on a Point<int>
_will_ _cause_ _a_
On 2013/10/03 09:33:50, floitsch wrote:
> No use for future: _causes a runtime error_ in checked mode.

Done.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:38: * it, passing in a double [factor] on a Point<int>
_will_ _cause_ _a_
On 2013/10/03 06:24:03, Lasse Reichstein Nielsen wrote:
> `Point<int>`

Done.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/point.dart#ne...
sdk/lib/math/point.dart:71: 
On 2013/10/03 06:24:03, Lasse Reichstein Nielsen wrote:
> Needs documentation.

Done.

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

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:26: * The number of units `right` of the origin
where this rectangle's bottom
On 2013/10/03 06:24:03, Lasse Reichstein Nielsen wrote:
> The x-coordinate of the right edge.

Done.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:31: * The number of units below the origin where
this rectangle's bottom
On 2013/10/03 06:24:03, Lasse Reichstein Nielsen wrote:
> below?
> How about "y-coordinate of this rectangle's bottom edge.".

Done.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:50: * Computes the intersection of `this` and
[other].
On 2013/10/03 06:24:03, Lasse Reichstein Nielsen wrote:
> *
> * The intersection of two rectangles, if any, is always a rectangle.
> * 
> * Returns the intersection of this and `other`, or `null` if they don't
> intersect.
> */

Done.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:75: top <= other.top + other.height && other.top <=
top + height);
On 2013/10/03 06:24:03, Lasse Reichstein Nielsen wrote:
> Personally I'd prefer this indentation:
> 
>   return left <= other.left + other.width && 
>          other.left <= left + width &&
>          top <= other.top + other.height && 
>          other.top <= top + height);
> 
> It also matches the contains-methods below.

Done.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:79: * Returns a new rectangle which completely
contains `this` and [other].
On 2013/10/03 06:24:03, Lasse Reichstein Nielsen wrote:
> "union" is a surprising term (to me).
> How about "boundingRectangle"? Or "boundingBox"?

Done.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:102: * Tests whether `this` contains a point.
On 2013/10/03 06:24:03, Lasse Reichstein Nielsen wrote:
> Mention that the edges of the rectangle are considered part of the rectangle.
> It's not important for the other, purely rectangular, methods, but this method
> makes it possible to talk about individual points of the rectangle.
> 
> For integer rectangles it's pretty much a given that the edge is included, but
> if you only think of doubles, it's not as obvious.

Done.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:111: Rectangle<int> ceil() => new
Rectangle<int>(left.ceil(), top.ceil(),
On 2013/10/03 09:33:50, floitsch wrote:
> I don't like ceil/floor/round/truncate on Points, but even less on Rectangle.
> If nobody is using them I would prefer removing them.

Left the round in but removed everything else so that people can "switch" from a
Point/Rect<double> to a Point<int>. If you prefer I can remove round as well.
Let me know.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:127: this.top + this.height);
On 2013/10/03 06:24:03, Lasse Reichstein Nielsen wrote:
> Add topRight and bottomLeft getters too, just for consistency.

Done.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:138: }
On 2013/10/03 06:24:03, Lasse Reichstein Nielsen wrote:
> How about:
>  num left = min(a.x, b.x);
>  num width = a.x + b.x - 2 * left;
> It's shorter, but perhaps a little too clever.
> 
> It's also so short that you might not need an extra helper method for it.

Done.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:157: class Rectangle<T extends num> extends
RectangleBase<T> {
On 2013/10/03 09:33:50, floitsch wrote:
> Agreed with Lasse, merge the two classes.

I need to separate them out so that I can reuse the code for the mutable version
of the rectangle in dart:html, CssRectangle. Should I swap the names? Make the
top level a Rectangle and rename the implementation something else?

[floitsch, 2013-10-04 08:59:12]

Almost there.
LGTM with base class addressed.

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

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:111: Rectangle<int> ceil() => new
Rectangle<int>(left.ceil(), top.ceil(),
On 2013/10/03 21:29:12, Emily Fortuna wrote:
> On 2013/10/03 09:33:50, floitsch wrote:
> > I don't like ceil/floor/round/truncate on Points, but even less on
Rectangle.
> > If nobody is using them I would prefer removing them.
> 
> Left the round in but removed everything else so that people can "switch" from
a
> Point/Rect<double> to a Point<int>. If you prefer I can remove round as well.
> Let me know.

Let's remove it for now. We can add it later if we see real use-cases.

https://codereview.chromium.org/25808002/diff/3001/sdk/lib/math/rectangle.dar...
sdk/lib/math/rectangle.dart:157: class Rectangle<T extends num> extends
RectangleBase<T> {
On 2013/10/03 21:29:12, Emily Fortuna wrote:
> On 2013/10/03 09:33:50, floitsch wrote:
> > Agreed with Lasse, merge the two classes.
> 
> I need to separate them out so that I can reuse the code for the mutable
version
> of the rectangle in dart:html, CssRectangle. Should I swap the names? Make the
> top level a Rectangle and rename the implementation something else?

In that case, either:
- add MutableRectangle into dart:math and hide the base class, or
- create hidden rectangle base-class in _collection_dev and use it from there in
dart:html.

I lean towards MutableRectangle in dart:math.

[Emily Fortuna, 2013-10-04 17:47:56]

Committed patchset #4 manually as r28272 (presubmit successful).

[siva, 2013-10-08 22:05:30]

DBC

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

https://codereview.chromium.org/25808002/diff/28001/sdk/lib/math/rectangle.da...
sdk/lib/math/rectangle.dart:157: }
Wouldn't it make more sense to call the classes :

ImmutableRectangle
Rectangle

Currently Rectangle does not seem to convey the fact that it is immutable.

[Emily Fortuna, 2013-10-08 22:20:26]

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

https://codereview.chromium.org/25808002/diff/28001/sdk/lib/math/rectangle.da...
sdk/lib/math/rectangle.dart:157: }
On 2013/10/08 22:05:30, siva wrote:
> Wouldn't it make more sense to call the classes :
> 
> ImmutableRectangle
> Rectangle
> 
> Currently Rectangle does not seem to convey the fact that it is immutable.

We convey immutability that in the documentation. And we have the immutable
version simply called "Rectangle" to encourage users to use that one, as it is
more performant, which would generally be more desirable if you're in the
dart:math class.