Skip to content

Conversation

@nate-thegrate
Copy link
Contributor

@nate-thegrate nate-thegrate commented Sep 11, 2024

This class looks like it should give valid equality checks:

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.


It'd be pretty nice if no copy-pasting was needed in the first place:

class _WidgetStateColorMapper extends WidgetStateMapper<Color> implements WidgetStateColor {
  const _WidgetStateColorMapper(super.map);
}



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.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. c: tech-debt Technical debt, code quality, testing, etc. labels Sep 11, 2024
@nate-thegrate nate-thegrate marked this pull request as ready for review September 11, 2024 05:03
@nate-thegrate nate-thegrate force-pushed the monomorphic-mapper branch 2 times, most recently from efa6ded to c598bf4 Compare September 26, 2024 01:46
Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a 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

Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a 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.

@github-actions github-actions bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Oct 4, 2024
Copy link
Contributor Author

@nate-thegrate nate-thegrate left a 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 🙏

@github-actions github-actions bot removed f: material design flutter/packages/flutter/material repository. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos c: tech-debt Technical debt, code quality, testing, etc. labels Oct 10, 2024
@nate-thegrate
Copy link
Contributor Author

I've updated this pull request so it only includes the WidgetStateMapper changes; we can save WidgetStateInputBorder for a future PR.

No rush on the review—I heard there's a lot of internal training going on this week.

Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a 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!

@nate-thegrate
Copy link
Contributor Author

@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:

  • There might be some frustration if .fromMap() is introduced and then is updated 3 months later to start throwing errors more often.
  • Plus, it'd be nice to not ship this out with the typo in the WidgetStateColor.fromMap() equality check.

Would this be worth pursuing?

@MitchellGoodwin
Copy link
Contributor

I've been thinking about potentially cherry-picking this change into the upcoming stable release:

There might be some frustration if .fromMap() is introduced and then is updated 3 months later to start throwing errors more often.
Plus, it'd be nice to not ship this out with the typo in the WidgetStateColor.fromMap() equality check.
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 😅

@nate-thegrate
Copy link
Contributor Author

Cherry picking is expensive on our current infrastructure team, and is usually reserved for p1/p0 issues.

Ah, this is good to know. Thanks!

@nate-thegrate nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 16, 2024
@auto-submit auto-submit bot merged commit 8d1ea1e into flutter:master Oct 16, 2024
@nate-thegrate nate-thegrate deleted the monomorphic-mapper branch October 16, 2024 21:08
Lurchfresser pushed a commit to Lurchfresser/flutter that referenced this pull request Oct 17, 2024
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.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 17, 2024
auto-submit bot pushed a commit that referenced this pull request Oct 22, 2024
**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
M97Chahboun pushed a commit to M97Chahboun/flutter that referenced this pull request Oct 30, 2024
**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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants