It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@victorsanni can you please review the changes, working on test cases.

also i'm not sure if loadingViewBuilder would be better or loadingStateBuilder or do you have any other suggestion in mind?

@mohsinraza-fdev the approach looks nice to me. cc @justinmc @LongCatIsLooong

This looks really exciting. @victorsanni and I have been thinking about how to make Autocomplete fully support loading and empty. I'm swamped this week but I'm writing myself a note to review this next week, sorry for the delay!

Some feedback after playing around with your autocomplete.5 example:

• Focus the field and the loading message appears, but that's probably unnecessary.
• Select an option, then type or backspace. No loading message is shown, but it should be. It does indeed make a request, because after a few seconds you will see the results or the "no results" message.
• I'm having trouble coming up with a reliable repro, but sometimes I saw the "network error" error message and the "no results found" message overlap (see image below). Maybe it's a Flutter layout bug unrelated to this PR, not sure. But maybe we should not display "no results found" when a network error happens? It might make the user think that their query legitimately found no results, when that's not necessarily true.

Ill working on the example, thanks for highlighting the issues.

What's the advantage of separating out loadingStateBuilder and emptyStateBuilder instead of handling everything inside of optionsViewBuilder? Maybe with the addition of an isLoading parameter or something.

We can put these states inside optionsViewBuilder, i looked at other widgets like Image.network which are using separate loading builders so thats why i suggested the idea.

We can definitely put loading and empty state indicators, boolean or enum inside optionsViewBuilder to handle these different views.

Which one do you think we should go with?

I'm leaning towards a combined optionsViewBuilder. Looking at how those similar builders are used in Image:

They are used at the same time, while in this PR it's always one or the other separately:

              if (_isLoadingOptions && widget.loadingStateBuilder != null) {
                return widget.loadingStateBuilder!(context, _options);
              }
              if (_options.isEmpty && widget.emptyStateBuilder != null) {
                return widget.emptyStateBuilder!(context, _textEditingController.text);
              }
              return widget.optionsViewBuilder(context, _select, _options);

@victorsanni Do you have thoughts on this?

If we do modify the parameters of optionsViewBuilder, we might have to deprecate optionsViewBuilder and replace it with a new parameter.

I'm thinking we can use optionsViewBuilder and add an enum to its parameters, like optionsStatus or something. And it could be .loading, .empty, .nonEmpty or something like that.

I'm thinking we can use optionsViewBuilder and add an enum to its parameters, like optionsStatus or something. And it could be .loading, .empty, .nonEmpty or something like that.

That would be great i think, ill start working on it.

@justinmc @LongCatIsLooong what do you think of an enum approach? Are there better alternatives?

@justinmc @LongCatIsLooong what do you think of an enum approach? Are there better alternatives?

So the builder callback would return null to indicate the candidate view should not show, if the developer wants to dismiss the candidate view when empty / loading? Adding additional parameters to the callback sounds reasonable as long as they don't break existing code. But I think it will likely be a breaking change.

Also, we currently don't show the "outdated" results when optionsBuilder future is being resolved right?

Also, we currently don't show the "outdated" results when optionsBuilder future is being resolved right?

No we do. Currently on master, the previous options remain visible while the next options are loading. So for example in autocomplete.4.dart, I type "a" and wait for all 3 options to appear, then type a second "a". All 3 options remain on screen until the async operation finishes and then only "aardvark" is displayed.

@victorsanni Could your enum approach be achieved by the user without needing us to pass anything into optionsViewBuilder?

return Autocomplete<String>(
  optionsBuilder: (TextEditingValue textEditingValue) async {
    _searchingWithQuery = textEditingValue.text;

    late final Iterable<String> options;
    try {
      _requestStatus = _RequestStatus.loading;
      options = await _FakeAPI.search(_currentQuery!, _networkEnabled);
      _requestStatus = _RequestStatus.inactive;
    } on _NetworkException {
      _requestStatus = _RequestStatus.error;
      if (mounted) {
        setState(() {
          _networkError = true;
        });
      }
      return <String>[];
    }

    if (_searchingWithQuery != textEditingValue.text) {
      return _lastOptions;
    }

    _lastOptions = options;
    return options;
  },
  optionsViewBuilder: (BuildContext context, AutocompleteOnSelected<T> onSelected, Iterable<T> options) {
    if (options.isEmpty) {
      // Don't show the options overlay at all.
      return null;
    }

    return switch (_requestStatus) {
      _RequestStatus.inactive => _MyOptions(options: options),
      _RequestStatus.loading => const CircularProgressIndicator(),
      _RequestStatus.error => const Text('Fail.'),
    };
  );

Then I guess the only code change we would need to make to RawAutocomplete is to call optionsViewBuilder more frequently (possibly a breaking change).

Oh and optionsViewBuilder would need to be able to return null to indicate that the options should not be shown at all. I guess the user can't just return SizedBox.shrink because we need to not show the Overlay at all.

Could your enum approach be achieved by the user without needing us to pass anything into optionsViewBuilder?

Then I guess the only code change we would need to make to RawAutocomplete is to call optionsViewBuilder more frequently (possibly a breaking change).

II think it makes sense for users to handle the output of optionsViewBuilder themselves instead of increasing the API surface, as long as we have a clear example on the website.

@mohsinraza-fdev can you try this approach to see if it can be done without breaking users?

@victorsanni Could your enum approach be achieved by the user without needing us to pass anything into optionsViewBuilder?

Using an inherited widget we can do that i think, just the way we track the highlighted item index.

@mohsinraza-fdev can you try this approach to see if it can be done without breaking users?

We can either add the OptionsViewState enum in optionsViewBuilder as an arguments or create an inherited widget which holds the state.
The builder argument method will change the function signature, but inherited widget method wont break the users.

What do you guys think?

One more thing, how many state are we talking about, currently i have empty, loading and ready in mind, do we need to add and error state or any other states?

and do we really need the empty state enum?, currently we hide the options overlay on empty results, but after this implementation we will have to build the overlay on empty results as well so the user can check the empty state from the options variable itself.

need a little clarification on this...

We can either add the OptionsViewState enum in optionsViewBuilder as an arguments or create an inherited widget which holds the state. The builder argument method will change the function signature, but inherited widget method wont break the users.

It would be nice to not break users. Would love to see what this implementation would look like. But I really don't know what the pros and cons are. Also, changing the function signature might be better for discoverability.

One more thing, how many state are we talking about, currently i have empty, loading and ready in mind, do we need to add and error state or any other states?

Currently, users can perform an async operation in optionsBuilder, and if that errors out, they might return some special set of options which they catch in optionsViewBuilder to return the desired view. It would be nice to wrap that logic for them with an error state. But I can't think of other states. Maybe initial, for when the field is focused for the first time? But I don't know if that's already wrapped somewhere else.

and do we really need the empty state enum?, currently we hide the options overlay on empty results, but after this implementation we will have to build the overlay on empty results as well so the user can check the empty state from the options variable itself.

My understanding is the options can be empty either on first focus or if nothing is found. Maybe those need separate states. I think in general we want to reduce the logic in optionsBuilder so users don't have to add special identifiers to the options returned from optionsBuilder to have different views in optionsViewBuilder.

(PR Triage): Hey @mohsinraza-fdev, would you like to continue working on this change?

(PR Triage): Hey @mohsinraza-fdev, would you like to continue working on this change?

Hey - I've been really swamped with work this month. I'd still love to help with this task, but I might need a bit more time. If it's urgent though, no worries about reassigning it to someone else!

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

(triage): I am going to close this PR since it has gone stale. Please feel free to open a new one when you find the time to work on this again. Thank you.