Migrate to widget state #174746
Migrate to widget state #174746
Conversation
github-actions
bot
added
a: text input
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request continues the migration from MaterialState to WidgetState across various material widgets and their tests. The changes are mostly a direct replacement, which is correct. However, I've identified a few instances where mutable Sets of states are being modified directly, leading to potential bugs in widget rendering. I've provided suggestions to fix these by creating copies of the state sets before modification.
| final Set<WidgetState> activeStates = widget.toggleableState.states..add(WidgetState.selected); | ||
| final Set<WidgetState> inactiveStates = widget.toggleableState.states | ||
| ..remove(WidgetState.selected); |
There was a problem hiding this comment.
This code mutates widget.toggleableState.states directly, which can lead to unpredictable behavior as activeStates and inactiveStates will end up pointing to the same modified set. You should create copies of the state set before modifying them to ensure activeStates and inactiveStates are distinct and correctly represent the intended states. The ..remove(WidgetState.selected) is only necessary because of the mutation and can be removed when creating a fresh copy for inactiveStates.
final Set<WidgetState> activeStates =
widget.toggleableState.states.toSet()..add(WidgetState.selected);
final Set<WidgetState> inactiveStates = widget.toggleableState.states.toSet();| final Set<WidgetState> activeStates = states..add(WidgetState.selected); | ||
| final Set<WidgetState> inactiveStates = states..remove(WidgetState.selected); |
There was a problem hiding this comment.
This code mutates the local states set directly. Both activeStates and inactiveStates will point to the same set object, which is modified in place. This leads to both variables holding the same final state of the set, which is likely not the intention. You should create copies to ensure they are independent.
final Set<WidgetState> activeStates = states.toSet()..add(WidgetState.selected);
final Set<WidgetState> inactiveStates = states.toSet()..remove(WidgetState.selected);| ) { | ||
| final Set<MaterialState> effectiveStates = selectedState..addAll(states); | ||
| final Set<WidgetState> effectiveStates = selectedState..addAll(states); |
There was a problem hiding this comment.
The selectedState set is being mutated here with addAll. Since this resolver can be called multiple times, this will lead to selectedState accumulating states from previous calls, which is incorrect. You should create a new set for effectiveStates by creating a copy of selectedState before adding the other states.
final Set<WidgetState> effectiveStates = selectedState.toSet()..addAll(states);
|
Do we want me to look at gemini's comments? I feel like it would be safer to keep this PR scoped to using |
Follow up of #174486
Migrate some files from
MaterialStatetoWidgetState. This PR only focus onWidgetStatefor a subset of files.packages/flutter/lib/src/material/material_state.dartinto other PRsPre-launch Checklist
//github.com/).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.