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 14, 2020

Related Issues

flutter/flutter#53661

@yjbanov yjbanov requested a review from cbracken May 14, 2020 22:11
@auto-assign auto-assign bot requested a review from chinmaygarde May 14, 2020 22:13
@yjbanov yjbanov requested a review from zanderso June 3, 2020 17:44
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

Noticed some function parameter types that look like they are missing annotations.


/// The text before this range.
String textBefore(String text) {
String/*!*/ textBefore(String text) {
Copy link
Member

Choose a reason for hiding this comment

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

Here and below do the parameters need to be annotated?

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.

@@ -1423,23 +1424,23 @@ abstract class Paragraph {
/// The [boxHeightStyle] and [boxWidthStyle] parameters must not be null.
///
/// See [BoxHeightStyle] and [BoxWidthStyle] for full descriptions of each option.
List<TextBox> getBoxesForRange(int start, int end,
List<TextBox/*!*/>/*!*/ getBoxesForRange(int start, int end,
Copy link
Member

Choose a reason for hiding this comment

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

ditto here and below

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.

@@ -360,17 +360,17 @@ class TextDecoration {
const TextDecoration._(this._mask);

/// Creates a decoration that paints the union of all the given decorations.
factory TextDecoration.combine(List<TextDecoration> decorations) {
factory TextDecoration.combine(List<TextDecoration/*!*/>/*!*/ decorations) {
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated question: How does nnbd handle factory constructors that return null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Factories are not allowed to return null any more.

@@ -131,7 +131,7 @@ class FontWeight {
///
/// Values for `t` are usually obtained from an [Animation<double>], such as
/// an [AnimationController].
static FontWeight lerp(FontWeight a, FontWeight b, double t) {
static FontWeight/*?*/ lerp(FontWeight/*?*/ a, FontWeight/*?*/ b, double/*!*/ t) {
Copy link
Member

Choose a reason for hiding this comment

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

values doesn't have any null entries:

Suggested change
static FontWeight/*?*/ lerp(FontWeight/*?*/ a, FontWeight/*?*/ b, double/*!*/ t) {
static FontWeight/*!*/ lerp(FontWeight/*?*/ a, FontWeight/*?*/ b, double/*!*/ t) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but there's also:

    if (a == null && b == null)
      return null;

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I don't follow =) Could you spell it out for me a bit more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I was looking at the lib/ui/text.dart, where the implementation is as follows:

  static FontWeight/*?*/ lerp(FontWeight/*?*/ a, FontWeight/*?*/ b, double/*!*/ t) {
    assert(t != null);
    if (a == null && b == null)
      return null;
    return values[lerpDouble(a?.index ?? normal.index, b?.index ?? normal.index, t).round().clamp(0, 8) as int];
  }

I should just sync the two so they are consistent. Both should be returning FontWeight? though.

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.

return (direction == TextDirection.ltr) ? right : left;
}

@override
bool operator ==(dynamic other) {
bool/*!*/ operator ==(dynamic other) {
Copy link
Member

Choose a reason for hiding this comment

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

Does dynamic imply ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes (but Object doesn't)

@yjbanov
Copy link
Contributor Author

yjbanov commented Jun 5, 2020

The luci-engine failure is due to flutter/flutter#58785. Ignoring it.

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