[go: nahoru, domu]
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move debugShowWidgetInspectorOverride #144029

Merged
merged 17 commits into from
Feb 26, 2024
Merged

Move debugShowWidgetInspectorOverride #144029

merged 17 commits into from
Feb 26, 2024

Conversation

polina-c
Copy link
Contributor
@polina-c polina-c commented Feb 23, 2024 •
edited
Loading

@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Feb 23, 2024
@polina-c polina-c marked this pull request as draft February 23, 2024 17:35
@polina-c polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Feb 23, 2024
jacob314
jacob314 reviewed Feb 23, 2024
View reviewed changes
Copy link
Contributor
@jacob314 jacob314 left a comment

Choose a reason for hiding this comment

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

Overall this is the right place for storing this per binding state but we need to make the way the state is reset to keep tests hermetic a bit cleaner.

jacob314
jacob314 approved these changes Feb 23, 2024
View reviewed changes
Copy link
Contributor
@jacob314 jacob314 left a comment

Choose a reason for hiding this comment

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

lgtm

@polina-c polina-c marked this pull request as ready for review February 23, 2024 19:20
goderbauer
goderbauer reviewed Feb 23, 2024
View reviewed changes
packages/flutter/lib/src/widgets/binding.dart Outdated Show resolved Hide resolved
packages/flutter_test/lib/src/binding.dart Outdated
Comment on lines 1181 to 1183
ServicesBinding.instance.resetInternalState();
// ignore: invalid_use_of_visible_for_testing_member
WidgetsBinding.instance.resetInternalState();
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this now execute the reset twice now?

Copy link
Contributor Author
@polina-c polina-c Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Formally no. This code prints different values:

class WidgetsBinding {
  static String instance = 'WidgetsBinding';
}


class ServicesBinding extends WidgetsBinding {
  static String instance = 'ServicesBinding';
}

void main() {
  print(WidgetsBinding.instance);
  print(ServicesBinding.instance);  
}

Practically maybe in some cases. But there may be other cases. And, if not yet, may be created in future. Will it hurt to do it twice?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the code given in the comment above prints different values since it assigns different values to the static instances. However, I don't understand how that relates to whether or not resetInternalState is executes twice? AFASIK, the way our bindings are set up, WidgetsBinding.instance and ServicesBinding.instance are assigned the same object.

Will it hurt to do it twice?

Why do more work then necessary? Isolated, this little bit may not matter. But all the little bits add up at some point and slow down test execution.

Copy link
Contributor Author
@polina-c polina-c Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Removed one call and added assert to make sure binding is still as expected by this code.

