Skip to content

Monomorphic Mapper - #154972

Merged
auto-submit[bot] merged 1 commit into
flutter:masterfrom
nate-thegrate:monomorphic-mapper
Oct 16, 2024
Merged

auto-submit[bot] merged 1 commit into
flutter:masterfrom
nate-thegrate:monomorphic-mapper

Conversation

@nate-thegrate

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

Copy link
Copy Markdown
Contributor

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. p: material_ui material_ui package in flutter/packages 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
Comment thread packages/flutter/test/widgets/widget_state_property_test.dart
Comment thread packages/flutter/test/widgets/widget_state_property_test.dart
Comment thread packages/flutter/lib/src/material/material_state.dart
@nate-thegrate
nate-thegrate force-pushed the monomorphic-mapper branch 2 times, most recently from efa6ded to c598bf4 Compare September 26, 2024 01:46

@MitchellGoodwin MitchellGoodwin left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread packages/flutter/lib/src/material/material_state.dart Outdated
Comment thread packages/flutter/lib/src/material/material_state.dart Outdated
Comment thread packages/flutter/lib/src/material/material_state.dart Outdated

@MitchellGoodwin MitchellGoodwin left a comment

Copy link
Copy Markdown
Contributor

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

@nate-thegrate nate-thegrate left a comment •

Copy link
Copy Markdown
Contributor Author

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 🙏

Comment thread packages/flutter/lib/src/material/material_state.dart Outdated
Comment thread packages/flutter/lib/src/material/material_state.dart Outdated
@github-actions github-actions Bot removed p: material_ui material_ui package in flutter/packages 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
Copy Markdown
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.

@MitchellGoodwin MitchellGoodwin left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@nate-thegrate nate-thegrate mentioned this pull request Nov 28, 2024
9 tasks done
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
chunhtai pushed a commit to chunhtai/packages that referenced this pull request Jun 12, 2026
**Changes**
- Add `WidgetStateInputBorder` class, with `.resolveWith()` and `.fromMap()` constructors
- Deprecate `MaterialStateOutlineInputBorder` and `MaterialStateUnderlineInputBorder` and provide data-driven fixes

<br>

**Other changes** based on flutter/flutter#154972 (review)
- Fix documentation copy-paste typo ("OutlinedBorder" � "InputBorder")
- Add test to ensure borders are painted correctly
- Add DartPad sample & relevant test
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