Fix: The enableFeedback property of InkWell cannot be set to a nullab… #158907
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
@StanleyCocos |
There was a problem hiding this comment.
Looks great overall, but @TahaTesser is right—this PR will need either a test or a test exemption. (One could argue that there are virtually no semantic changes here, but in my experience, requesting a test exemption for a borderline case hasn't succeeded.)
One approach could be to keep the bool? enableFeedback = true for now and add an assert; then test whether the assert fires (perhaps this isn't the best idea, since we'd have to ignore tighten_type_of_initializing_formals), and then a while after it lands in stable we could get back in here and finish the job.
|
Yes, you're right. However, as a developer, I believe that if someone explicitly calls InkWell(enableFeedback: null), it is an error. For developers, setting it to null likely implies an intention to set it to false, but it will actually default to true. Therefore, we think it would be more intuitive to notify them that there's an error here. Using assert(enableFeedback != null) is a runtime reminder, but developers may not immediately notice this issue. For developers who set enableFeedback != null, this change would not affect them. |
|
Fortunately, this PR isn't a breaking change per our Tree Hygiene rules, since all checks are passing. In my personal opinion, I'd be happy to see this merged right away. However, the bot comment above regarding tests needs to be resolved first. This seems to me like it could qualify as a "code refactor with no semantic changes"—feel free to head over to Discord (specifically the |
|
@nate-thegrate If you could help me resolve the robot comments above, it would be greatly appreciated. Thank you! |
|
Sounds good! I should have an update for you within the next few days :) |
|
I'm very happy to see this outcome. Thank you |
|
test-exempt: code refactor with no semantic change |
Interestingly, at the time this file was migrated to null-safety and the /// The [mouseCursor], [enableFeedback], and [excludeFromSemantics] arguments
/// must not be null. |
|
Interesting, I guess this must have slipped under the radar! @StanleyCocos we'll be able to merge this pull request once it has 2 approvals. |
bleroux
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the cleanup. 🙏
Roll Flutter from 8536b96 to 93d772c (37 revisions) flutter/flutter@8536b96...93d772c 2024-11-21 bkonyi@google.com Added additional logging to `_listCoreDevices` (flutter/flutter#159275) 2024-11-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 78b87f3fe023 to d1a08064e193 (1 revision) (flutter/flutter#159280) 2024-11-21 jason-simmons@users.noreply.github.com Shut down DevTools and DDS processes if flutter_tools is killed by a signal (flutter/flutter#159238) 2024-11-21 matanlurey@users.noreply.github.com Remove `RepaintBoundary` that is no longer needed. (flutter/flutter#159232) 2024-11-21 matanlurey@users.noreply.github.com Try a speculative fix for Gradle OOMs. (flutter/flutter#159234) 2024-11-21 21270878+elliette@users.noreply.github.com On-device Widget Inspector button exits widget selection (flutter/flutter#158219) 2024-11-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 523d381893c8 to 78b87f3fe023 (2 revisions) (flutter/flutter#159270) 2024-11-21 jessiewong401@gmail.com Remove `firebase_abstract_method_smoke_test` (flutter/flutter#159145) 2024-11-21 engine-flutter-autoroll@skia.org Roll Packages from e95f6d8 to 913b99e (7 revisions) (flutter/flutter#159268) 2024-11-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 69c325513a65 to 523d381893c8 (3 revisions) (flutter/flutter#159263) 2024-11-21 jonahwilliams@google.com [flutter_tools] opt iOS/macOS apps out of Metal API validation via migrator, update templates in repo. (flutter/flutter#159228) 2024-11-21 jmccandless@google.com Scribe Android handwriting text input (flutter/flutter#148784) 2024-11-21 matanlurey@users.noreply.github.com Terminate non-detached test devices on `flutter run` completion (flutter/flutter#159170) 2024-11-21 matanlurey@users.noreply.github.com Un-skip tests that use `flutter build apk`. (flutter/flutter#159231) 2024-11-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2d32cf3a7971 to 69c325513a65 (1 revision) (flutter/flutter#159229) 2024-11-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3828681d1f86 to 2d32cf3a7971 (3 revisions) (flutter/flutter#159226) 2024-11-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3245c8976976 to 3828681d1f86 (1 revision) (flutter/flutter#159217) 2024-11-20 matanlurey@users.noreply.github.com Add `--dry-run` to `dev/bots/test.dart`. (flutter/flutter#158956) 2024-11-20 34871572+gmackall@users.noreply.github.com Add docs for setting up Android Studio to auto format Kotlin code (flutter/flutter#159209) 2024-11-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 80d77505fdde to 3245c8976976 (1 revision) (flutter/flutter#159214) 2024-11-20 stanleycocoa@gmail.com Fix: The enableFeedback property of InkWell cannot be set to a nullab� (flutter/flutter#158907) 2024-11-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3f19207e820e to 80d77505fdde (1 revision) (flutter/flutter#159210) 2024-11-20 engine-flutter-autoroll@skia.org Roll Packages from fc4adc7 to e95f6d8 (6 revisions) (flutter/flutter#159201) 2024-11-20 kustermann@google.com Remove dependency on [Target] and instead operate on [Architecture] (flutter/flutter#159196) 2024-11-20 63278936+hongeSunCoder@users.noreply.github.com Fix git command in Quality-Assurance.md (flutter/flutter#155146) 2024-11-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7eb87547cbc6 to 3f19207e820e (4 revisions) (flutter/flutter#159176) 2024-11-20 matanlurey@users.noreply.github.com Make `runner` non-nullable as it always is. (flutter/flutter#159156) 2024-11-19 tessertaha@gmail.com Update Material 3 `CircularProgressIndicator` for new visual style (flutter/flutter#158104) 2024-11-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4ff696b555dc to 7eb87547cbc6 (3 revisions) (flutter/flutter#159168) 2024-11-19 magder@google.com Add platform-android label for all flutter_tools *android* files (flutter/flutter#159166) 2024-11-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from d5820a638885 to 4ff696b555dc (1 revision) (flutter/flutter#159164) 2024-11-19 bernaferrari2@gmail.com Reland Add UI Benchmarks (flutter/flutter#153368) 2024-11-19 mohellebiabdessalem@gmail.com fix lint usage of `task` inside `resolve_dependecies.gradle` file (flutter/flutter#158022) 2024-11-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from cff1e751f853 to d5820a638885 (5 revisions) (flutter/flutter#159155) 2024-11-19 mohellebiabdessalem@gmail.com Removing redundant backticks in `flutter\packages\flutter_tools\gradle\gradle.kts` (flutter/flutter#159051) 2024-11-19 50643541+Mairramer@users.noreply.github.com Fixes initial validation with AutovalidateMode.always on first build (flutter/flutter#156708) 2024-11-19 tessertaha@gmail.com Introduce new Material 3 `Slider` shapes (flutter/flutter#152237) 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 louisehsu@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 ...
I found that enableFeedback can be set to null, but in actual use, setting it to null has no effect and is overwritten by true. I think this is an issue.
Call Error