Repository navigation
Toggleable Refactor - #76745
Toggleable Refactor#76745
Conversation
flutter-dashboard
Bot
added
p: material_ui
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
This is a nice refactoring. LGTM
There was a problem hiding this comment.
I think it would be fair to qualify this class as material-specific. The animation controllers and their curves work for the material toggle components (but not ToggleButtons I assume) and I don't think developers should expect that we'd lift every opinion in this class into a parameter. In other words, I think the API doc should make it clear that this is something we've factored out of several widgets, not the one and only basis for all toggle buttons.
There was a problem hiding this comment.
Maybe explain that it's the first pointer-down or the most recent one?
There was a problem hiding this comment.
Should this be null if isInteractive is false?
There was a problem hiding this comment.
Added a note that it is null if isInteractive is false.
There was a problem hiding this comment.
Is there a redundant setState() here if the user double-taps? Not sure if this is necessary:
if (isInteractive && downPosition != details.localPosition) {There was a problem hiding this comment.
If the users double taps, the first tap would null out _downPosition in _handleTapEnd - so we have to set it again here for the second tap.
There was a problem hiding this comment.
Only call setState if _downPosition is non-null?
There was a problem hiding this comment.
Maybe: "Typically wraps a painter"
This reverts commit 14552a9.
|
Removing RenderToggleable breaks packages like circular_check_box. |
|
I've upgraded from 2.0.4 to 2.2.0. This was a breaking change for me as I was using I have no easy migration path for this, other than picking apart the PR and figuring out how to use |
|
Flutter's breaking change policy (including when we consider a change to be breaking) is documented here: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes |
|
@goderbauer Thanks very much for the link. We normally encourage our developers to perform regular minor version upgrades for dependencies, as such upgrades rarely break public APIs. So I need to make sure I understand that policy in detail. When developing with Flutter, is it considered bad practice to extend a public class such as Again, thanks! We love Flutter, we just want to ensure we're using it as intended. |
I have the same issue, Do you have any solution for this? |
|
Your circular check box is similar to my custom checkbox, so it should be the same process for you. Your checkbox state class currently returns a Instead, add the You'll also need to clean up all your state handling to defer to the new mixin. This was a really lengthy process, so in the end it might have been easier to just copy the latest Material Checkbox and manually apply any custom rendering code to its painter. |
…oggleable in Flutter 2.1.0. See flutter/flutter#76745 for reference.
|
@goderbauer I'm wondering why wasn't it deprecated before being removed? This is a public API and per the breaking change policy it should have been deprecated first, no? |
7 participants
This started out as a simple fix for #76411, which was caused by RenderToggleable not disposing its GestureRecognizer. Unfortunately, the RenderObject doesn't have a point where it is appropriate to dispose the recognizer.
In this refactoring, I am removing the RenderToggleable class altogether. Instead, I am introducing a ToggleableStateMixin that toggleable widgets (like Switch, Checkbox, Radio) can mix into their state. The ToggleableStateMixin implements logic to trigger state transitions of the toggleable (e.g. when it is tapped). For this, it also manages a bunch of AnimationControllers that drive the transition.
ToggleableStateMixin has no opinion on the visual appearance of the toggleable widget. Users of the ToggleableStateMixin have to provide a CustomPainter, that draws the current visual appearance of the toggleable based on the Animations provided by the mixin.
The idea is that managing the lifecycle of various elements a toggleable needs (e.g. a GestureRecognizer) is easier in the widget layer (i.e. in the ToggleableStateMixin) than it is in the render layer (as it was implemented before in the RenderToggleable).
Prior to this change, concrete Toggleable implementations would subclass RenderToggleable and implement a LeafRenderObjectWidget as well as a StatefulWidget. With this change, they mixin the ToggleableStateMixin into their StatefulWidget and provide a CustomPainter that draws the visual appearance.
The ToggleableStateMixin also implements some common logic (e.g. the FocusableActionDetector) that was previously repeated in the various toggleable widgets.