Changes to use valuenotifier instead of a force rebuild for WidgetInspector #131634

CoderDake · 2023-07-31T22:05:29Z

Change the forceRebuild behaviour of the WidgetInspector to use ValueListenableBuilders instead. This should help resolve heavy rebuilds when the widgetInspectorOverride value changes.

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

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

packages/flutter/test/widgets/widget_inspector_test.dart

jacob314

LGTM after fixing a couple minor issues.

jacob314 · 2023-08-28T21:17:08Z

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

+  /// and view what widgets and render objects associated with it. An outline of
+  /// the selected widget and some summary information is shown on device and
+  /// more detailed information is shown in the IDE or DevTools.
+  static ValueNotifier<bool> debugShowWidgetInspectorOverrideNotifier = ValueNotifier<bool>(false);


Fyi @goderbauer. I'd like your opinion on the naming convention to use for a debug override like this.
If it wasn't for the conflict with the deprecated name, would we want to call this just
debugShowWeidgetInspectorOverride even though the type is ValueNotifier?

Why the "override" in this name? What is getting overridden? The answer should be added to the docs.

Wondering: Why does the Notifier need to be part of the public API? It seems like it could be private, only exposing a public getter/setter to change its value since there do not appear to be any clients outside of this file that need to listen to the listenable? The public getter/setter could be the existing debugShowWidgetInspectorOverride, so there wouldn't be a need for new API surface at all...

jacob314

LGTM after fixing a couple minor issues.

packages/flutter/test_fixes/widgets/app.dart

packages/flutter/test_fixes/widgets/app.dart.expect

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

goderbauer · 2023-09-11T18:11:04Z

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

+    'Use debugShowWidgetInspectorOverrideNotifier.value instead. '
+    'This feature was deprecated after v3.13.0-19.0.pre.'
+  )
+  bool get debugShowWidgetInspectorOverride =>


https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#consider-using--for-short-functions-and-methods

=> may only be used if everything is formatted in one line. Either move everything to the same line or switch to block.

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

goderbauer · 2023-09-11T18:17:47Z

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

+  /// and view what widgets and render objects associated with it. An outline of
+  /// the selected widget and some summary information is shown on device and
+  /// more detailed information is shown in the IDE or DevTools.
+  static ValueNotifier<bool> debugShowWidgetInspectorOverrideNotifier = ValueNotifier<bool>(false);


Why the "override" in this name? What is getting overridden? The answer should be added to the docs.

Wondering: Why does the Notifier need to be part of the public API? It seems like it could be private, only exposing a public getter/setter to change its value since there do not appear to be any clients outside of this file that need to listen to the listenable? The public getter/setter could be the existing debugShowWidgetInspectorOverride, so there wouldn't be a need for new API surface at all...

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

flutter-dashboard · 2023-10-01T18:19:27Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

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.

goderbauer

Implementation looks good, but some checks are unhappy.

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

goderbauer · 2023-10-11T19:56:51Z

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

  /// and view what widgets and render objects associated with it. An outline of
  /// the selected widget and some summary information is shown on device and
  /// more detailed information is shown in the IDE or DevTools.
-  static bool debugShowWidgetInspectorOverride = false;
+  @visibleForTesting


We try to avoid this because at the end of the day it is still public API.

Instead of using WidgetsApp.debugShowWidgetInspectorOverrideNotifier.value the test could be using the public WidgetsApp.debugShowWidgetInspectorOverride right? That just leaves us with WidgetsApp.debugShowWidgetInspectorOverrideNotifier.addListener. The old version was testing this indirectly via service.rebuildCount. Any reason we can't continue to do that and keep the Notifier private? If we can't do that, we could at least lock down the public type of the debugShowWidgetInspectorOverrideNotifier to something like Listenable so it doesn't expose more API surface then necessary.

I was able to overcome this by grabbing the Listenable from the ValueListenableBuilder, instead of exposing it in WidgetsApp :D Let me know what you think :D

…Builder and accessing the listenable there.

goderbauer

LGTM

This looks great! 🎉

packages/flutter/test/widgets/widget_inspector_test.dart

