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

Add convenience accessor for primaryFocus #43859

Merged
merged 2 commits into from
Nov 1, 2019

Conversation

gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Oct 30, 2019

Description

This adds accessors for WidgetsBinding.instance.focusManager and WidgetsBinding.instance.focusManager.primaryFocus so that they can be more easily found in IDEs and accessed.

This adds a top level getter for WidgetsBinding.instance.focusManager.primaryFocus called primaryFocus, and a static accessor FocusManager.instance that returns WidgetsBinding.instance.focusManager.

Tests

  • Modified tests to use the accessors.

Breaking Change

  • No, this is not a breaking change.

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels Oct 30, 2019
Copy link
Contributor

@darrenaustin darrenaustin left a comment

Choose a reason for hiding this comment

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

Other than my concern about using the global namespace, this LGTM.

@@ -1418,6 +1424,14 @@ class FocusManager with DiagnosticableTreeMixin {
}
}

/// Provides convenient access to the current [FocusManager] from the
/// [WidgetsBinding] instance.
FocusManager get focusManager => WidgetsBinding.instance.focusManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. Did you consider using a static method on FocusManager instead of a top-level getter here? Do we have other top-level globals like this? Is there any concern about name clashing with existing app code for either of these?

Copy link
Contributor Author

@gspencergoog gspencergoog Oct 31, 2019

Choose a reason for hiding this comment

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

Well, I did consider a static, but it would have had to have a different name (because primaryFocus is taken), and it's pretty useful at the top level. There are other top level globals like these: imageCache, and defaultTargetPlatform to name a couple. Like any new name, It could clash, of course, but I don't think it's very likely to clash.

However, I think I will convert the focusManager one to a static FocusManager.instance so that it appears as the singleton that it is (but still get it internally from the WidgetsBinding.instance.focusManager). That way it's one less thing to clash.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we already have a precedence for top level names, this is probably not a big deal. Pragmatically speaking I doubt there would be much collision. I prefer not using global namespaces, but this case maybe useful enough to do so.

Or given that you already changed it to FocusManager.instance while I was typing this :), never-mind.

@gspencergoog gspencergoog changed the title Add convenience accessors for focusManager, primaryFocus Add convenience accessors for primaryFocus Oct 31, 2019
@gspencergoog gspencergoog changed the title Add convenience accessors for primaryFocus Add convenience accessor for primaryFocus Oct 31, 2019
@gspencergoog gspencergoog merged commit 3a30722 into flutter:master Nov 1, 2019
@gspencergoog gspencergoog deleted the primary_focus_getter branch November 13, 2019 23:22
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
This adds accessors for WidgetsBinding.instance.focusManager and WidgetsBinding.instance.focusManager.primaryFocus so that they can be more easily found in IDEs and accessed.

This adds a top level getter for WidgetsBinding.instance.focusManager.primaryFocus called primaryFocus, and a static accessor FocusManager.instance that returns WidgetsBinding.instance.focusManager.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants