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

Removing Shorcuts.of and Shortctus.maybeOf #104215

Merged

Conversation

gspencergoog
Copy link
Contributor

Description

This removes Shorcuts.of and Shortctus.maybeOf because they're not especially useful, since the only thing you can really set on a ShortcutManager is the shortcuts, and the Shortcuts widget that you give it to manages those, so if it rebuilds, it overwrites what you set.

Also, adds a Shortcuts.manager constructor and removes the manager argument to the Shortcuts widget.

Removing these will also eliminate an InheritedWidget for each Shortcuts widget, improving memory usage.

Tests

  • Update tests, added tests for Shortcuts.manager.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label May 19, 2022
@gspencergoog gspencergoog changed the title Deprecating Shorcuts.of and Shortctus.maybeOf Removing Shorcuts.of and Shortctus.maybeOf May 19, 2022
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 if this doesn't break anything.

/// [shortcuts] change materially.
/// If this widget was created with [Shortcuts.manager], the
/// [ShortcutsManager.shortcuts] will be used as the source for shortcuts. If
/// the unnamed constructor is used, this manager will be given new
Copy link
Member

Choose a reason for hiding this comment

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

this field will stay null if the unnamed constructor is used, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! yes. Duh.

@fluttergithubbot
Copy link
Contributor

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

  • The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot
Copy link
Contributor

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

  • The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot
Copy link
Contributor

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

  • The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot
Copy link
Contributor

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

  • The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

CaseyHillers added a commit that referenced this pull request May 26, 2022
dkwingsmt added a commit to dkwingsmt/flutter that referenced this pull request Jun 1, 2022
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
This removes Shorcuts.of and Shortctus.maybeOf because they're not especially useful, since the only thing you can really set on a ShortcutManager is the shortcuts, and the Shortcuts widget that you give it to manages those, so if it rebuilds, it overwrites what you set.

Also, adds a Shortcuts.manager constructor and removes the manager argument to the Shortcuts widget.

Removing these will also eliminate an InheritedWidget for each Shortcuts widget, improving memory usage.
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
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants