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 WindowBuilder::with_active #2585

Merged
merged 3 commits into from
Jan 27, 2023

Conversation

amrbashir
Copy link
Contributor

@amrbashir amrbashir commented Dec 6, 2022

  • Tested on all platforms changed
  • 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@e42ff07

Fixes #2580

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.

I'm fine with the change, however we should solve the focusing on startup issue first. Given that you don't add any new events, I'd assume that you rely on window being focused.

See this issue
#1077

I'd advise to add a synthetic event(Basically send Focused(false)/Focused(true) where it makes sense) on platform where window is focused/unfocused by default.

By default we should also state that the window is unfocused.

@amrbashir
Copy link
Contributor Author

I'm fine with the change, however we should solve the focusing on startup issue first. Given that you don't add any new events, I'd assume that you rely on window being focused.

I think on all platforms, a newly spawned window always gains focus, and this method mainly exists to stop that behavior which is useful for a widgets/overlay app where you don't necessarily need focus and merely want to show the window on Top with some content.

I'd advise to add a synthetic event(Basically send Focused(false)/Focused(true) where it makes sense) on platform where window is focused/unfocused by default.

I am okay with synthesizing a Focus event, should that be implemented in another PR or in this one?

By default we should also state that the window is unfocused.

I think the opposite is true, no? a window by default gains focus, unless with_active is used with false?
or did you mean that users should base their logic on the window not having focus and handle the synthesized Focus event instead?

@kchibisov
Copy link
Member

I think on all platforms, a newly spawned window always gains focus, and this method mainly exists to stop that behavior which is useful for a widgets/overlay app where you don't necessarily need focus and merely want to show the window on Top with some content.

That's not true, on Wayland the window is unfocused by default, we have a workaround for that. For X11 it could be the same actually, and I think on macOS as well. On macOS you should be under certain conditions though.

I think the opposite is true, no? a window by default gains focus, unless with_active is used with false?
or did you mean that users should base their logic on the window not having focus and handle the synthesized Focus event instead?

The default should be false, since unmapped window isn't focused.

What happens when you try to create a new window while being in a full-screen mode? I think in that case new window will be unfocused and in the background, no?

I am okay with synthesizing a Focus event, should that be implemented in another PR or in this one?

I think the only option is to synthesis the Focused(true).

@amrbashir
Copy link
Contributor Author

amrbashir commented Dec 20, 2022

Looking at #1077, using winit's master branch, I get the correct events order on Windows that matches x11 here #1077 (comment) so I guess no work needed on those two platforms.
Unfortunately, I won't be able to test on macOS or Wayland so I think this should be fixed in another PR, is this considered a hard blocker?

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

So what I think we should do is to:

  1. Say in docs for Window::new that Focus state of the window will be delivered via explicit event. And say that initial state of the window is unfocused...
  2. Emit explicitly whether the window is focused or not from the new (this is done on Wayland already).

So if the new window is always focused then you send explicit focus, if it's not focused on start or may not be focused you send unfocused, but be careful here, since we don't want to end up unfocused.

@madsmtm do you know the focus situation on macOS on start? I'd assume it's always focused, no? So we'd need an explicit focus.

@amrbashir
Copy link
Contributor Author

If I understand this correctly the flow should be:

  1. create window (unfocused)
  2. emit Focused(false) event
  3. create second window (unfocused)
  4. emit Focused(false) event
  5. emit Focused(true) event for the first window

@kchibisov
Copy link
Member

emit Focused(true) event for the first window

For the one that is focused by the os. But yes, something like that. If you can guarantee that Os will send Focused(true) then it's fine, I guess.

@amrbashir
Copy link
Contributor Author

amrbashir commented Jan 2, 2023

I tested on Windows and according to this comment #1077 (comment) for X11 , that seems to be the current behavior and you said that Wayland synthesize this so only macOS is left which I cannot test.

@amrbashir amrbashir force-pushed the feat/show-no-activate branch from 8f7c4e0 to 4fa2925 Compare January 2, 2023 15:08
@kchibisov
Copy link
Member

@madsmtm ping wrt macOS.

@kchibisov kchibisov mentioned this pull request Jan 17, 2023
11 tasks
@madsmtm
Copy link
Member

madsmtm commented Jan 18, 2023

windowDidBecomeKey: is run regardless of how the window became focused, so the Focused(_) event is correctly tracked on macOS.

The window is assumed to be unfocused by default, so if we need to match the other platforms here, we should emit a Focused(false) event once the window is created (a simple AppState::queue_event(...) inside Window::new should suffice).

@madsmtm
Copy link
Member

madsmtm commented Jan 18, 2023

I've pushed a fix that does this now

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.

This looks fine to me.

(Note that, like many things, it won't work on macOS if done outside the event loop, see the readme. But that's fine for now).

src/window.rs Outdated Show resolved Hide resolved
@kchibisov kchibisov force-pushed the feat/show-no-activate branch from 6d5cc89 to d26c160 Compare January 19, 2023 21:44
@kchibisov
Copy link
Member

I tested on Windows and according to this comment #1077 (comment) for X11 , that seems to be the current behavior and you said that Wayland synthesize this so only macOS is left which I cannot test.

I've retested on X11 and it wasn't like that. Though, I've pushed a fix for that. Be aware that we can't use CreateNotify given that for whatever reason Xorg doesn't bother sending it (Or maybe it's an XWayland bug?).

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.

I've added X11 impl and slightly changed the docs for the with_active. Should be good now.

@kchibisov kchibisov force-pushed the feat/show-no-activate branch 2 times, most recently from 2d03de2 to 714a503 Compare January 27, 2023 02:33
@kchibisov kchibisov force-pushed the feat/show-no-activate branch from 714a503 to e1db81c Compare January 27, 2023 04:24
@kchibisov kchibisov force-pushed the feat/show-no-activate branch from e1db81c to db71a49 Compare January 27, 2023 04:39
@kchibisov kchibisov merged commit b457329 into rust-windowing:master Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Possibility of supporting the ability to make a window visible but not active?
4 participants