-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Monomorphic Mapper #154972
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Monomorphic Mapper #154972
Conversation
59bf976 to
97d4852
Compare
efa6ded to
c598bf4
Compare
MitchellGoodwin
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.
Just reviewing the WidgetStateInputBorder migration before doing a separate review of the mappers. That maybe should have been it's own PR. Code LGTM. Some doc nits, and I think there needs to be tests specific to the new class
MitchellGoodwin
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.
Mapper code LGTM, no nits there.
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.
@MitchellGoodwin thanks so much for the review! You're right that I should have added the mapper class in a separate PR first before doing the WidgetStateInputBorder stuff. Many thanks for looking over the whole thing regardless 🙏
5a20f70 to
5a97c7b
Compare
|
I've updated this pull request so it only includes the No rush on the review—I heard there's a lot of internal training going on this week. |
MitchellGoodwin
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.
LGTM! Thank you very much!
|
@MitchellGoodwin thanks to you for the review! I wanted to ask a question before landing this one… I've been thinking about potentially cherry-picking this change into the upcoming stable release:
Would this be worth pursuing? |
As this wouldn't be a breaking change with .fromMap() and typos in the documentation aren't critical, I don't think there's a good chance of it being cherry picked. Cherry picking is expensive on our current infrastructure team, and is usually reserved for p1/p0 issues. You can ask around in the discord for opinions from people more closely involved with releases, but I don't think it's likely to go through. Last release there were some changes that didn't make the cut which I really wanted to get through but wasn't able to 😅 |
Ah, this is good to know. Thanks! |
This class looks like it _should_ give valid equality checks:
```dart
class _WidgetStateColorMapper extends _WidgetStateColorTransparent {
const _WidgetStateColorMapper(this.map);
final WidgetStateMap<Color> map;
_WidgetStateMapper<Color> get _mapper => _WidgetStateMapper<Color>(map);
@OverRide
Color resolve(Set<WidgetState> states) => _mapper.resolve(states);
@OverRide
bool operator ==(Object other) {
return other is _WidgetStateColorMapper && other.map == map;
}
@OverRide
int get hashCode => map.hashCode;
}
```
But I made a mistake: I should have used `other._mapper == _mapper` and `_mapper.hashCode`.
<br>
It'd be pretty nice if no copy-pasting was needed in the first place:
```dart
class _WidgetStateColorMapper extends WidgetStateMapper<Color> implements WidgetStateColor {
const _WidgetStateColorMapper(super.map);
}
```
<br><br>
This PR makes `WidgetStateMapper` a public class, similar to `WidgetStatePropertyAll`. The new API, when used incorrectly, will `throw` with a detailed error message, rather than silently returning irrelevant values.
**Changes** - Add `WidgetStateInputBorder` class, with `.resolveWith()` and `.fromMap()` constructors - Deprecate `MaterialStateOutlineInputBorder` and `MaterialStateUnderlineInputBorder` and provide data-driven fixes <br> **Other changes** based on #154972 (review) - Fix documentation copy-paste typo ("OutlinedBorder" � "InputBorder") - Add test to ensure borders are painted correctly - Add DartPad sample & relevant test
**Changes** - Add `WidgetStateInputBorder` class, with `.resolveWith()` and `.fromMap()` constructors - Deprecate `MaterialStateOutlineInputBorder` and `MaterialStateUnderlineInputBorder` and provide data-driven fixes <br> **Other changes** based on flutter#154972 (review) - Fix documentation copy-paste typo ("OutlinedBorder" � "InputBorder") - Add test to ensure borders are painted correctly - Add DartPad sample & relevant test
This class looks like it should give valid equality checks:
But I made a mistake: I should have used
other._mapper == _mapperand_mapper.hashCode.It'd be pretty nice if no copy-pasting was needed in the first place:
This PR makes
WidgetStateMappera public class, similar toWidgetStatePropertyAll. The new API, when used incorrectly, willthrowwith a detailed error message, rather than silently returning irrelevant values.