Update example code and docs for InteractiveViewer.builder #96019

jollygreenlaser · 2022-01-01T22:58:06Z

The old example code laid out every single cell and did visibility checks on each of them. This resulted in poor performance and is not a viable pattern to copy.

This takes inspiration from the official youtube video on this builder form to turn this example into an infinite grid. This also adds this video to the documentation because it's really low in the SEO yet very valuable to understand the builder constructor.

While performance can still be hit (especially in debug mode) it degrades with # of cells on screen, rather than # of cells in total.

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.

examples/api/lib/widgets/interactive_viewer/interactive_viewer.builder.0.dart

goderbauer · 2022-01-05T00:21:06Z

Can you fix the analyzer issues please? Also, we have started to add tests for these samples. Can you please add one that covers your change? Thanks!

jollygreenlaser · 2022-01-05T04:35:20Z

@goderbauer I fixed the analyzer issues, thanks for pointing that out.

I will add the unit tests as soon as I can. If you have any examples to point me towards it's greatly appreciated, no worries if not I can probably find some. This is my first PR that touches flutter!

justinmc

Thanks for cleaning up this example! I should have updated it when I did Lazy Flutter Performance. This looks more useful and easier to understand, I'm on board. Just a few small nits you might want to consider.

LGTM 👍

examples/api/lib/widgets/interactive_viewer/interactive_viewer.builder.0.dart

packages/flutter/lib/src/widgets/interactive_viewer.dart

examples/api/lib/widgets/interactive_viewer/interactive_viewer.builder.0.dart

justinmc

Sorry, I forgot that these examples can be tested now. I'll find you an example.

justinmc · 2022-01-07T00:00:33Z

Here's an example of testing an example:

The example: https://github.com/flutter/flutter/blob/master/examples/api/lib/widgets/layout_builder/layout_builder.0.dart
The example's test: https://github.com/flutter/flutter/blob/master/examples/api/test/widgets/layout_builder/layout_builder.0_test.dart

Maybe something like this would make a good test for this example:

Render the example.
Expect there to be a Positioned with "0 x 0".
Expect there to be the correct amount of Positioned widgets.
Expect there to be NO Positioned with "0 x 10".
Use a gesture to pan over to "0 x 10".
Expect that "0 x 0" is gone.
Expect that "0 x 10" is there.
Expect the number of Positioned widgets is the same.

jollygreenlaser · 2022-01-08T21:19:07Z

Thanks for all the help! Hopefully this test covers enough.

jollygreenlaser · 2022-01-14T03:27:51Z

@justinmc I noticed that this still lacks performance when zoomed out and many cells are on screen (this is why I increased the dimensions of each cell).

However, the same # of cells being rendered via the standard child form has no performance issue. My understanding is that this is due to the cells being rebuilt each tick with builder.

I'm pretty new to Flutter and couldn't figure out a way around this. Using the same const cell for every cell didn't make it happy. Read a ton of performance blogs now. Tried reading the source code for how slivers handle this, couldn't quite parse it, going to try again soon.

If you have any thoughts on this or time to help I'd greatly appreciate it. Hoping to be able to use this builder form with the same performance I could get from something like ListView.builder.

justinmc

LGTM 👍

So sorry this took me forever to re-review, I need to clean out my Github notifications. Thanks for adding the test.

If you want to try to improve the performance of the rebuilding cells, I have 2 ideas. At a glance I think they are being rebuilt every time the InteractiveViewer transformation matrix changes, which is a ton, probably 60 x per second during a pan gesture.

As a quick improvement, consider giving them a key that can uniquely identify them between builds. The widgets will still be rebuilt, but that might allow the underlying RenderObjects/Elements to be reused if I'm correct. Something like ValueKey('$row,$col').
You could try to cache the cell widgets. Any widgets that are being instantiated inside of InteractiveViewer's builder method will be re-instantiated every frame that InteractiveViewer zooms or pans. Doing something fancier there so we only instantiate/dispose the cells when they enter/leave the screen would probably help a lot.

I'll wait to merge this in case you want to take a look at the performance, or that could also be another PR.

justinmc · 2022-01-27T18:22:54Z

@gspencergoog Tagged you for secondary review since this uses your new example testing.

gspencergoog

gspencergoog · 2022-01-28T02:06:40Z

