[beta] Cherry pick https://github.com/flutter/flutter/pull/181074#181367
[beta] Cherry pick https://github.com/flutter/flutter/pull/181074#181367auto-submit[bot] merged 1 commit intoflutter:flutter-3.41-candidate.0from
Conversation
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
github-actions
bot
added
framework
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a cherry-pick to fix a regression preventing the use of nullable types with DropdownMenu and DropdownMenuFormField. The changes correctly remove the extends Object constraint on the generic type parameter T for these widgets and their corresponding states. New regression tests have been added to ensure that nullable types, including selecting a null value, work as expected. The changes are sound and directly address the issue. I have one minor suggestion to improve code quality in the tests by reducing some duplication.
| Finder findMenuItemButton(String label) { | ||
| // For each menu items there are two MenuItemButton widgets. | ||
| // The last one is the real button item in the menu. | ||
| // The first one is not visible, it is part of _DropdownMenuBody | ||
| // which is used to compute the dropdown width. | ||
| return find.widgetWithText(MenuItemButton, label).last; | ||
| } |
There was a problem hiding this comment.
This new helper function findMenuItemButton is very similar to the existing findMenuItem function in this file. To avoid code duplication and improve maintainability, consider refactoring findMenuItem to call this new, more generic function.
For example, you could change findMenuItem to:
Finder findMenuItem(MenuItem menuItem) {
return findMenuItemButton(menuItem.label);
}This would also allow you to remove the duplicated comment from findMenuItem.
There was a problem hiding this comment.
I originally thought about this, but decided to keep both methods to not need a refactor in a "revert" PR and keep the method in sync with the conventions in dropdown_menu_test.dart. We could do a follow-up refactor, if desired.
LongCatIsLooong
left a comment
LongCatIsLooong
left a comment
There was a problem hiding this comment.
The revert LGTM. But I don't think the original commit has reached stable: 0205d4e. So can you confirm that this is a CP to beta?
| await tester.tap(findMenuItemButton(selectNoneLabel)); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(controller.text, selectNoneLabel); |
There was a problem hiding this comment.
Huh I thought the text should be empty or something since the widget uses null for "no selection"?
There was a problem hiding this comment.
What exactly is the difference? Only the label? Is that something we can integrate in #181369, but still merge this cherry pick?
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍 but I'm unsure about @LongCatIsLooong's comment https://github.com/flutter/flutter/pull/181367/files#r2733486116.
dkwingsmt
changed the title
eyebrowsoffire
added
the
autosubmit
auto-submit
bot
merged commit f3c54a6
into
flutter:flutter-3.41-candidate.0
Resolves #181367 (comment) The test was still valid as `DropdownMenuFormField` includes `DropdownMenu` and therefore could select the desired MenuItemButton. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Resolves flutter#181367 (comment) The test was still valid as `DropdownMenuFormField` includes `DropdownMenu` and therefore could select the desired MenuItemButton. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
5 participants
This PR cherrypicks #181074 into beta, which is mostly a revert of #176711 (except that some docs are left unreverted and a few new tests are added.) This is needed to address a breakage as described in #180121.
Impacted Users: Apps that use
DropdownMenuorDropdownMenuFormFieldfor a nullable class, such asDropdownMenu<int?>.Impact Description: Can not compile.
Workaround: Yes, through non-trivial changes. For example, the developer can replace
DropdownMenu<int?>withDropdownMenu<Maybe<int>>, as long as they importMaybeor create their own equivalence.Risk: Low. This PR is basically a revert.
Test Coverage: Yes. The widget has high test coverage.
Validation Steps: The PR compiles and tests pass.