Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display custom options view while options iterable waits to be resolved or is empty #147900

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

victorsanni
Copy link
Contributor

@victorsanni victorsanni commented May 6, 2024

Currently in Autocomplete, there is no way to:

This PR fixes these issues by introducing two boolean parameters to Autocomplete and RawAutocomplete: showOptionsViewOnEmptyOptions and showOptionsViewOnPendingOptions.

When set to true, showOptionsViewOnEmptyOptions displays the options view even when the options returned from optionsBuilder is empty.

When set to true, showOptionsViewOnPendingOptions displays the options view just before awaiting the optionsBuilder future, allowing a custom loading view to be shown while waiting for the optionsBuilder future to be resolved.

Fixes #108258 and #147377.

Before:

autocomplete_b4.mov

After:

Autocomplete.video.mov

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels May 6, 2024
@victorsanni victorsanni requested a review from justinmc May 6, 2024 23:42
@victorsanni victorsanni changed the title Enable displaying custom options view while options wait to be resolved or is empty Display custom options view while options iterable waits to be resolved or is empty May 7, 2024
@victorsanni victorsanni marked this pull request as ready for review May 7, 2024 20:57
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need make a final decision on if this 2-boolean approach is the best way to go. It looks pretty good in this PR. Maybe the error case that I mentioned in the comments is something we should make sure that it can do. Is there anything else that this 2-boolean approach would not cover? How about that issue that I tagged you in earlier?

examples/api/lib/material/autocomplete/autocomplete.5.dart Outdated Show resolved Hide resolved
examples/api/lib/material/autocomplete/autocomplete.5.dart Outdated Show resolved Hide resolved
examples/api/lib/material/autocomplete/autocomplete.5.dart Outdated Show resolved Hide resolved
examples/api/lib/material/autocomplete/autocomplete.5.dart Outdated Show resolved Hide resolved
///
/// This means that the original function will be called only after no calls
/// have been made for the given Duration.
_Debounceable<S, T> _debounce<S, T>(_Debounceable<S?, T> function) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you totally removed the debouncing from this example, would it be easier to understand the new stuff, or would it be too unrealistic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it might also make it difficult to understand why _isLoading should be set to false only when the current text is equal to the last text searched with (_lastValue). So, if someone were to dup the example, remove the (seemingly) extra variables _lastOptions and _lastValue and later add debouncing, they would probably run into errors and get confused.

packages/flutter/lib/src/material/autocomplete.dart Outdated Show resolved Hide resolved
@@ -353,6 +366,9 @@ class _RawAutocompleteState<T extends Object> extends State<RawAutocomplete<T>>
// Called when _textEditingController changes.
Future<void> _onChangedField() async {
final TextEditingValue value = _textEditingController.value;
if (widget.showOptionsViewOnUncompletedOptions){
_updateOptionsViewVisibility();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this affect synchronous values of optionsBuilder? optionsViewBuilder could be called twice back-to-back. Is that something we should avoid or it shouldn't matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If optionsBuilder is synchronous, and showOptionsViewOnPendingOptions is set to true, then the options view will just be rebuilt to display its current contents.

I've added a note for users to ignore showOptionsViewOnPendingOptions if their optionsBuilder is synchronous.

packages/flutter/test/widgets/autocomplete_test.dart Outdated Show resolved Hide resolved
@justinmc
Copy link
Contributor

Bug report on the example:

Screencast.from.2024-05-20.12-02-59.webm

Using the same steps, one time I also saw the "Loading" disappear and a few seconds later the correct results appeared. Couldn't capture that in the recording though. It probably has to do with subtle timing.

@victorsanni
Copy link
Contributor Author

Bug report on the example:

Screencast.from.2024-05-20.12-02-59.webm
Using the same steps, one time I also saw the "Loading" disappear and a few seconds later the correct results appeared. Couldn't capture that in the recording though. It probably has to do with subtle timing.

Fixing this bug required me to revert the change which terminates _onChangedField if the current textEditingController's text value is equal to the previous value, which means currently _onChangedField completes (rebuilding the options view as a result) on mere selection changes (which is the current behavior that was identified as undesired).

I was also running into another bug where if there was a selection and the app window goes into the background, when the text field is refocused, the loading message displays infinitely. To fix that, I use setState now in the examples to rebuild the widget once loading is completed.

@@ -157,6 +157,8 @@ class RawAutocomplete<T extends Object> extends StatefulWidget {
this.onSelected,
this.textEditingController,
this.initialValue,
this.showOptionsViewOnEmptyOptions = false,
this.showOptionsViewOnPendingOptions = false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that these two flags have a lot in common: they both want to show the options view. I think that's a sign that the API could be more generic / flexible. Can we let the user specify what the logic in _canShowOptionsView should look like, and have the current behavior as the default?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same thing. I'd like to see what that would look like.

bool get _canShowOptionsView => _focusNode.hasFocus && _selection == null && _options.isNotEmpty;
bool get _canShowOptionsView => _focusNode.hasFocus && _selection == null &&
(_options.isNotEmpty
|| widget.showOptionsViewOnEmptyOptions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: { } for multiline expressions per the style guide, instead of =>.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the logic doesn't seem correct? showOptionsViewOnEmptyOptions and showOptionsViewOnPendingOptions seem to have the same effect here, but they are different flags?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autocomplete widget calls optionsViewBuilder even when optionsBuilder return empty Iterable
3 participants