examples/api/lib/widgets/interactive_viewer/interactive_viewer.builder.0.dart

+                    child: builder(context, row, col)),
+          ],
+        ));


Add a couple of commas and reformat for better formatting.

Suggested change

child: builder(context, row, col)),

],

));

child: builder(context, row, col),

),

],

),

);

gspencergoog · 2022-01-28T02:09:28Z

examples/api/lib/widgets/interactive_viewer/interactive_viewer.builder.0.dart

-                        alignment: Alignment.centerLeft,
                        child: Text('$row x $column'),
                      ),
                    );


add a comma between the "}" and the ")" on the next line and reformat. It'll cascade nicely then (I know it was like that before, but might was well fix it).

goderbauer · 2022-02-09T22:43:25Z

(triage) @AleLazar Can you please fix the two nits so we can submit this? Thanks!

goderbauer · 2022-02-16T23:44:45Z

Superseded by #98623.

flutter-dashboard bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation c: contributor-productivity Team-specific productivity, code health, technical debt. labels Jan 1, 2022

jollygreenlaser force-pushed the infinite-grid-example branch from a2ef628 to 1e13abf Compare January 1, 2022 22:59

jollygreenlaser commented Jan 1, 2022

View reviewed changes

examples/api/lib/widgets/interactive_viewer/interactive_viewer.builder.0.dart Outdated Show resolved Hide resolved

jollygreenlaser commented Jan 1, 2022

View reviewed changes

examples/api/lib/widgets/interactive_viewer/interactive_viewer.builder.0.dart Outdated Show resolved Hide resolved

jollygreenlaser commented Jan 1, 2022

View reviewed changes

examples/api/lib/widgets/interactive_viewer/interactive_viewer.builder.0.dart Outdated Show resolved Hide resolved

jollygreenlaser force-pushed the infinite-grid-example branch from 1e13abf to f81ed79 Compare January 2, 2022 00:26

jollygreenlaser changed the title ~~Update example code for InteractiveViewer.builder~~ Update example code and docs for InteractiveViewer.builder Jan 2, 2022

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Jan 2, 2022

goderbauer requested a review from justinmc January 5, 2022 22:32

justinmc approved these changes Jan 6, 2022

View reviewed changes

justinmc requested changes Jan 6, 2022

View reviewed changes

jollygreenlaser force-pushed the infinite-grid-example branch from ddff8a1 to 47418fe Compare January 8, 2022 19:41

jollygreenlaser force-pushed the infinite-grid-example branch 2 times, most recently from cdc48cf to 4facbc4 Compare January 8, 2022 21:53

jollygreenlaser added 7 commits January 8, 2022 14:02

Update example code for InteractiveViewer.builder

091a4ba

Add youtube video to documentation

ecefa20

Fix analyzer issues

42a3b6c

Fix for comments

a4a1c75

Update copy

959d944

Avoid redundant args

32c224c

Add tests

a5b866e

jollygreenlaser force-pushed the infinite-grid-example branch from d680bcd to a5b866e Compare January 9, 2022 00:26

jollygreenlaser marked this pull request as ready for review January 9, 2022 01:06

jollygreenlaser requested a review from justinmc January 9, 2022 01:06

justinmc approved these changes Jan 27, 2022

View reviewed changes

justinmc requested a review from gspencergoog January 27, 2022 18:22

gspencergoog approved these changes Jan 28, 2022

View reviewed changes

goderbauer mentioned this pull request Feb 16, 2022

Update example code and docs for InteractiveViewer.builder #98623

Merged

goderbauer closed this Feb 16, 2022

Update example code and docs for InteractiveViewer.builder #96019

Update example code and docs for InteractiveViewer.builder #96019

Uh oh!

Conversation

jollygreenlaser commented Jan 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goderbauer commented Jan 5, 2022

Uh oh!

jollygreenlaser commented Jan 5, 2022

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc commented Jan 7, 2022

Uh oh!

jollygreenlaser commented Jan 8, 2022

Uh oh!

jollygreenlaser commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc commented Jan 27, 2022

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

gspencergoog Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

gspencergoog Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Feb 9, 2022

Uh oh!

goderbauer commented Feb 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jollygreenlaser commented Jan 1, 2022 •

edited

Loading

jollygreenlaser commented Jan 14, 2022 •

edited

Loading

gspencergoog Jan 28, 2022 •

edited

Loading