Changes to use valuenotifier instead of a force rebuild for WidgetInspector by CoderDake · Pull Request #131634 · flutter/flutter

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.

jacob314

LGTM after fixing a couple minor issues.

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

+  /// 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.

goderbauer

Implementation looks good, but some checks are unhappy.

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

  /// 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! 🎉

…ce rebuild for (flutter/flutter#131634)

Roll Flutter from 3865e49 to b00216b (34 revisions) flutter/flutter@3865e49...b00216b 2023-10-15 [email protected] Roll Flutter Engine from e2319935d8c8 to f8eb68b115f1 (1 revision) (flutter/flutter#136605) 2023-10-15 [email protected] Roll Flutter Engine from edf688d30ae3 to e2319935d8c8 (1 revision) (flutter/flutter#136604) 2023-10-15 [email protected] Roll Flutter Engine from 1b3bf985490c to edf688d30ae3 (1 revision) (flutter/flutter#136603) 2023-10-15 [email protected] Roll Flutter Engine from e4f8b2267906 to 1b3bf985490c (1 revision) (flutter/flutter#136601) 2023-10-15 [email protected] Roll Flutter Engine from b64ebc0be62d to e4f8b2267906 (1 revision) (flutter/flutter#136599) 2023-10-15 [email protected] Roll Flutter Engine from 529eb423604c to b64ebc0be62d (1 revision) (flutter/flutter#136592) 2023-10-14 [email protected] Roll Flutter Engine from 82596277bd2b to 529eb423604c (1 revision) (flutter/flutter#136591) 2023-10-14 [email protected] Roll Flutter Engine from 015e352a174e to 82596277bd2b (1 revision) (flutter/flutter#136590) 2023-10-14 [email protected] Roll Flutter Engine from 73acf62a2fe1 to 015e352a174e (1 revision) (flutter/flutter#136589) 2023-10-14 [email protected] Upload GPU frame times for Impeller on Android/iOS. (flutter/flutter#136565) 2023-10-14 [email protected] Roll Flutter Engine from c777aae5ca15 to 73acf62a2fe1 (1 revision) (flutter/flutter#136581) 2023-10-14 [email protected] Roll Flutter Engine from d4e765fbefb8 to c777aae5ca15 (1 revision) (flutter/flutter#136580) 2023-10-14 [email protected] Roll Flutter Engine from 472be42e90e0 to d4e765fbefb8 (1 revision) (flutter/flutter#136578) 2023-10-14 [email protected] Roll Flutter Engine from 9149a427ef98 to 472be42e90e0 (1 revision) (flutter/flutter#136574) 2023-10-14 [email protected] _RouterState should dispose created _RestorableRouteInformation. (flutter/flutter#136556) 2023-10-14 [email protected] Roll Flutter Engine from cd1a5ed6f961 to 9149a427ef98 (1 revision) (flutter/flutter#136571) 2023-10-14 [email protected] increase windows build test sharding, revert timeout 30 mins (flutter/flutter#136474) 2023-10-14 [email protected] Roll Flutter Engine from a4e62581525c to cd1a5ed6f961 (2 revisions) (flutter/flutter#136566) 2023-10-14 [email protected] Change some usage of RawKeyEvent to KeyEvent in preparation for deprecation (flutter/flutter#136420) 2023-10-13 [email protected] 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 [email protected] [Impeller] GPU frame timings summarization. (flutter/flutter#136408) 2023-10-13 [email protected] Changes WidgetInspector to use valuenotifier instead of a force rebuild for (flutter/flutter#131634) 2023-10-13 [email protected] Roll Flutter Engine from 799e8de11c12 to 6605151b4d0a (3 revisions) (flutter/flutter#136558) 2023-10-13 [email protected] Roll Flutter Engine from 93f02f7237db to 799e8de11c12 (6 revisions) (flutter/flutter#136553) 2023-10-13 [email protected] Mark leak in NativeCodec.getNextFrame. (flutter/flutter#136514) 2023-10-13 [email protected] Stop skipping leaks in the test. (flutter/flutter#136512) 2023-10-13 [email protected] run tests under dev/tools as part of framework-misc and get them passing (flutter/flutter#136501) 2023-10-13 [email protected] Roll Flutter Engine from 60968c892649 to 93f02f7237db (5 revisions) (flutter/flutter#136546) 2023-10-13 [email protected] Remove bringup from tests and move some back to presubmit. Reorganize TESTOWNERS. (flutter/flutter#136498) 2023-10-13 [email protected] [leak-tracking] Add leak tracking in test/rendering -2 (flutter/flutter#136310) 2023-10-13 [email protected] [leak-tracking] Add leak tracking in test/rendering -3 (flutter/flutter#136308) 2023-10-13 [email protected] Roll Packages from 93c3f69 to fd84e65 (3 revisions) (flutter/flutter#136538) 2023-10-13 [email protected] 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 [email protected],[email protected] 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 ...

Roll Flutter from 3865e49 to b00216b (34 revisions) flutter/flutter@3865e49...b00216b 2023-10-15 [email protected] Roll Flutter Engine from e2319935d8c8 to f8eb68b115f1 (1 revision) (flutter/flutter#136605) 2023-10-15 [email protected] Roll Flutter Engine from edf688d30ae3 to e2319935d8c8 (1 revision) (flutter/flutter#136604) 2023-10-15 [email protected] Roll Flutter Engine from 1b3bf985490c to edf688d30ae3 (1 revision) (flutter/flutter#136603) 2023-10-15 [email protected] Roll Flutter Engine from e4f8b2267906 to 1b3bf985490c (1 revision) (flutter/flutter#136601) 2023-10-15 [email protected] Roll Flutter Engine from b64ebc0be62d to e4f8b2267906 (1 revision) (flutter/flutter#136599) 2023-10-15 [email protected] Roll Flutter Engine from 529eb423604c to b64ebc0be62d (1 revision) (flutter/flutter#136592) 2023-10-14 [email protected] Roll Flutter Engine from 82596277bd2b to 529eb423604c (1 revision) (flutter/flutter#136591) 2023-10-14 [email protected] Roll Flutter Engine from 015e352a174e to 82596277bd2b (1 revision) (flutter/flutter#136590) 2023-10-14 [email protected] Roll Flutter Engine from 73acf62a2fe1 to 015e352a174e (1 revision) (flutter/flutter#136589) 2023-10-14 [email protected] Upload GPU frame times for Impeller on Android/iOS. (flutter/flutter#136565) 2023-10-14 [email protected] Roll Flutter Engine from c777aae5ca15 to 73acf62a2fe1 (1 revision) (flutter/flutter#136581) 2023-10-14 [email protected] Roll Flutter Engine from d4e765fbefb8 to c777aae5ca15 (1 revision) (flutter/flutter#136580) 2023-10-14 [email protected] Roll Flutter Engine from 472be42e90e0 to d4e765fbefb8 (1 revision) (flutter/flutter#136578) 2023-10-14 [email protected] Roll Flutter Engine from 9149a427ef98 to 472be42e90e0 (1 revision) (flutter/flutter#136574) 2023-10-14 [email protected] _RouterState should dispose created _RestorableRouteInformation. (flutter/flutter#136556) 2023-10-14 [email protected] Roll Flutter Engine from cd1a5ed6f961 to 9149a427ef98 (1 revision) (flutter/flutter#136571) 2023-10-14 [email protected] increase windows build test sharding, revert timeout 30 mins (flutter/flutter#136474) 2023-10-14 [email protected] Roll Flutter Engine from a4e62581525c to cd1a5ed6f961 (2 revisions) (flutter/flutter#136566) 2023-10-14 [email protected] Change some usage of RawKeyEvent to KeyEvent in preparation for deprecation (flutter/flutter#136420) 2023-10-13 [email protected] 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 [email protected] [Impeller] GPU frame timings summarization. (flutter/flutter#136408) 2023-10-13 [email protected] Changes WidgetInspector to use valuenotifier instead of a force rebuild for (flutter/flutter#131634) 2023-10-13 [email protected] Roll Flutter Engine from 799e8de11c12 to 6605151b4d0a (3 revisions) (flutter/flutter#136558) 2023-10-13 [email protected] Roll Flutter Engine from 93f02f7237db to 799e8de11c12 (6 revisions) (flutter/flutter#136553) 2023-10-13 [email protected] Mark leak in NativeCodec.getNextFrame. (flutter/flutter#136514) 2023-10-13 [email protected] Stop skipping leaks in the test. (flutter/flutter#136512) 2023-10-13 [email protected] run tests under dev/tools as part of framework-misc and get them passing (flutter/flutter#136501) 2023-10-13 [email protected] Roll Flutter Engine from 60968c892649 to 93f02f7237db (5 revisions) (flutter/flutter#136546) 2023-10-13 [email protected] Remove bringup from tests and move some back to presubmit. Reorganize TESTOWNERS. (flutter/flutter#136498) 2023-10-13 [email protected] [leak-tracking] Add leak tracking in test/rendering -2 (flutter/flutter#136310) 2023-10-13 [email protected] [leak-tracking] Add leak tracking in test/rendering -3 (flutter/flutter#136308) 2023-10-13 [email protected] Roll Packages from 06be4b3 to 9caa356 (3 revisions) (flutter/flutter#136538) 2023-10-13 [email protected] 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 [email protected],[email protected] 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

Comment thread packages/flutter/lib/src/widgets/app.dart Outdated

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

Comment thread packages/flutter/lib/src/widgets/app.dart Outdated

jacob314 reviewed Aug 28, 2023

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/widget_inspector.dart Outdated

jacob314 reviewed Aug 28, 2023

View reviewed changes

Comment thread packages/flutter/test/widgets/widget_inspector_test.dart

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

Daniel Chevalier 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

Comment thread packages/flutter/lib/src/widgets/app.dart Outdated

Comment thread packages/flutter/lib/src/widgets/app.dart Outdated

Comment thread packages/flutter/lib/src/widgets/app.dart Outdated

Comment thread packages/flutter/lib/src/widgets/app.dart Outdated

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

Comment thread packages/flutter/test/widgets/widget_inspector_test.dart Outdated

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

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

ac095ed Changes WidgetInspector to use valuenotifier instead of a for…

7c944bf

…ce 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

ac095ed Changes WidgetInspector to use valuenotifier instead of a for…

860339b

…ce 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

ac095ed Changes WidgetInspector to use valuenotifier instead of a for…

952e458

…ce rebuild for (flutter/flutter#131634)

Navigation Menu

Repository navigation

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 merged 31 commits into
flutter:masterfrom
CoderDake:no-force-rebuild

CoderDake commented Jul 31, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacob314 left a comment

Uh oh!

jacob314 Aug 28, 2023

Uh oh!

goderbauer Sep 11, 2023

Uh oh!

jacob314 left a comment

Uh oh!

goderbauer left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goderbauer Oct 11, 2023

Uh oh!

CoderDake Oct 12, 2023

Uh oh!

goderbauer left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

CoderDake commented Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacob314 left a comment

Choose a reason for hiding this comment

Uh oh!

jacob314 Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

goderbauer Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

jacob314 left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goderbauer Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

CoderDake Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CoderDake commented Jul 31, 2023 •

edited

Loading