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

Conversation

@mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Jun 8, 2022

Improves the EngineLineMetrics type situation.

The class EngineLineMetrics becomes just a bag of properties that satisfy the ui.LineMetrics interface.

The new class RichLineMetrics includes all the properties that are specific to our implementation.

This gets rid of several unnecessary initializations with meaningless values:

image

@mdebbar mdebbar added the platform-web Code specifically for the web engine label Jun 8, 2022
@mdebbar mdebbar requested review from ditman and yjbanov June 8, 2022 19:33
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

I'd call the class RichEngineLineMetrics so it's clear that it extends EngineLineMetrics, but other than that, this makes sense.

The TL:DR; of this change is: there was a special .rich constructor of EngineLineMetrics that added a bunch of new properties to the class, that now have been promoted to the new RichLineMetrics class.

🚀

int get nonTrailingSpaceBoxCount => spaceBoxCount - trailingSpaceBoxCount;

bool overlapsWith(int startIndex, int endIndex) {
return startIndex < this.endIndex && this.startIndex < endIndex;
Copy link
Member

Choose a reason for hiding this comment

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

🤯

}
}

class RichLineMetrics extends EngineLineMetrics {
Copy link
Member

Choose a reason for hiding this comment

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

(I'd call this RichEngineLineMetrics, but that might be my Java subconscious speaking)

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!

@mdebbar
Copy link
Contributor Author

mdebbar commented Jun 9, 2022

The TL:DR; of this change is: there was a special .rich constructor of EngineLineMetrics that added a bunch of new properties to the class, that now have been promoted to the new RichLineMetrics class.

You are absolutely right! You explained the change better than I did in the PR description 😜

}
}

class RichEngineLineMetrics extends EngineLineMetrics {
Copy link
Contributor

@yjbanov yjbanov Jun 9, 2022

Choose a reason for hiding this comment

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

nit: I have never been able to build a clean conceptual model for data classes that used inheritance. For example, the runtimeType check breaks the substitutability property that's expected from the inheriting class. Could composition work here? For example, the EngineLineMetrics could have a nullable field RichMetrics:

class EngineLineMetrics implements ui.LineMetrics {
  const EngineLineMetrics(this.otherProperties) : richMetrics = null;

  const EngineLineMetrics.rich(this.otherProperties, this.richMetrics);

  final RichMetrics? richMetrics;
}

The relationship can also be inverted, such that RichMetrics contains EngineLineMetrics. Depends on the use-case.

Composition should still be able to clean up most of the noise with unrelated properties, but it would avoid complexities from inheritance, such as the unusual usage of super in hashCode and operator ==, and the shaky substitutability of the two types.

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 sample you provided could work, but it means I have to add ! everywhere:

lineMetrics.richMetrics!.*

Reversing the composition might work better in this case:

class ParagraphLine { // Instead of RichEngineLineMetrics
  const ParagraphLine(this.lineMetrics, ... other properties);

  final ui.LineMetrics lineMetrics;
}

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

lgtm

@mdebbar mdebbar added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 10, 2022
@fluttergithubbot fluttergithubbot merged commit 2245bda into flutter:main Jun 10, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 10, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 10, 2022
houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022
@mdebbar mdebbar deleted the engine_line_metrics branch January 17, 2023 18:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

platform-web Code specifically for the web engine waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants