Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

yjbanov
Copy link
Contributor

@yjbanov yjbanov commented May 13, 2020

No description provided.

@yjbanov yjbanov requested a review from cbracken May 13, 2020 21:27
/// vertical component.
const OffsetBase(double dx, double dy)
: _dx = dx ?? 0.0,
_dy = dy ?? 0.0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow -- this was for the STRAIGHT TO RELEASE MODE crowd I guess :)


@override
String toString() => 'OffsetBase(${_dx?.toStringAsFixed(1)}, ${_dy?.toStringAsFixed(1)})';
String/*!*/ toString() => 'OffsetBase(${_dx?.toStringAsFixed(1)}, ${_dy?.toStringAsFixed(1)})';
Copy link
Member

@cbracken cbracken May 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we kill the ?s here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

/// Creates an offset. The first argument sets [dx], the horizontal component,
/// and the second sets [dy], the vertical component.
const Offset(double dx, double dy) : super(dx, dy);
const Offset(double/*!*/ dx, double/*!*/ dy) : super(dx, dy);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optionally, add non-null asserts for dx/dy here for (hopefully very limited-time) future proofing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The super already does that (as of this PR).


@override
String toString() => 'Offset(${dx?.toStringAsFixed(1)}, ${dy?.toStringAsFixed(1)})';
String/*!*/ toString() => 'Offset(${dx?.toStringAsFixed(1)}, ${dy?.toStringAsFixed(1)})';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kill the ?s here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool actually did it automatically! But removed anyway.


@override
String toString() => 'Size(${width?.toStringAsFixed(1)}, ${height?.toStringAsFixed(1)})';
String/*!*/ toString() => 'Size(${width?.toStringAsFixed(1)}, ${height?.toStringAsFixed(1)})';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kill the ?s here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

class Size extends OffsetBase {
/// Creates a [Size] with the given [width] and [height].
const Size(double width, double height) : super(width, height);
const Size(double/*!*/ width, double/*!*/ height) : super(width, height);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again optionally add the asserts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: see the implementation of OffsetBase.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM stamp from a Japanese personal seal

LGTM modulo comments.

@yjbanov
Copy link
Contributor Author

yjbanov commented May 16, 2020

The pending bots are infra flakiness. Merging.

@yjbanov yjbanov merged commit f6847b1 into flutter:master May 16, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 16, 2020
wandyers pushed a commit to wandyers/engine that referenced this pull request May 23, 2020
…tations (flutter#18356)

sync ui geometry.dart into web_ui geometry.dart; add null safety
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants