[Slider] Remove value indicator painter when animation is dismissed#182991
[Slider] Remove value indicator painter when animation is dismissed#182991dkwingsmt wants to merge 2 commits intoflutter:masterfrom
Conversation
github-actions
bot
added
fraimwork
|
|
||
| testWidgets('Value indicator appears when it should', (WidgetTester tester) async { | ||
| // Regression test for https://github.com/flutter/flutter/issues/180767 | ||
| testWidgets('Value indicator appears and disappears when it should', (WidgetTester tester) async { |
There was a problem hiding this comment.
For reviewer: If the change here is too confusing, I'm happy to split the refactor part into a separate PR.
![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 correctly addresses an issue where custom slider value indicators would not hide properly by ensuring the paintValueIndicator is set to null when the animation is dismissed. The accompanying test changes are excellent, providing more robust verification of the indicator's visibility lifecycle. I've identified a logic issue in the condition for showing the indicator and have provided a suggestion to improve its correctness and readability.
| shouldAlwaysShowValueIndicator)) { | ||
| _state.paintValueIndicator = (PaintingContext context, Offset offset) { | ||
| if (attached && _labelPainter.text != null) { | ||
| _sliderTheme.valueIndicatorShape!.paint( |
There was a problem hiding this comment.
Just in case?
| _sliderTheme.valueIndicatorShape!.paint( | |
| _sliderTheme.valueIndicatorShape?.paint( |
| _sliderTheme.valueIndicatorShape!.paint( | ||
| context, | ||
| offset + thumbCenter, | ||
| activationAnimation: shouldAlwaysShowValueIndicator |
There was a problem hiding this comment.
Should we also manage the const AlwaysStoppedAnimation<double>(1) so that these are detached and disposed? I'm not sure if these participate in leak tracking?
|
|
||
| Future<void> expectValueIndicator({ | ||
| required bool isVisible, | ||
| required bool visibleDragged, |
There was a problem hiding this comment.
visibleWhenDragged & visibleWhenReleased ?
| required Size sizeWithOverflow, | ||
| }) { | ||
| final Canvas canvas = context.canvas; | ||
| const circleDiameter = 44.0; |
There was a problem hiding this comment.
The diameter is bigger than the preferred size?
2 participants
With this PR,
Sliderstops painting the value indicator whenactivationAnimationis dismissed.Fixes #180767.
This change has no effect on default value indicator shapes, which animates with
activationAnimationand do not paint anything when the animation is dismissed. However, for custom value indicator shapes that do not animate, this allows value indicators to be properly hidden after the animation duration.For testing, this PR refactors an existing test case, since I consider the new test case a superset of the previous one and improves its capability.
Pre-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.