Roll Switch.adaptive changes into CupertinoSwitch
#149465
Conversation
github-actions
bot
added
framework
victorsanni
changed the title
Switch.adaptive changes into CupertinoSwitch
If these properties did not exist before, is this a good opportunity to introduce them with WidgetStateProperties? The migration from MSP --> WSP was motivated by extending the concept to Cupertino. Should we be adopting that as we are revamping Cupertino widgets? Same for Separate question, some folks have asked - what do we do about the reimplemented iOS Switch in the material library (behind Switch.adaptive)? That is the only (I think?) existing implementation of an iOS widget in material, rather than .adaptive returning a Cupertino variant. We could decide to go back to Switch.adaptive and have it return CupertinoSwitch again while still using theme adaptations to help style, or maybe we will implement more material.adaptive widgets like Switch later. We're waiting on the results of a user study (just a couple of weeks away) to help make these decisions. I am curious though, @MitchellGoodwin @justinmc what are your thoughts here? |
|
On widget state properties in general, I think we should use them if there's multiple styling variations for a property. If the Cupertino version (according to native, not necessarily how we have it now) is always the same value, then in my opinion it's ok to not use widget state properties. On the adaptive Switch in the Material library, I think eventually we need to change it to align with everything else we are doing (or change everything else to match it). For now though I think it's a low priority until after the study, and we can keep it around. It's a good example case of that approach. |
@MitchellGoodwin I love this way of determining if we should use WidgetStateProperty or not. |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
As others were discussing above, I agree with keeping the Material iOS implementation around until we've seen the user study and made a clear plan about how to handle this stuff.
Do you plan to make Switch.adaptive point to CupertinoSwitch in a subsequent PR?
Is it not practical to abstract any of this to a shared RawSwitch for sharing code? Just making sure we've thought about that. I'm guessing that a lot of the tests you added are mostly duplicated in Material, but they maybe could be deduplicated if there was a RawSwitch.
| /// Paints an iOS-style slider thumb. | ||
| /// | ||
| /// Used by [CupertinoSwitch] and [CupertinoSlider]. | ||
| /// Used by [CupertinoSlider]. |
There was a problem hiding this comment.
Why not use CupertinoThumbPainter? Is it out of date?
There was a problem hiding this comment.
CupertinoThumbPainter doesn't paint an image/icon on the thumb. I see some duplicated code, especially in the _paintCupertinoThumbShadowAndBorder method as compared to CupertinoThumbPainter's paint method. I thought it would be a challenge to refactor CupertinoThumbPainter to use this new implementation, so I just decided to not use it for now.
| ..rrect(color: const Color(0xffffffff)), | ||
| ); | ||
| }); | ||
| }, variant: TargetPlatformVariant.only(TargetPlatform.macOS)); |
There was a problem hiding this comment.
Why is this Mac only now?
There was a problem hiding this comment.
I had focus and focus tests only on macOS, I am not sure if that is good or bad (or what the consensus/best practice is).
There was a problem hiding this comment.
I have removed the macOS requirement for focus.
QuncCccccc
left a comment
QuncCccccc
left a comment
There was a problem hiding this comment.
Just left one more question. Otherwise, overall this looks good to me:)!
| if (reaction.isCompleted) { | ||
| // This happens when the thumb is dragged instead of being tapped. | ||
| _pressedInactiveThumbRadius = lerpDouble(inactiveThumbRadius, pressedThumbRadius, reaction.value); | ||
| _pressedActiveThumbRadius = lerpDouble(activeThumbRadius, pressedThumbRadius, reaction.value); | ||
| } | ||
| if (currentValue == 0) { | ||
| _pressedInactiveThumbRadius = lerpDouble(inactiveThumbRadius, pressedThumbRadius, reaction.value); | ||
| _pressedActiveThumbRadius = activeThumbRadius; | ||
| } | ||
| if (currentValue == 1) { | ||
| _pressedActiveThumbRadius = lerpDouble(activeThumbRadius, pressedThumbRadius, reaction.value); | ||
| _pressedInactiveThumbRadius = inactiveThumbRadius; | ||
| } |
There was a problem hiding this comment.
Just one more question(sorry), will we move this _SwitchPainter class in base widget so material and cupertino can use for both Switch? The reason I'm asking is that this block of code is for the pressing behavior in Material Switch. The thumb of the material switch has several different radius in different states, like when we press material switch, the thumb becomes larger, and inactive radius is also different from the active radius. But for Cupertino switch, seems we only need to care about the thumb extension which is handled below.
If we only want to focus on Cupertino here for now, maybe we can just remove these material-related code to make the implementation simpler. But if we eventually want to move this part to a base widget which needs to include both behavior, then probably we can keep the same implementation here.
There was a problem hiding this comment.
I'm not sure to be honest. Although for this PR I think we should remove it anyways just so there isn't dead code floating around here. If we need to add it back it should be trivial (I think).
There was a problem hiding this comment.
Sounds good! Then I can take another look to double check whether we have some more extra things to remove! (I'll need to double check but I think activeThumbRadius and inactiveThumbRadius may also be redundant because CupertinoSwitch should just have the same radius when it's on and off, these two properties are also for the M3 switch)
There was a problem hiding this comment.
I think they are redundant too. I removed them, along with those other material-related code.
MitchellGoodwin
left a comment
MitchellGoodwin
left a comment
There was a problem hiding this comment.
A small nit and a question. Otherwise, this looks good to me.
| }); | ||
|
|
||
| /// Creates an object that paints an iOS-style switch thumb. | ||
| const CupertinoThumbPainter.switchThumb({ |
There was a problem hiding this comment.
Should we deprecate this? I wouldn't be surprised if nobody uses it, but it's technically a public API.
There was a problem hiding this comment.
That's true. It's not used anywhere else in the framework at least, but this would not be the way to deprecate it anyways.
QuncCccccc
left a comment
QuncCccccc
left a comment
There was a problem hiding this comment.
LGTM, assuming we only focus on having matched properties in material Switch and cupertino Switch in this PR:)!
victorsanni
added
the
autosubmit
Roll Flutter from 5103d75 to 58068d8 (42 revisions) flutter/flutter@5103d75...58068d8 2024-07-12 [email protected] Reland: Move all Linux Moto G4 tests to mokey in staging (flutter/flutter#151654) 2024-07-12 [email protected] Update obsolete comment in InputDecorator test (flutter/flutter#151651) 2024-07-12 [email protected] Fix `TabBar` tab indicator stretch effect (flutter/flutter#150868) 2024-07-12 [email protected] Remove workaround for a bug in dart2wasm (flutter/flutter#151603) 2024-07-12 [email protected] [native_assets] Stop running link hooks in JIT mode (flutter/flutter#151534) 2024-07-12 [email protected] Roll `Switch.adaptive` changes into `CupertinoSwitch` (flutter/flutter#149465) 2024-07-11 [email protected] Unmark java11 tests as bringup:true (flutter/flutter#151612) 2024-07-11 [email protected] Add link to design document archive (flutter/flutter#151489) 2024-07-11 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Move all Linux Moto G4 tests to mokey in staging (#151608)" (flutter/flutter#151620) 2024-07-11 [email protected] Move all Linux Moto G4 tests to mokey in staging (flutter/flutter#151608) 2024-07-11 [email protected] docimports for API samples (flutter/flutter#151606) 2024-07-11 [email protected] docimports for flutter_goldens, flutter_localizations, flutter_web_plugins, fuchsia_remote_debug_protocol, integration_test (flutter/flutter#151271) 2024-07-11 [email protected] Re-enable debug canvaskit e2e tests. (flutter/flutter#151565) 2024-07-11 [email protected] Fix: Submenu anchor misaligned with child panel in web (Resolved #151081) (flutter/flutter#151294) 2024-07-11 [email protected] Replaced {@tool snippet} with {@tool dartpad} in CupertinoTabController (flutter/flutter#151272) 2024-07-11 [email protected] feat: Support overriding native endorsed plugins (flutter/flutter#137040) 2024-07-11 [email protected] expose keyboardType in DropdownMenu #150894 (flutter/flutter#150896) 2024-07-11 [email protected] docimports for flutter_driver (flutter/flutter#151267) 2024-07-11 [email protected] Add `TimeOfDay` comparison methods (flutter/flutter#151233) 2024-07-11 [email protected] Roll Flutter Engine from 6534fbf3c2c1 to 36dccf7bb25c (2 revisions) (flutter/flutter#151577) 2024-07-11 [email protected] Roll Flutter Engine from 1c23c8f64076 to 6534fbf3c2c1 (3 revisions) (flutter/flutter#151572) 2024-07-11 [email protected] Use correct locale for `CupertinoDatePicker` weekday (flutter/flutter#151494) 2024-07-10 [email protected] doc imports for enum values (flutter/flutter#151548) 2024-07-10 [email protected] Reland "Upgrade template Gradle, App AGP, Module AGP, and Kotlin versions, and tests"... but no longer upgrade module AGP version (flutter/flutter#151433) 2024-07-10 [email protected] Roll Packages from 14341d1 to ea35fc6 (16 revisions) (flutter/flutter#151556) 2024-07-10 [email protected] [CupertinoActionSheet] Fix padding and font size of buttons (flutter/flutter#151199) 2024-07-10 [email protected] Roll Flutter Engine from db2b45bea2c0 to 1c23c8f64076 (2 revisions) (flutter/flutter#151550) 2024-07-10 [email protected] Add docImports for flutter_test references (flutter/flutter#151175) 2024-07-10 [email protected] Mention not @-mentioning people in commit messages in tree hygiene (flutter/flutter#151487) 2024-07-10 [email protected] Roll Flutter Engine from 371db85f33d7 to db2b45bea2c0 (8 revisions) (flutter/flutter#151522) 2024-07-10 [email protected] fix heading level absorption, diagnostics; add tests and an a11y use-case (flutter/flutter#151421) 2024-07-10 [email protected] Roll Flutter Engine from 9d943eb2b37a to 371db85f33d7 (3 revisions) (flutter/flutter#151505) 2024-07-10 [email protected] Roll Flutter Engine from d3269d5855a7 to 9d943eb2b37a (5 revisions) (flutter/flutter#151495) 2024-07-09 [email protected] Update doc of `SemanticsProperties.identifier` (flutter/flutter#149915) 2024-07-09 [email protected] Clean up leaky test. (flutter/flutter#151131) 2024-07-09 [email protected] Roll pub packages (flutter/flutter#151492) 2024-07-09 [email protected] testAdd tests for stepper.controls_builder.0.dart (flutter/flutter#150669) 2024-07-09 [email protected] Add Semantics Property `linkUrl` (flutter/flutter#150639) 2024-07-09 [email protected] Roll Flutter Engine from 4a2ac0e51a8f to d3269d5855a7 (1 revision) (flutter/flutter#151488) 2024-07-09 [email protected] Link engine docs on AS setup in flutter/flutter docs on engine contributor setup (flutter/flutter#151481) 2024-07-09 [email protected] Roll Flutter Engine from 69075e7e87d4 to 4a2ac0e51a8f (21 revisions) (flutter/flutter#151482) 2024-07-09 [email protected] Fix Material 3 `Dialog` default background color (flutter/flutter#151400) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages ...
5 participants
Brings over the changes from
Switch.adaptiveintoCupertinoSwitch.This change adds the following
Switchparameters toCupertinoSwitch:inactiveThumbColor,activeThumbImage,onActiveThumbImageError,inactiveThumbImage,onInactiveThumbImageError,trackOutlineColor,trackOutlineWidth,thumbIcon,mouseCursor.The following
Switchparameters are ignored:activeTrackColor: becauseactiveColorhas the same function.inactiveTrackColor: becausetrackColorhas the same function.materialTapTargetSize: because it is only applicable inMaterial.hoverColor,overlayColor,splashRadius: because these parameters configure the radial reaction overlay inMaterial, so are not applicable here.The following
CupertinoSwitchparameters which are absent fromSwitch.adaptiveare retained:onLabelColor,offLabelColor,applyThemetrackColorandthumbColorare of typeWidgetStatePropertyin MaterialSwitch, but are currently of typeColorinCupertinoSwitch. For backwards compatibility, both parameters are kept asColors, but can be resolved in differentWidgetStates usingWidgetStateColor.resolve().This PR does not apply any fidelity updates to
CupertinoSwitch.Fixes #149275
Related PRs: #130425 #148804
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.