-
Notifications
You must be signed in to change notification settings - Fork 6k
[web] Cleaner types for EngineLineMetrics #33911
Conversation
ditman
left a comment
There was a problem hiding this 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤯
| } | ||
| } | ||
|
|
||
| class RichLineMetrics extends EngineLineMetrics { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
You are absolutely right! You explained the change better than I did in the PR description 😜 |
| } | ||
| } | ||
|
|
||
| class RichEngineLineMetrics extends EngineLineMetrics { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
}
yjbanov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

Improves the
EngineLineMetricstype situation.The class
EngineLineMetricsbecomes just a bag of properties that satisfy theui.LineMetricsinterface.The new class
RichLineMetricsincludes all the properties that are specific to our implementation.This gets rid of several unnecessary initializations with meaningless values: