Fix DropdownMenu entry selection with enableFilter
#174757
Conversation
|
I forgot to rebase with master first, my bad. |
There was a problem hiding this comment.
Code Review
This pull request primarily fixes an issue with DropdownMenu where entry selection was not persisted correctly when filtering was enabled. The fix correctly maps the index from the filtered list back to the original list of entries. The PR also includes a major refactoring to introduce a new debugging path for physical iOS devices using devicectl and lldb, which is a significant improvement for iOS development workflows. My review includes a couple of suggestions to improve maintainability by addressing some duplicated code in the new iOS debugging logic and to seek clarification on a removed regression test to prevent potential regressions.
e6c5176 to
1338607
Compare
1338607 to
010db58
Compare
|
@ahmedrasar I filed #174778 which fixes the same issue. |
|
@bleroux. I had gone through many iterations on this, and one of them was your suggestion. It does indeed improve readability and unlike storing the index it doesn't loop the entries to get the value. However it has two drawbacks. The first is that storing a I comared every possible soultion I can think of in this comment Your test is better than mine, though. |
|
Thanks for the clarification.
Feel free to use it. |
There was a problem hiding this comment.
I think the original issue(#155660) was when dropdownMenuEntries are changed, didUpdateWidget needs rematch. But here and previous PR, we just introduced extra logic to keep the behavior correct.
I think the most simple solution that I can think of is the comment here. It fixed the original issue and won't introduce regression. Please do consider that solution as I just feel the previous PR and this one added new logics in several places.
|
@QuncCccccc. Yes, #155660 was about initial selection not rematching. But the issue is present with any selection not just initial selection. Fixing the issue only for initial selection would be more bizarre IMO.
It would be more straightforward for sure, but for it to work as expected labels should be unique, otherwise it could cause mismatching. Also, an additional variable would be needed to counter the case of using an external controller. That was the reason for reverting #155757. A third option would be something like class _DropdownMenuState<T> {
late T _lastSleectedValue;
late bool _hasSelection;
}It would eliminate the frequent looping to get the value since it's already stored, with the trade off having to maintain an additional variable.
|
There was a problem hiding this comment.
Thanks a lot for your contribution.
It would be more straightforward for sure, but for it to work as expected labels should be unique, otherwise it could cause mismatching.
Could you provide an example for this? I don't think it doesn't work when the labels are the same. Here's the demo. The value 4 and 5 have the same label "Five" but the search highlight can find the correct "Five".
Screen.Recording.2025-09-24.at.6.54.19.PM.mov
However, with this PR and previous PR change, the demo shows a wrong search highlight (When we select the second "Five", next time when we open the menu, it should highlight the second "Five").
Screen.Recording.2025-09-24.at.7.01.14.PM.mov
Please do consider to revert the previous PR and apply the change I suggested(here). Please let me know if there are any concerns:)
I agree with this, it is always sad to have to revert previous work but this is usually the best option as it gives time to find a proper solution on master while the revert will be cherry-picked quickly. |
|
What I meant by 'it could cause mismatching' is that when two or more entries have the same label, the looping function ( Take this example Screencast_20250926_232502.mp4Although both green and blue have the same label (green) toggling localization wouldn't mismatch blue to green's label (since it's first in the last). This is because both have different values. If we would rematch based on the current textfield aganist the previous list's labels, blue (with green label) would map to verde (green in espanol). code sampleimport 'package:flutter/material.dart';
void main(List<String> args) {
runApp(MaterialApp(home: DropdownPage()));
}
class DropdownPage extends StatefulWidget {
const DropdownPage({super.key});
@override
_DropdownPageState createState() => _DropdownPageState();
}
class _DropdownPageState extends State<DropdownPage> {
var _selectedValue = Colors.white;
Localization _localization = EnglishLocalization();
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(title: Text('Dropdown Page')),
backgroundColor: _selectedValue,
body: Row(
mainAxisAlignment: MainAxisAlignment.center,
children: [
DropdownMenu(
initialSelection: _selectedValue,
onSelected: (Color? newValue) {
setState(() {
_selectedValue = newValue!;
});
},
dropdownMenuEntries: [
DropdownMenuEntry<Color>(
value: Colors.red,
label: _localization.redLabel,
),
DropdownMenuEntry<Color>(
value: Colors.green,
label: _localization.greenLabel,
),
DropdownMenuEntry<Color>(
value: Colors.blue,
label: _localization.blueLabel,
),
].toList(),
),
IconButton(
onPressed: () {
// Toggle between English and Spanish localization
setState(() {
_localization = _localization is EnglishLocalization
? EspanolLocalization()
: EnglishLocalization();
});
},
icon: Icon(Icons.language),
),
],
),
);
}
}
abstract class Localization {
String get redLabel;
String get greenLabel;
String get blueLabel;
}
class EnglishLocalization implements Localization {
@override
String get redLabel => 'Red';
@override
String get greenLabel => 'Green';
@override
String get blueLabel => 'Green';
}
class EspanolLocalization implements Localization {
@override
String get redLabel => 'Rojo';
@override
String get greenLabel => 'Verde';
@override
String get blueLabel => 'Azul';
}It's not really a big deal since it wouldn't make sense to use the same label anyway. I'll file a revert and apply the new changes asap. |
|
@QuncCccccc. Hi! I did some tests on what you said here on 3.35.2, 3.27.3, this PR, and master. And it's reproducible on all of them. It seems that the problem is not about highlighting the current entry but with searching and highlighting the first match of the search function. In this clicking the dropdown menu itself has no effect on the current entry is highlighted despite having the same label. The first match is highlighted when you press the textfield itself because it apply the search function. This test is on 3.27.3 before the previous PR. Screencast_20250928_165058.mp4codeimport 'package:flutter/material.dart';
void main() {
runApp(DropdownSameLabels());
}
class DropdownSameLabels extends StatelessWidget {
const DropdownSameLabels({super.key});
@override
Widget build(BuildContext context) {
return MaterialApp(
home: Scaffold(
appBar: AppBar(title: Text('Dropdown Menu with Same Labels')),
body: DropdownMenuExample(),
),
);
}
}
class DropdownMenuExample extends StatefulWidget {
const DropdownMenuExample({super.key});
@override
_DropdownMenuExampleState createState() => _DropdownMenuExampleState();
}
class _DropdownMenuExampleState extends State<DropdownMenuExample> {
int? _selectedItem;
@override
Widget build(BuildContext context) {
return Row(
mainAxisAlignment: MainAxisAlignment.center,
spacing: 10,
children: <Widget>[
Text('Selected Item: ${_selectedItem ?? 'None'}'),
DropdownMenu(
// requestFocusOnTap: false,
onSelected: (int? newValue) {
setState(() {
_selectedItem = newValue;
});
},
dropdownMenuEntries: const <DropdownMenuEntry<int>>[
DropdownMenuEntry(value: 1, label: 'One'),
DropdownMenuEntry(value: 2, label: 'Two'),
DropdownMenuEntry(value: 3, label: 'Three'),
DropdownMenuEntry(value: 4, label: 'Four'),
DropdownMenuEntry(value: 5, label: 'Five'),
DropdownMenuEntry(value: 6, label: 'Five'),
DropdownMenuEntry(value: 7, label: 'Five'),
DropdownMenuEntry(value: 8, label: 'Five'),
DropdownMenuEntry(value: 9, label: 'Nine'),
DropdownMenuEntry(value: 10, label: 'Ten'),
],
initialSelection: _selectedItem,
),
],
);
}
}It could be worth creating an issue for it, but as you suggested in the previous PR, having duplicate values in the entries list doesn't make sense, so I would assume that having duplicate labels also doesn't make sense. And about this PR, I'm okay with either storing the index or compering the textfield. But compering the textfield would result in the subtle bug I mentioned in the previous comment, so WDYT? |
Oh I thought we are going to revert the PR that caused regression and start a new fix? |
|
I see no reason for the revert since the bug you mentioned is not related to this or the previous PR. The new bug that this PR fixes is a simple mistake of storing the index of selected entry from the filtered list instead of the absolute index. Even when applying your changes, it's about the same since the only thing that would differ is getting the index from a loop instead of storing it locally. |
You put a lot of effort on analyzing this problem, this is much appreciated. 🙏 For instance, in #174757 (comment) you mentioned that storing values would be better but not possible because "So if an entry with null value was clicked". This is true, but maybe the problem is that this is a bug which has to be fixed and we should not accept entries with null value (see #176711 which might address this problem). |
|
@QuncCccccc I think the most actionable path is to get this PR merged. The additional code is somewhat small and it will be easy to cherry-pick this fix. (Cherry-picking the revert might be more difficult as the original PR landed long ago). |
Issues
Pre-launch Checklist
///).