Skip to content

Add Window::has_focus #2531

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.

Sign up for GitHub

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

Jump to bottom
Merged
kchibisov merged 1 commit into from
Jan 17, 2023
Merged

Add Window::has_focus #2531

kchibisov merged 1 commit into from
Jan 17, 2023

Conversation

@amrbashir
Copy link
Contributor

@amrbashir amrbashir commented Oct 26, 2022 •
edited by madsmtm
Loading

  • Tested on all platforms changed
    • Windows
    • X11
    • Wayland
    • Web
    • Android
    • iOS
    • macOS
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Porting tauri-apps/tao@7d2eeee

@amrbashir amrbashir mentioned this pull request Oct 26, 2022
Open
22 tasks
madsmtm
madsmtm approved these changes Nov 2, 2022
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Thanks, looks good, only a few minor nits.

@kchibisov
Copy link
Member

kchibisov commented Nov 3, 2022

I think it's dangerous to add this with blanket true impl to be honest, since users my start relying on it. Given that you simply want to get the last value of Focused(changed) event we should have the Api on all the platform implemented from the start.

@amrbashir
Copy link
Contributor Author

amrbashir commented Nov 4, 2022

Unfortunately, I don't have much experience with other platforms so I can't implement them, however I may be able to implement x11 if you steer me into the right direction.

@kchibisov
Copy link
Member

kchibisov commented Nov 4, 2022

@amrbashir I mean it's not about knowing though. You can see where we sent Fosused event and stash its result on a window, it's just rust and doesn't require extra knowledge at all. For example on Wayland you'd need to pass some Arc<AtomicBool> around, on X11 can likely something similar or maybe you can query things directly.

There're no platform specifics at all in the end.

@amrbashir
Copy link
Contributor Author

amrbashir commented Nov 4, 2022

cool, thought there might be a way to query the OS like macOS impl for example.

@kchibisov
Copy link
Member

kchibisov commented Nov 4, 2022

Maybe on X11, but be aware that active window isn't keyboard focused window sometimes, since you can hover the window with mouse and it'll be active, but won't have focus.

@amrbashir
Copy link
Contributor Author

amrbashir commented Nov 4, 2022 •
edited
Loading

alright, x11, wayland and web, are implemented now but I was not able to test wayland.

@amrbashir amrbashir changed the title On Windows and MacOS, add Window::has_focus Add Window::has_focus Nov 4, 2022
@amrbashir amrbashir force-pushed the feat/has-focus branch 2 times, most recently from 754075f to fd1ffc3 Compare November 4, 2022 21:49
kchibisov
kchibisov requested changes Nov 5, 2022
View reviewed changes
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

While there's a way to know whether the window is focus or not on ios/Android, I guess the issue was that it's unclear how to hook into the current handlers, which is sort of understandable.

Would leave that to android and ios maintainers.

src/platform_impl/linux/x11/window.rs
max_inner_size: None,
resize_increments: None,
base_size: None,
has_focus: true,
Copy link
Member

@kchibisov kchibisov Nov 5, 2022

Choose a reason for hiding this comment

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

Shouldn't this be false?

Copy link
Contributor Author

@amrbashir amrbashir Nov 5, 2022

Choose a reason for hiding this comment

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

doesn't the window start initially with keyboard focus?

Copy link
Member

@madsmtm madsmtm Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

On macOS, the window is initially not focused (and window.has_focus() returns false), but it receives a WindowEvent::Focused shortly after launching (which also makes window.has_focus() return true)

Copy link
Contributor Author

@amrbashir amrbashir Nov 23, 2022

Choose a reason for hiding this comment

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

This only affects x11, on macOS we query the OS directly. The reason I made x11 start with true is because Windows also starts with true and I think x11 is very similar to Windows in this matter, but I haven't had the chance to test x11 (I may test it later today).

@amrbashir
Copy link
Contributor Author

amrbashir commented Nov 5, 2022

Android and iOS are now implemented, although I am not sure if a global state on Android is the way to go.

@kchibisov kchibisov requested a review from MarijnS95 November 20, 2022 13:04
kchibisov
kchibisov approved these changes Nov 20, 2022
View reviewed changes
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

LGTM.

Would like to hear from android maintainers wrt the change and tracking
focus in a global state. @MarijnS95 or @msiglreith.

Also, you'd need a rebase, I guess.

@MarijnS95
Copy link
Member

MarijnS95 commented Nov 20, 2022 •
edited
Loading

Android can support multiple windows (called Activitys) so this should not go into a static global.

EDIT: I haven't checked what the right/updated way is to do this since #2444.

@amrbashir
Copy link
Contributor Author

amrbashir commented Nov 20, 2022 •
edited
Loading

Android can support multiple windows (called Activitys) so this should not go into a static global.

I am aware of that but glancing over the code, the events are dispatched with a dummy window id and there is no way to differentiate between different activity events.

@MarijnS95
Copy link
Member

MarijnS95 commented Nov 20, 2022 •
edited
Loading

That's unfortunate to hear @amrbashir, I thought @rib's solution in #2444 would implicitly address this; or at least his crate opens the door for multi-window (multiple Activity instances) setup as that's relevant to some of my use-cases going forward.

Regardless, I thought at least in the past we had a per-window struct/storage location where I stored the RawWindowHandle lock.
Looks like @rib removed that (probably for good reason, though I have not had the opportunity to look at any of the code in detail yet), and looking back it seems to have been in EventLoop which is already a better place than a global static (but not per-Window, as I remembered), allowing for easier move to a per-window struct when we get to finally implementing this.

madsmtm
madsmtm approved these changes Nov 22, 2022
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Haven't tested the iOS impl, but is very trivial, so accepted from my side. macOS also looks good, thanks for your work @amrbashir!

@kchibisov kchibisov added this to the Version 0.28 milestone Dec 24, 2022
@kchibisov
Copy link
Member

kchibisov commented Jan 15, 2023

@amrbashir could you rebase it?

@amrbashir amrbashir requested a review from jackpot51 as a code owner January 16, 2023 19:59
@amrbashir
Copy link
Contributor Author

amrbashir commented Jan 16, 2023

Should be fine now

@kchibisov kchibisov merged commit a88d2e0 into rust-windowing:master Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@madsmtm madsmtm madsmtm approved these changes

@kchibisov kchibisov kchibisov approved these changes

@msiglreith msiglreith Awaiting requested review from msiglreith

@francesca64 francesca64 Awaiting requested review from francesca64

@MarijnS95 MarijnS95 Awaiting requested review from MarijnS95

@jackpot51 jackpot51 Awaiting requested review from jackpot51

Assignees

No one assigned

Labels

None yet

Milestone

Version 0.28

Development

Successfully merging this pull request may close these issues.

4 participants

@amrbashir @kchibisov @MarijnS95 @madsmtm