goderbauer
goderbauer approved these changes Feb 26, 2024
View reviewed changes
Copy link
Member
@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@polina-c polina-c added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 26, 2024
@auto-submit auto-submit bot merged commit 523b0c4 into flutter:master Feb 26, 2024
66 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 27, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2024
@engine-flutter-autoroll
523b0c4d8 Move debugShowWidgetInspectorOverride (flutter/flutter#144029)
4fb6841
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Feb 27, 2024
@engine-flutter-autoroll
Roll Flutter from b77560e92a58 to c30f998eb5db (12 revisions) (#6211)
bc51dea 
flutter/flutter@b77560e...c30f998

2024-02-27 jiahaog@users.noreply.github.com [flutter_tools] Fix missing stack trace from daemon (flutter/flutter#144113)
2024-02-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 04ff2868ce80 to 0bc21ea7bc92 (21 revisions) (flutter/flutter#144208)
2024-02-26 polinach@google.com Move debugShowWidgetInspectorOverride (flutter/flutter#144029)
2024-02-26 nate.w5687@gmail.com Allow `Listenable.merge()` to use any iterable (flutter/flutter#143675)
2024-02-26 54558023+keyonghan@users.noreply.github.com Mark firebase_release_smoke_test as `bringup: true` (flutter/flutter#144186)
2024-02-26 git@reb0.org refactor: Differentiate pubspec and resolution errors for plugins (flutter/flutter#142035)
2024-02-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 34a8b9bdaaac to 04ff2868ce80 (1 revision) (flutter/flutter#144159)
2024-02-26 nate.w5687@gmail.com Implementing `switch` expressions in `rendering/` (flutter/flutter#143812)
2024-02-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7a573c21a4ad to 34a8b9bdaaac (1 revision) (flutter/flutter#144147)
2024-02-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from a15326b2c439 to 7a573c21a4ad (1 revision) (flutter/flutter#144145)
2024-02-26 15619084+vashworth@users.noreply.github.com Update copyDirectory to allow links to not be followed (flutter/flutter#144040)
2024-02-26 engine-flutter-autoroll@skia.org Roll Packages from 7df2085 to 353086c (7 revisions) (flutter/flutter#144144)

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 bmparr@google.com,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

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@polina-c polina-c deleted the notifier branch February 28, 2024 00:18
LouiseHsu pushed a commit to LouiseHsu/packages that referenced this pull request Mar 7, 2024
@engine-flutter-autoroll @LouiseHsu
Roll Flutter from b77560e92a58 to c30f998eb5db (12 revisions) (flutte…
387b75f 
…r#6211)

flutter/flutter@b77560e...c30f998

2024-02-27 jiahaog@users.noreply.github.com [flutter_tools] Fix missing stack trace from daemon (flutter/flutter#144113)
2024-02-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 04ff2868ce80 to 0bc21ea7bc92 (21 revisions) (flutter/flutter#144208)
2024-02-26 polinach@google.com Move debugShowWidgetInspectorOverride (flutter/flutter#144029)
2024-02-26 nate.w5687@gmail.com Allow `Listenable.merge()` to use any iterable (flutter/flutter#143675)
2024-02-26 54558023+keyonghan@users.noreply.github.com Mark firebase_release_smoke_test as `bringup: true` (flutter/flutter#144186)
2024-02-26 git@reb0.org refactor: Differentiate pubspec and resolution errors for plugins (flutter/flutter#142035)
2024-02-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 34a8b9bdaaac to 04ff2868ce80 (1 revision) (flutter/flutter#144159)
2024-02-26 nate.w5687@gmail.com Implementing `switch` expressions in `rendering/` (flutter/flutter#143812)
2024-02-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7a573c21a4ad to 34a8b9bdaaac (1 revision) (flutter/flutter#144147)
2024-02-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from a15326b2c439 to 7a573c21a4ad (1 revision) (flutter/flutter#144145)
2024-02-26 15619084+vashworth@users.noreply.github.com Update copyDirectory to allow links to not be followed (flutter/flutter#144040)
2024-02-26 engine-flutter-autoroll@skia.org Roll Packages from 7df2085 to 353086c (7 revisions) (flutter/flutter#144144)

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 bmparr@google.com,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

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This was referenced Mar 18, 2024
Closed
Merged
auto-submit bot pushed a commit that referenced this pull request Mar 21, 2024
@passsy
Add WidgetsApp.debugShowWidgetInspectorOverride again (deprecated) (#…
4bbe6d5 
…145334)

`WidgetsApp.debugShowWidgetInspectorOverride` was replaced with ` WidgetsBinding.instance.debugShowWidgetInspectorOverrideNotifier` in #144029.

The old API was removed, not deprecated.

It is used by some [open-source projects](https://github.com/search?q=WidgetsApp.debugShowWidgetInspectorOverride&type=code), thus I'm making the effort of bringing the API back as deprecated.

Fixes #145333
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
@engine-flutter-autoroll
523b0c4d8 Move debugShowWidgetInspectorOverride (flutter/flutter#144029)
3779ce1
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@engine-flutter-autoroll @arc-yong
Roll Flutter from b77560e92a58 to c30f998eb5db (12 revisions) (flutte…
a007125 
…r#6211)

flutter/flutter@b77560e...c30f998

2024-02-27 jiahaog@users.noreply.github.com [flutter_tools] Fix missing stack trace from daemon (flutter/flutter#144113)
2024-02-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 04ff2868ce80 to 0bc21ea7bc92 (21 revisions) (flutter/flutter#144208)
2024-02-26 polinach@google.com Move debugShowWidgetInspectorOverride (flutter/flutter#144029)
2024-02-26 nate.w5687@gmail.com Allow `Listenable.merge()` to use any iterable (flutter/flutter#143675)
2024-02-26 54558023+keyonghan@users.noreply.github.com Mark firebase_release_smoke_test as `bringup: true` (flutter/flutter#144186)
2024-02-26 git@reb0.org refactor: Differentiate pubspec and resolution errors for plugins (flutter/flutter#142035)
2024-02-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 34a8b9bdaaac to 04ff2868ce80 (1 revision) (flutter/flutter#144159)
2024-02-26 nate.w5687@gmail.com Implementing `switch` expressions in `rendering/` (flutter/flutter#143812)
2024-02-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7a573c21a4ad to 34a8b9bdaaac (1 revision) (flutter/flutter#144147)
2024-02-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from a15326b2c439 to 7a573c21a4ad (1 revision) (flutter/flutter#144145)
2024-02-26 15619084+vashworth@users.noreply.github.com Update copyDirectory to allow links to not be followed (flutter/flutter#144040)
2024-02-26 engine-flutter-autoroll@skia.org Roll Packages from 7df2085 to 353086c (7 revisions) (flutter/flutter#144144)

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 bmparr@google.com,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

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacob314 jacob314 jacob314 approved these changes

@goderbauer goderbauer goderbauer approved these changes

Assignees
No one assigned
Labels
a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker a: tests "flutter test", flutter_test, or one of our tests autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@polina-c @jacob314 @goderbauer