flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
166.22k stars 27.5k forks source link

RawAutocomplete: Doc on `onSelected` seems wrong about `TextEditingController` listeners #130187

Closed chrisbobbe closed 1 year ago

chrisbobbe commented 1 year ago

Is there an existing issue for this?

Steps to reproduce

This is the dartdoc on [RawAutocomplete.onSelected]:

  /// {@template flutter.widgets.RawAutocomplete.onSelected}
  /// Called when an option is selected by the user.
  ///
  /// Any [TextEditingController] listeners will not be called when the user
  /// selects an option, even though the field will update with the selected
  /// value, so use this to be informed of selection.
  /// {@endtemplate}
  final AutocompleteOnSelected<T>? onSelected;

That second paragraph—

  /// Any [TextEditingController] listeners will not be called when the user
  /// selects an option, even though the field will update with the selected
  /// value, so use this to be informed of selection.

—doesn't seem quite right. When RawAutocomplete registers a new option selection (i.e., when it calls its _select method with a new nextSelection), this code is run:

    _textEditingController.value = TextEditingValue(
      selection: TextSelection.collapsed(offset: selectionString.length),
      text: selectionString,
    );

Since [TextEditingController] extends ValueNotifier<…>, it will indeed notify any listeners that have been added. This can be seen in my code sample, copy-pasted below:

  1. Run the code sample. It has a stateful widget holding a _textEditingController, which it passes to a RawAutocomplete. That controller has a listener on it that prints "called $index".
  2. Tap the text field to focus it.
  3. (See called 0 printed, because focusing caused _textEditingController.value to change; it now has a cursor at 0.)
  4. Tap one of the autocomplete options.
  5. See called 1 printed, despite the doc saying that no [TextEditingController] listeners would be called.

As a fix, I think that whole paragraph in the doc can be removed. I have an idea what might have led to adding that paragraph, and a further improvement to the docs with that in mind: https://github.com/flutter/flutter/issues/130187#issuecomment-1626371195.

Expected results

I think the doc should not say

  /// Any [TextEditingController] listeners will not be called when the user
  /// selects an option, even though the field will update with the selected
  /// value

Actual results

The doc does say that.

Code sample

Code sample ```dart import 'package:flutter/material.dart'; void main() => runApp(const AutocompleteExampleApp()); class AutocompleteExampleApp extends StatelessWidget { const AutocompleteExampleApp({super.key}); @override Widget build(BuildContext context) { return const MaterialApp( home: Scaffold( body: Center( child: AutocompleteExample()))); } } class AutocompleteExample extends StatefulWidget { const AutocompleteExample({super.key}); @override AutocompleteExampleState createState() => AutocompleteExampleState(); } class AutocompleteExampleState extends State { final TextEditingController _textEditingController = TextEditingController(); final FocusNode _focusNode = FocusNode(); int index = 0; @override void initState() { super.initState(); _textEditingController.addListener(() { print('called $index'); index++; }); } @override Widget build(BuildContext context) { return RawAutocomplete( textEditingController: _textEditingController, focusNode: _focusNode, optionsBuilder: (TextEditingValue textEditingValue) => ['aardvark', 'bobcat', 'chameleon'], fieldViewBuilder: (BuildContext context, TextEditingController textEditingController, FocusNode focusNode, VoidCallback onFieldSubmitted) => TextField( controller: textEditingController, focusNode: focusNode), optionsViewBuilder: (BuildContext context, AutocompleteOnSelected onSelected, Iterable options) => Align( alignment: Alignment.topLeft, child: Material( child: ListView.builder( itemCount: options.length, itemBuilder: (BuildContext context, int index) { final String option = options.elementAt(index); return GestureDetector( onTap: () { onSelected(option); }, child: ListTile( title: Text(option))); })))); } } ```

Screenshots or Video

Screenshots / Video demonstration [Upload media here]

Logs

Logs (No errors expected or observed.)

Flutter Doctor output

Doctor output ```console $ flutter doctor Doctor summary (to see all details, run flutter doctor -v): [✓] Flutter (Channel main, 3.12.0-15.0.pre.39, on macOS 13.4.1 22F82 darwin-x64, locale en-US) [✓] Android toolchain - develop for Android devices (Android SDK version 30.0.3) [✓] Xcode - develop for iOS and macOS (Xcode 14.3.1) [✓] Chrome - develop for the web [✓] Android Studio (version 2021.3) [✓] VS Code (version 1.79.2) [✓] Connected device (3 available) ! Error: Chris’s Apple Watch has recently restarted. To use Chris’s Apple Watch with Xcode, unlock it and its companion iPhone (code 19) [✓] Network resources • No issues found! ```
chrisbobbe commented 1 year ago

I have an idea what might have led to adding that paragraph, and a further improvement to the docs with that in mind; coming up.

I think there is a way to get unexpected, probably buggy behavior in this area, and I wonder if it was happening when that paragraph was added back at this feature's beginning in 291ee9450. As of that commit, the only way to get access to the TextEditingController was to consume it in the provided fieldViewBuilder function. If you take the controller from there, it's easy to imagine bugs where a change happens before you've added your intended listener.

Callers can still run into that pitfall at main, and it might be good to add a tip to fieldViewBuilder's doc about not doing that. But as of 29d33cc38ecfd787bafae95bb82bdd6b6eb1c563, which let callers pass their own TextEditingController, it became possible to add listeners at an appropriate time, like in an initState, and those listeners will get called as expected.

gnprice commented 1 year ago

Interesting, thanks.

That second paragraph

Hmm yeah, that seems mistaken. If I'm understanding right: when the user selects an option, [RawAutocomplete._select] gets called. It will (if the selection is different from its previous value) call the [TextEditingController.value] setter, which will indeed call the listeners.

the provided fieldViewBuilder function. If you take the controller from there and add a listener to it, it seems likely that the listener might not get called at a time when you expect it to be.

Callers can still run into that pitfall at main, and it might be good to add a tip to fieldViewBuilder's doc about not doing that.

I don't think I'm following this part. That's the same controller, right? Is the issue that the selection might have happened before the listener was added? But I'm not seeing how that would happen either. If you have a draft of what an alternative, more useful warning would be, perhaps that would clarify matters.

In any case, removing that paragraph sounds good to me.

chrisbobbe commented 1 year ago

Is the issue that the selection might have happened before the listener was added?

Yeah, that's what I was thinking, but yeah, now I'm less sure about it actually being likely. Maybe, for example:

  1. You call .addListener from within your fieldViewBuilder function.
  2. You realize that your listener isn't just added once, but once every time fieldViewBuilder is called.
  3. You mistakenly correct that by making a nullable TextEditingController? _textEditingController on your stateful widget, and you add the listener with _textEditingController?.addListener inside your state's initState. (Which I suspect means your listener isn't added late, it's just never added, because _textEditingController is null inside initState.)

Anyway, especially with RawAutocomplete accepting a textEditingController param, maybe the pitfall is already made unlikely by just using good Flutter coding patterns that apply generally, and a special warning isn't needed. 🙂

I'll send a PR that removes the paragraph, soon.

chrisbobbe commented 1 year ago

Sent #130190. 🙂

github-actions[bot] commented 1 year ago

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.