Skip to content

Conversation

@TahaTesser
Copy link
Member

fixes SearchAnchor triggers extra search operations

Code sample

expand to view the code sample
import 'package:flutter/material.dart';

Future<List<String>> createFuture() async {
  return List.generate(1000, (index) => "Hello World!");
}

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      title: 'Flutter Demo',
      home: MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({super.key, required this.title});

  final String title;

  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  final SearchController controller = SearchController();

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        backgroundColor: Theme.of(context).colorScheme.inversePrimary,
        title: Text(widget.title),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            SearchAnchor(
              searchController: controller,
              suggestionsBuilder: (suggestionsContext, controller) {
                final resultFuture = createFuture();
                return [
                  FutureBuilder(
                    future: resultFuture,
                    builder: ((context, snapshot) {
                      if (snapshot.connectionState != ConnectionState.done) {
                        return const LinearProgressIndicator();
                      }
                      final result = snapshot.data;
                      if (result == null) {
                        return const LinearProgressIndicator();
                      }
                      return ListView.builder(
                        shrinkWrap: true,
                        physics: const NeverScrollableScrollPhysics(),
                        itemCount: result.length,
                        itemBuilder: (BuildContext context, int index) {
                          final root = result[index];
                          return ListTile(
                            leading: const Icon(Icons.article),
                            title: Text(root),
                            subtitle: Text(
                              root,
                              overflow: TextOverflow.ellipsis,
                              style: TextStyle(
                                color: Theme.of(suggestionsContext)
                                    .colorScheme
                                    .onSurfaceVariant,
                              ),
                            ),
                            onTap: () {},
                          );
                        },
                      );
                    }),
                  ),
                ];
              },
              builder: (BuildContext context, SearchController controller) {
                return IconButton(
                  onPressed: () {
                    controller.openView();
                  },
                  icon: const Icon(Icons.search),
                );
              },
            ),
          ],
        ),
      ),
    );
  }
}

Before

before.mp4

After

after.mp4

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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. labels Feb 14, 2024
@TahaTesser TahaTesser marked this pull request as ready for review February 14, 2024 11:10
@TahaTesser
Copy link
Member Author

cc: @sun-jiao

@TahaTesser TahaTesser force-pushed the fix_suggestionsBuilder_bug branch from d1cb932 to 3a8cbde Compare February 21, 2024 14:04
@TahaTesser
Copy link
Member Author

cc: @QuncCccccc

@TahaTesser TahaTesser force-pushed the fix_suggestionsBuilder_bug branch from 3a8cbde to 451f182 Compare March 1, 2024 09:46
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! LGTM! Sorry for the late response.

@TahaTesser TahaTesser force-pushed the fix_suggestionsBuilder_bug branch from 451f182 to 8fc89e3 Compare March 18, 2024 11:20
@QuncCccccc
Copy link
Contributor

@TahaTesser
Copy link
Member Author

@TahaTesser
Copy link
Member Author

TahaTesser commented Mar 19, 2024

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should use a Timer. And the pending timer should be disposed if State.dispose is called while the timer is open

Copy link
Member Author

@TahaTesser TahaTesser Mar 19, 2024

Choose a reason for hiding this comment

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

unawaited was there so i can try work around it.

I applied your suggestion to use Timer and it does pass flutter_hooks test locally.

Can you please explain why Timer is better? unawaited is fairly new.

Copy link
Contributor

Choose a reason for hiding this comment

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

Future internally creates a Timer. But that Timer is not disposed until the callback executes.

So this creates a race condition between the test assertion about "non-disposed Timers" and the callback execution.

If the Timer assert runs before the callback is executed, Future's Timer has yet to be cleaned-up, making the Timer assert fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

I appreciate your suggestion and explanation, it helped a lot.

@TahaTesser TahaTesser force-pushed the fix_suggestionsBuilder_bug branch from cbb0a0f to 5dac0ff Compare March 19, 2024 14:22
@TahaTesser TahaTesser requested a review from QuncCccccc March 19, 2024 14:51
@TahaTesser
Copy link
Member Author

@QuncCccccc

With Remi's suggestion, all tests are green now.

Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

LGTM:) Thanks for the fix!

@TahaTesser TahaTesser force-pushed the fix_suggestionsBuilder_bug branch from 5dac0ff to 24964b2 Compare March 25, 2024 20:28
@TahaTesser TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 25, 2024
@auto-submit auto-submit bot merged commit 8363e78 into flutter:master Mar 25, 2024
@TahaTesser TahaTesser deleted the fix_suggestionsBuilder_bug branch March 26, 2024 07:58
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App 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.

SearchAnchor triggers extra search operations

3 participants