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

fix: Focus handling with a scope on the GameWidget #1725

Merged
merged 3 commits into from
Oct 25, 2022
Merged

Conversation

renancaraujo
Copy link
Member

Description

This ignores keyboard events if the game widget doesn't have the primary focus. This allows focus nodes on overlay widgets to take precedence on keyboard resolving.

Also, add focus scope in the game widget for when the overlay is removed, the focus goes back to the game.

Also, add tests for GameWidget for all these cases and some more.

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

Related Issues

This is related to:

@renancaraujo renancaraujo marked this pull request as ready for review October 24, 2022 12:35
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Nice!!

@spydon spydon requested review from st-pasha and a team October 24, 2022 12:42
Copy link
Member

@erickzanardo erickzanardo left a comment

Choose a reason for hiding this comment

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

One nit, but LGTM! Can we have a more contextual PR title though?

Comment on lines +249 to +250
// ignore: avoid_redundant_argument_values
autofocus: true,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// ignore: avoid_redundant_argument_values
autofocus: true,

Copy link
Member

Choose a reason for hiding this comment

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

It's pretty good to keep this imho, it gives the reader of the test more context of what is being tested.

Copy link
Member Author

@renancaraujo renancaraujo Oct 24, 2022

Choose a reason for hiding this comment

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

Disagree with the suggestion, that is to explicitly show what that test tests.

Co-authored-by: Lukas Klingsbo <me@lukas.fyi>
@renancaraujo renancaraujo changed the title fix: focus stuff fix: focus handling with a scope Oct 24, 2022
@spydon spydon changed the title fix: focus handling with a scope fix: Focus handling with a scope on the GameWidget Oct 25, 2022
@spydon spydon enabled auto-merge (squash) October 25, 2022 08:55
@spydon spydon merged commit d1cd851 into main Oct 25, 2022
@spydon spydon deleted the renan/fix-focus-stuff branch October 25, 2022 09:02
Hwan-seok pushed a commit to Hwan-seok/flame that referenced this pull request Jan 5, 2023
This ignores keyboard events if the game widget doesn't have the primary focus. This allows focus nodes on overlay widgets to take precedence on keyboard resolving.

Also, add focus scope in the game widget for when the overlay is removed, the focus goes back to the game.

Also, add tests for GameWidget for all these cases and some more.
Hwan-seok pushed a commit to Hwan-seok/flame that referenced this pull request Jan 5, 2023
This ignores keyboard events if the game widget doesn't have the primary focus. This allows focus nodes on overlay widgets to take precedence on keyboard resolving.

Also, add focus scope in the game widget for when the overlay is removed, the focus goes back to the game.

Also, add tests for GameWidget for all these cases and some more.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants