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

[Merged by Bors] - Add Windows::get_focused(_mut) #6571

Closed
wants to merge 2 commits into from

Conversation

tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Nov 12, 2022

Add a method to get the focused window.

Use this instead of WindowFocused events in close_on_esc.
Seems that the OS/window manager might not always send focused events on application startup.

Sadly, not a fix for #5646.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Windowing Platform-agnostic interface layer to run your app in C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Nov 12, 2022
@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 13, 2022
@DJMcNab
Copy link
Member

DJMcNab commented Nov 13, 2022

I'd not looked into this before. Surely assuming that every window is focused on creation also doesn't make sense.

E.g. on systems where new windows don't get focus automatically

Something else needs to change here, I think. We need winit to provide some guidance here.

So this is still an improvement, but I think this is unviable as an end state

@tim-blackbird
Copy link
Contributor Author

tim-blackbird commented Nov 13, 2022

Thanks for pointing that out. I had thought that focused would get its initial state from winit, but it simply defaults to true.
Looked through the winit docs and focus events is all the info we have on the window's focused state, leading me to believe the linked issue is a bug in the i3 window manager.

I've changed focused to be false on window creation as true causes the following system to print '2' for a single frame when added to the multiple_windows example.

.add_system(|windows: Res<Windows>| {
    println!("{}", windows.iter().filter(|w| w.is_focused()).count())
})

@DJMcNab
Copy link
Member

DJMcNab commented Nov 13, 2022

There is the possibility that a rogue Window 'unfocused' event is getting in there (e.g. 1: true, 2: false is sent in that order), which this would handle correctly but close_on_esc currently does not.

@tim-blackbird
Copy link
Contributor Author

There is the possibility that a rogue Window 'unfocused' event is getting in there (e.g. 1: true, 2: false is sent in that order)

Surely that would be considered a bug in the window manager?

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I'm happy with this now

@bevyengine bevyengine deleted a comment from ryrony123 Nov 14, 2022
@cart
Copy link
Member

cart commented Nov 14, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 14, 2022
Add a method to get the focused window.

Use this instead of `WindowFocused` events in `close_on_esc`.
Seems that the OS/window manager might not always send focused events on application startup.

Sadly, not a fix for #5646.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
@bors
Copy link
Contributor

bors bot commented Nov 14, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Nov 14, 2022
Add a method to get the focused window.

Use this instead of `WindowFocused` events in `close_on_esc`.
Seems that the OS/window manager might not always send focused events on application startup.

Sadly, not a fix for #5646.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
@bors bors bot changed the title Add Windows::get_focused(_mut) [Merged by Bors] - Add Windows::get_focused(_mut) Nov 14, 2022
@bors bors bot closed this Nov 14, 2022
@tim-blackbird tim-blackbird deleted the focused-window branch November 24, 2022 11:22
taiyoungjang pushed a commit to taiyoungjang/bevy that referenced this pull request Dec 15, 2022
Add a method to get the focused window.

Use this instead of `WindowFocused` events in `close_on_esc`.
Seems that the OS/window manager might not always send focused events on application startup.

Sadly, not a fix for bevyengine#5646.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
Add a method to get the focused window.

Use this instead of `WindowFocused` events in `close_on_esc`.
Seems that the OS/window manager might not always send focused events on application startup.

Sadly, not a fix for bevyengine#5646.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
Add a method to get the focused window.

Use this instead of `WindowFocused` events in `close_on_esc`.
Seems that the OS/window manager might not always send focused events on application startup.

Sadly, not a fix for bevyengine#5646.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants