Move Feedback to widgets layer
#148523
Move Feedback to widgets layer
#148523
Conversation
github-actions
bot
added
a: text input
| @@ -167,6 +168,4 @@ abstract final class Feedback { | |||
| callback(); | |||
| }; | |||
| } | |||
|
|
|||
| static TargetPlatform _platform(BuildContext context) => Theme.of(context).platform; | |||
There was a problem hiding this comment.
This makes me wonder what we think the difference should be between defaultTargetPlatform and ThemeData.platform. Notice that this change could be breaking if any users are changing their theme platform but not their defaultTargetPlatform. This is probably another sign that we need some kind of theming at the widgets library level.
defaultTargetPlatform can only be changed with debugDefaultTargetPlatformOverride, which doesn't work in production builds. So by using defaultTargetPlatform we're saying that users can't change the feature, it must match the real platform. By using ThemeData.platform we're saying that it's valid for users to prefer the value of some other platform and use that in production instead of the platform default. Is that how we draw the line?
There was a problem hiding this comment.
Yeah, we chatted about this a bit yesterday.
I also thought it might be breaking to not defer to the theme, but then I realized - oh wait. Feedback is device-specific feedback actions like haptic buzzes, so it does not really apply to whatever the ThemeData says the platform is.
For example, if I am on a desktop machine, and Theme.of(context).platform has been configured to Android, the desktop won't actually produce haptic feedback. Currently Feedback only has implemented Android feedback options as well, so for these reasons it seemed like a safe change, and one that made the feature a bit more logical.. or at least less prone to Feedback that does not make sense for the actual device that is being used.
WDYT @justinmc?
This is probably another sign that we need some kind of theming at the widgets library level.
I agree with this, I don't know if it applies here, but I am excited we are exploring this more and more. :)
There was a problem hiding this comment.
Proposal: Use ThemeData.platform if you can, unless it's something that affects platform calls, in which case use defaultTargetPlatform. So deciding which context menu to show should be a theme thing, but deciding to call the platform to show the native iOS context menu should use defaultTargetPlatform.
Currently it's not possible to use Theme in the widgets library, so I would also propose that long-term we should have a unified theme in the widgets library that Material and Cupertino derive their themes from.
In the case of this PR, following the proposal I would say that the question of which haptic vibration to use should be controlled by the theme. Since that's not yet possible, use defaultTargetPlatform in the meantime.
So according to the proposal, this PR is correct, with the caveat that we'd like to migrate this to use some kind of widget theme in the future. That's another reason to keep the BuildContext parameters, as you have done. In the future we'll use that to get the theme.
There was a problem hiding this comment.
@Piinks Sorry, my previous comment was written without seeing your reply, I went to lunch and didn't refresh the page!
There was a problem hiding this comment.
Yeah actually this PR would affect platform channel calls, because HapticFeedback directly calls platform channel methods to cause vibration. But it still feels to me like setting the Theme to iOS while on Android in order to change the vibration is a valid thing to do. Maybe I need to rephrase my proposal...
flutter/flutter@00425ef...adf279f 2024-05-18 nate.w5687@gmail.com Fix template manifest test (flutter/flutter#148616) 2024-05-18 jason-simmons@users.noreply.github.com Disable shuffling in the flutter_tools create_test suite (flutter/flutter#148619) 2024-05-18 victorsanniay@gmail.com Move `Feedback` to widgets layer (flutter/flutter#148523) 2024-05-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 93f1b5a146ed to 552a965b707b (4 revisions) (flutter/flutter#148595) 2024-05-18 andrewrkolos@gmail.com Swap crash reporting with unified analytics (flutter/flutter#148525) 2024-05-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5b3bf9a950b2 to 93f1b5a146ed (3 revisions) (flutter/flutter#148581) 2024-05-17 katelovett@google.com Migrate the flutter/flutter wiki to docs/unsorted_wiki (flutter/flutter#148562) 2024-05-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 17decdf12557 to 5b3bf9a950b2 (3 revisions) (flutter/flutter#148567) 2024-05-17 zanderso@users.noreply.github.com Shift a test on MotoG4 to staging, add it on Mokey (flutter/flutter#148552) 2024-05-17 zanderso@users.noreply.github.com Revert "`if` chains � `switch` expressions" (flutter/flutter#148556) 2024-05-17 engine-flutter-autoroll@skia.org Roll Packages from 87a02e3 to ae4dd32 (9 revisions) (flutter/flutter#148555) 2024-05-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7d244ab0348a to 17decdf12557 (1 revision) (flutter/flutter#148549) 2024-05-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from a19d3722922d to 7d244ab0348a (4 revisions) (flutter/flutter#148546) 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 Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r#6761) flutter/flutter@00425ef...adf279f 2024-05-18 nate.w5687@gmail.com Fix template manifest test (flutter/flutter#148616) 2024-05-18 jason-simmons@users.noreply.github.com Disable shuffling in the flutter_tools create_test suite (flutter/flutter#148619) 2024-05-18 victorsanniay@gmail.com Move `Feedback` to widgets layer (flutter/flutter#148523) 2024-05-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 93f1b5a146ed to 552a965b707b (4 revisions) (flutter/flutter#148595) 2024-05-18 andrewrkolos@gmail.com Swap crash reporting with unified analytics (flutter/flutter#148525) 2024-05-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5b3bf9a950b2 to 93f1b5a146ed (3 revisions) (flutter/flutter#148581) 2024-05-17 katelovett@google.com Migrate the flutter/flutter wiki to docs/unsorted_wiki (flutter/flutter#148562) 2024-05-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 17decdf12557 to 5b3bf9a950b2 (3 revisions) (flutter/flutter#148567) 2024-05-17 zanderso@users.noreply.github.com Shift a test on MotoG4 to staging, add it on Mokey (flutter/flutter#148552) 2024-05-17 zanderso@users.noreply.github.com Revert "`if` chains â�� `switch` expressions" (flutter/flutter#148556) 2024-05-17 engine-flutter-autoroll@skia.org Roll Packages from 87a02e3 to ae4dd32 (9 revisions) (flutter/flutter#148555) 2024-05-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7d244ab0348a to 17decdf12557 (1 revision) (flutter/flutter#148549) 2024-05-17 engine-flutter-autoroll@skia.org Roll Flutter Engine from a19d3722922d to 7d244ab0348a (4 revisions) (flutter/flutter#148546) 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 Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Currently,
Feedbackexists in the Material layer. However, not only isFeedbacknot material-opinionated, but it is an abstract class that defines its functionality depending on the user's platform.It makes sense that
Feedbackshould exist in the widgets layer instead. This makes it easier to incorporate platform specificFeedbackupdates as they arrive, fixing issues like #148391.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.