…orce rebuild for (flutter/flutter#131634)

Roll Flutter from 3865e49a68ab to b00216b3201d (34 revisions) flutter/flutter@3865e49...b00216b 2023-10-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from e2319935d8c8 to f8eb68b115f1 (1 revision) (flutter/flutter#136605) 2023-10-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from edf688d30ae3 to e2319935d8c8 (1 revision) (flutter/flutter#136604) 2023-10-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1b3bf985490c to edf688d30ae3 (1 revision) (flutter/flutter#136603) 2023-10-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from e4f8b2267906 to 1b3bf985490c (1 revision) (flutter/flutter#136601) 2023-10-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from b64ebc0be62d to e4f8b2267906 (1 revision) (flutter/flutter#136599) 2023-10-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from 529eb423604c to b64ebc0be62d (1 revision) (flutter/flutter#136592) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 82596277bd2b to 529eb423604c (1 revision) (flutter/flutter#136591) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 015e352a174e to 82596277bd2b (1 revision) (flutter/flutter#136590) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 73acf62a2fe1 to 015e352a174e (1 revision) (flutter/flutter#136589) 2023-10-14 jonahwilliams@google.com Upload GPU frame times for Impeller on Android/iOS. (flutter/flutter#136565) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from c777aae5ca15 to 73acf62a2fe1 (1 revision) (flutter/flutter#136581) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from d4e765fbefb8 to c777aae5ca15 (1 revision) (flutter/flutter#136580) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 472be42e90e0 to d4e765fbefb8 (1 revision) (flutter/flutter#136578) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9149a427ef98 to 472be42e90e0 (1 revision) (flutter/flutter#136574) 2023-10-14 sokolovskyi.konstantin@gmail.com _RouterState should dispose created _RestorableRouteInformation. (flutter/flutter#136556) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from cd1a5ed6f961 to 9149a427ef98 (1 revision) (flutter/flutter#136571) 2023-10-14 christopherfujino@gmail.com increase windows build test sharding, revert timeout 30 mins (flutter/flutter#136474) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from a4e62581525c to cd1a5ed6f961 (2 revisions) (flutter/flutter#136566) 2023-10-14 gspencergoog@users.noreply.github.com Change some usage of RawKeyEvent to KeyEvent in preparation for deprecation (flutter/flutter#136420) 2023-10-13 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from 6605151b4d0a to a4e62581525c (9 revisions) (flutter/flutter#136564) 2023-10-13 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.0 to 2.22.3 (flutter/flutter#136563) 2023-10-13 jonahwilliams@google.com [Impeller] GPU frame timings summarization. (flutter/flutter#136408) 2023-10-13 chevalier.dan@gmail.com Changes WidgetInspector to use valuenotifier instead of a force rebuild for (flutter/flutter#131634) 2023-10-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 799e8de11c12 to 6605151b4d0a (3 revisions) (flutter/flutter#136558) 2023-10-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 93f02f7237db to 799e8de11c12 (6 revisions) (flutter/flutter#136553) 2023-10-13 polinach@google.com Mark leak in NativeCodec.getNextFrame. (flutter/flutter#136514) 2023-10-13 polinach@google.com Stop skipping leaks in the test. (flutter/flutter#136512) 2023-10-13 christopherfujino@gmail.com run tests under dev/tools as part of framework-misc and get them passing (flutter/flutter#136501) 2023-10-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 60968c892649 to 93f02f7237db (5 revisions) (flutter/flutter#136546) 2023-10-13 15619084+vashworth@users.noreply.github.com Remove bringup from tests and move some back to presubmit. Reorganize TESTOWNERS. (flutter/flutter#136498) 2023-10-13 41873024+droidbg@users.noreply.github.com [leak-tracking] Add leak tracking in test/rendering -2 (flutter/flutter#136310) 2023-10-13 41873024+droidbg@users.noreply.github.com [leak-tracking] Add leak tracking in test/rendering -3 (flutter/flutter#136308) 2023-10-13 engine-flutter-autoroll@skia.org Roll Packages from 93c3f69 to fd84e65 (3 revisions) (flutter/flutter#136538) 2023-10-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from f9aed0267352 to 60968c892649 (1 revision) (flutter/flutter#136539) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 ...

…r#5147) Roll Flutter from 3865e49a68ab to b00216b3201d (34 revisions) flutter/flutter@3865e49...b00216b 2023-10-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from e2319935d8c8 to f8eb68b115f1 (1 revision) (flutter/flutter#136605) 2023-10-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from edf688d30ae3 to e2319935d8c8 (1 revision) (flutter/flutter#136604) 2023-10-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1b3bf985490c to edf688d30ae3 (1 revision) (flutter/flutter#136603) 2023-10-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from e4f8b2267906 to 1b3bf985490c (1 revision) (flutter/flutter#136601) 2023-10-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from b64ebc0be62d to e4f8b2267906 (1 revision) (flutter/flutter#136599) 2023-10-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from 529eb423604c to b64ebc0be62d (1 revision) (flutter/flutter#136592) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 82596277bd2b to 529eb423604c (1 revision) (flutter/flutter#136591) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 015e352a174e to 82596277bd2b (1 revision) (flutter/flutter#136590) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 73acf62a2fe1 to 015e352a174e (1 revision) (flutter/flutter#136589) 2023-10-14 jonahwilliams@google.com Upload GPU frame times for Impeller on Android/iOS. (flutter/flutter#136565) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from c777aae5ca15 to 73acf62a2fe1 (1 revision) (flutter/flutter#136581) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from d4e765fbefb8 to c777aae5ca15 (1 revision) (flutter/flutter#136580) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 472be42e90e0 to d4e765fbefb8 (1 revision) (flutter/flutter#136578) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9149a427ef98 to 472be42e90e0 (1 revision) (flutter/flutter#136574) 2023-10-14 sokolovskyi.konstantin@gmail.com _RouterState should dispose created _RestorableRouteInformation. (flutter/flutter#136556) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from cd1a5ed6f961 to 9149a427ef98 (1 revision) (flutter/flutter#136571) 2023-10-14 christopherfujino@gmail.com increase windows build test sharding, revert timeout 30 mins (flutter/flutter#136474) 2023-10-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from a4e62581525c to cd1a5ed6f961 (2 revisions) (flutter/flutter#136566) 2023-10-14 gspencergoog@users.noreply.github.com Change some usage of RawKeyEvent to KeyEvent in preparation for deprecation (flutter/flutter#136420) 2023-10-13 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from 6605151b4d0a to a4e62581525c (9 revisions) (flutter/flutter#136564) 2023-10-13 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.0 to 2.22.3 (flutter/flutter#136563) 2023-10-13 jonahwilliams@google.com [Impeller] GPU frame timings summarization. (flutter/flutter#136408) 2023-10-13 chevalier.dan@gmail.com Changes WidgetInspector to use valuenotifier instead of a force rebuild for (flutter/flutter#131634) 2023-10-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 799e8de11c12 to 6605151b4d0a (3 revisions) (flutter/flutter#136558) 2023-10-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 93f02f7237db to 799e8de11c12 (6 revisions) (flutter/flutter#136553) 2023-10-13 polinach@google.com Mark leak in NativeCodec.getNextFrame. (flutter/flutter#136514) 2023-10-13 polinach@google.com Stop skipping leaks in the test. (flutter/flutter#136512) 2023-10-13 christopherfujino@gmail.com run tests under dev/tools as part of framework-misc and get them passing (flutter/flutter#136501) 2023-10-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 60968c892649 to 93f02f7237db (5 revisions) (flutter/flutter#136546) 2023-10-13 15619084+vashworth@users.noreply.github.com Remove bringup from tests and move some back to presubmit. Reorganize TESTOWNERS. (flutter/flutter#136498) 2023-10-13 41873024+droidbg@users.noreply.github.com [leak-tracking] Add leak tracking in test/rendering -2 (flutter/flutter#136310) 2023-10-13 41873024+droidbg@users.noreply.github.com [leak-tracking] Add leak tracking in test/rendering -3 (flutter/flutter#136308) 2023-10-13 engine-flutter-autoroll@skia.org Roll Packages from 93c3f69 to fd84e65 (3 revisions) (flutter/flutter#136538) 2023-10-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from f9aed0267352 to 60968c892649 (1 revision) (flutter/flutter#136539) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 ...

Initial changes to use valuenotifier instead of a force rebuild

3601c78

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jul 31, 2023

CoderDake added 2 commits August 1, 2023 11:14

better import to fix analysis issues

a6518eb

My attempt

723b47c

github-actions bot added the c: tech-debt Technical debt, code quality, testing, etc. label Aug 2, 2023

small changes

5615361

CoderDake mentioned this pull request Aug 2, 2023

dart fix bug with static fields in a Class dart-lang/sdk#53101

Closed

CoderDake added 9 commits August 3, 2023 12:59

Merge branch 'main' into no-force-rebuild

d409eb3

Cleanup

37ddd4d

Merge remote-tracking branch 'upstream/master' into no-force-rebuild

f6c0ca2

better deprecation formatting

80ca685

no number after pre

3fdcaa6

vvvvvv

e982f31

Doc Fix

f7ba402

Merge branch 'master' into no-force-rebuild

bb0af08

Merge branch 'master' into no-force-rebuild

86e587c

CoderDake commented Aug 9, 2023

View reviewed changes

packages/flutter/lib/src/widgets/app.dart Outdated Show resolved Hide resolved

version update for deprecation

bf7f4a5

CoderDake requested review from jacob314 and keertip August 9, 2023 14:19

CoderDake marked this pull request as ready for review August 9, 2023 14:21

CoderDake added 2 commits August 9, 2023 15:39

Merge branch 'master' into no-force-rebuild

d02d537

Merge branch 'master' into no-force-rebuild

5267395

jacob314 reviewed Aug 28, 2023

View reviewed changes

packages/flutter/lib/src/widgets/app.dart Outdated Show resolved Hide resolved

jacob314 reviewed Aug 28, 2023

View reviewed changes

packages/flutter/lib/src/widgets/widget_inspector.dart Outdated Show resolved Hide resolved

jacob314 reviewed Aug 28, 2023

View reviewed changes

packages/flutter/test/widgets/widget_inspector_test.dart Show resolved Hide resolved

jacob314 reviewed Aug 28, 2023

View reviewed changes

jacob314 approved these changes Aug 28, 2023

View reviewed changes

CoderDake requested a review from goderbauer August 29, 2023 13:39

goderbauer reviewed Sep 11, 2023

View reviewed changes

CoderDake and others added 7 commits October 5, 2023 14:59

Handling easy comment feedback

832719f

handling follow up

6b1d106

Merge remote-tracking branch 'upstream/main' into no-force-rebuild

fea0285

nice cleanup based on comments. Much simpler

9d5049f

Merge remote-tracking branch 'origin/main' into no-force-rebuild

401b961

fix import and add comment about override

fc05ca2

static is needed

9b7f6b7

goderbauer reviewed Oct 6, 2023

View reviewed changes

CoderDake added 2 commits October 10, 2023 11:08

goder lint

8560213

visible for testing

b3f4fe1

CoderDake changed the title ~~Initial changes to use valuenotifier instead of a force rebuild~~ Changes to use valuenotifier instead of a force rebuild for WidgetInspector Oct 11, 2023

goderbauer reviewed Oct 11, 2023

View reviewed changes

CoderDake added 2 commits October 12, 2023 10:52

Found a way to avoid visibleForTesting by finding the ValueListenable…

ea59460

…Builder and accessing the listenable there.

quick rename

8652ed4

goderbauer approved these changes Oct 12, 2023

View reviewed changes

packages/flutter/test/widgets/widget_inspector_test.dart Outdated Show resolved Hide resolved

CoderDake added 2 commits October 12, 2023 13:13

cleaner builder find

e449181

Merge remote-tracking branch 'upstream/main' into no-force-rebuild

96515e8

CoderDake merged commit ac095ed into flutter:master Oct 13, 2023
67 checks passed

CoderDake deleted the no-force-rebuild branch October 13, 2023 21:08

engine-flutter-autoroll mentioned this pull request Oct 14, 2023

Roll Flutter from 3865e49a68ab to 2963b7460dfb (24 revisions) flutter/packages#5145

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 14, 2023

ac095ed39 Changes WidgetInspector to use valuenotifier instead of a f…

7c944bf

…orce rebuild for (flutter/flutter#131634)

engine-flutter-autoroll mentioned this pull request Oct 14, 2023

Roll Flutter from 3865e49a68ab to 195647440917 (25 revisions) flutter/packages#5146

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 14, 2023

ac095ed39 Changes WidgetInspector to use valuenotifier instead of a f…

860339b

…orce rebuild for (flutter/flutter#131634)

engine-flutter-autoroll mentioned this pull request Oct 15, 2023

Roll Flutter from 3865e49a68ab to b00216b3201d (34 revisions) flutter/packages#5147

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 15, 2023

ac095ed39 Changes WidgetInspector to use valuenotifier instead of a f…

952e458

…orce rebuild for (flutter/flutter#131634)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to use valuenotifier instead of a force rebuild for WidgetInspector #131634

Changes to use valuenotifier instead of a force rebuild for WidgetInspector #131634

CoderDake commented Jul 31, 2023 •

edited

jacob314 left a comment

jacob314 Aug 28, 2023

goderbauer Sep 11, 2023

jacob314 left a comment

goderbauer Sep 11, 2023

goderbauer Sep 11, 2023

flutter-dashboard bot commented Oct 1, 2023

goderbauer left a comment

goderbauer Oct 11, 2023

CoderDake Oct 12, 2023

goderbauer left a comment

Changes to use valuenotifier instead of a force rebuild for WidgetInspector #131634

Changes to use valuenotifier instead of a force rebuild for WidgetInspector #131634

Conversation

CoderDake commented Jul 31, 2023 • edited

jacob314 left a comment

Choose a reason for hiding this comment

jacob314 Aug 28, 2023

Choose a reason for hiding this comment

goderbauer Sep 11, 2023

Choose a reason for hiding this comment

jacob314 left a comment

Choose a reason for hiding this comment

goderbauer Sep 11, 2023

Choose a reason for hiding this comment

goderbauer Sep 11, 2023

Choose a reason for hiding this comment

flutter-dashboard bot commented Oct 1, 2023

goderbauer left a comment

Choose a reason for hiding this comment

goderbauer Oct 11, 2023

Choose a reason for hiding this comment

CoderDake Oct 12, 2023

Choose a reason for hiding this comment

goderbauer left a comment

Choose a reason for hiding this comment

CoderDake commented Jul 31, 2023 •

edited