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.

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

A bunch of cleanups and a missing ShortcutRegistar in WidgetsApp #104560

Merged
merged 4 commits into from May 25, 2022

Conversation

gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented May 24, 2022

Description

A bunch of random cleanup things I found while doing MenuBar development.

  • Changes an if test to an assert in binding.dart, since the if should always be true.
  • Adds the default ShortcutRegistrar that should have been in the ShortcutRegistry PR.
  • Moves a debug message in the FocusManager to print the result after the focus change instead of before.
  • Reorders the test parameters in theme_data_test.dart to match the order of the theme data fields everywhere else.

Tests

  • Added a test for the missing ShortcutRegistar in WidgetsApp.

@flutter-dashboard flutter-dashboard bot added f: focus Focus traversal, gaining or losing focus f: gestures flutter/packages/flutter/gestures repository. f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 24, 2022
@@ -1730,7 +1730,9 @@ class _WidgetsAppState extends State<WidgetsApp> with WidgetsBindingObserver {
actions: widget.actions ?? WidgetsApp.defaultActions,
child: FocusTraversalGroup(
policy: ReadingOrderTraversalPolicy(),
child: child,
child: ShortcutRegistrar(
Copy link
Contributor

Choose a reason for hiding this comment

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

This change does not look like a clean up. Is it intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I see you mentioning it in the PR. Maybe this should be the title of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, it's not really a "cleanup", but something that was missing from the previous PR that I committed with the ShortcutRegistry. Without this, people will have to add their own registrar manually.

I've added a test to make sure it exists.

@gspencergoog gspencergoog changed the title A bunch of cleanups found while doing MenuBar development. A bunch of cleanups and a missing ShortcutRegistar in WidgetsApp May 24, 2022
Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

LGTM

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Linux framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux framework_tests_widgets has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_tests_0 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_tests_1 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_tests_2 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_tests_4 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_tests_5 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_tests_6 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_canvaskit_tests_0 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_canvaskit_tests_2 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_canvaskit_tests_4 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_canvaskit_tests_5 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux web_canvaskit_tests_7_last has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Mac framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Mac framework_tests_widgets has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Windows framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Windows framework_tests_widgets has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite framework_tests-widgets-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite framework_tests-libraries-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

@gspencergoog gspencergoog merged commit 0a417c3 into flutter:master May 25, 2022
@gspencergoog gspencergoog deleted the menu_bar_extras branch May 25, 2022 16:32
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 25, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 25, 2022
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
…lutter#104560)

A bunch of random cleanup things I found while doing MenuBar development.

Changes an if test to an assert in binding.dart, since the if should always be true.
Adds the default ShortcutRegistrar that should have been in the ShortcutRegistry PR.
Moves a debug message in the FocusManager to print the result after the focus change instead of before.
Reorders the test parameters in theme_data_test.dart to match the order of the theme data fields everywhere else.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: focus Focus traversal, gaining or losing focus f: gestures flutter/packages/flutter/gestures repository. 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.

None yet

4 participants