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 window.level to set window level (Normal, AlwaysOnTop). #8269

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

davehorner
Copy link

Great software. Thank you.

Copy link
Member

@chrisduerr chrisduerr left a comment

Choose a reason for hiding this comment

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

Why do you want this in Alacritty? Why are you adding things that you confirmed yourself to not be working?

@nixpulvis
Copy link
Contributor

FWIW, I could see using this, though it sounds like it's not supported on Wayland which is unfortunate.

This is another example of a feature that could probably be solved by the window manager as well. But we have configurations for things like the window position and default window mode, so I think this fits in pretty naturally.

@chrisduerr
Copy link
Member

But we have configurations for things like the window position and default window mode, so I think this fits in pretty naturally.

Window positioning controls were a mistake and would not exist in Alacritty if it was written today.

@nixpulvis
Copy link
Contributor

Window positioning controls were a mistake and would not exist in Alacritty if it was written today.

Yea I agree with that.

But I still think this fits in somewhat well with window mode, opacity, etc.

@davehorner
Copy link
Author

Why do you want this in Alacritty? Why are you adding things that you confirmed yourself to not be working?

I understand some people don't want this stuff; others do. I made a contribution. It's not finished. I guess I just have to come with everything finished? I saw a thing I wanted to do and I did it.

@nixpulvis
Copy link
Contributor

I guess I just have to come with everything finished?

Personally, I like the GitHub draft PRs for things like this. That way it's clear it's not done yet.

@davehorner
Copy link
Author

davehorner commented Oct 25, 2024

my apologies, I can pull it if I'm out of place with an draft change. I think I should have just left a link on the closed discussion.

with regard to the why; people have to do acrobatics to accomplish input/window orchestration.
rust-windowing/winit#1944 -timing ontop and not is really where we end up and its not pretty.
then people want to toggle in realtime. I could see this being called something else and support something like 1944 which is also in winit.

what people are trying to do is just not possible on some platforms and where it is; it would be nice to have.

make two windows move relative to each other on modern linux (Wayland)...wmctrl and windows managers don't have the capabilities for it today. try to get alacritty in a list of terminals from wmctrl -l; they aren't listed. people want to say those things are WM problems and concerns. I'd like to be able to position windows; show me how to do so using the tools we're given today?

I don't know why the window positioning was a mistake but it enables possibilities that aren't available otherwise.

@chrisduerr chrisduerr marked this pull request as draft October 25, 2024 20:00
@chrisduerr
Copy link
Member

Feel free to mark it as ready for review once you think it's ready. If you don't plan on getting it to the finish line, please close the PR.

@davehorner
Copy link
Author

Let me know what you think about expanding it to an WindowLevelStrategy or WindowLevelFocus or a different option entirely or....or maybe winit#1944 not a good idea. maybe too much for those who dislike these sorts of concerns being in their terminal.

its not only about ontop but also about focus control. I will try and figure out the PR problems this weekend.

@davehorner
Copy link
Author

davehorner commented Oct 26, 2024

I spent a few hours on it last night and a few hours this morning trying to get AlwaysOnBottom to work. I am not sure what is going on. I modified winit locally to debug and I am seeing the proper constants being set.

running cargo run -- -o window.window_level="'Normal'"
winit::window::WindowAttributes::with_window_level Normal
winit::window::init TOP false
winit::window::init BOTTOM false
on taskbar
winit::set_window_level level=TOP false
winit::set_window_level level=BOTTOM false
cargo run -- -o window.window_level="'AlwaysOnTop'"
winit::window::WindowAttributes::with_window_level AlwaysOnTop
winit::window::init TOP true
winit::window::init BOTTOM false
on taskbar
winit::WindowFlags::diff.intersects insects WindowFlags::ALWAYS_ON_TOP(true) or WindowFlags::ALWAYS_ON_BOTTOM(false)
HWND_TOPMOST
winit::set_window_level level=TOP true
winit::set_window_level level=BOTTOM false
cargo run -- -o window.window_level="'AlwaysOnBottom'"
self.window_level.unwrap_or_default()=AlwaysOnBottom
winit::window::WindowAttributes::with_window_level AlwaysOnBottom
self.window_level.unwrap_or_default()=AlwaysOnBottom
winit::window::WindowAttributes::with_window_level AlwaysOnBottom
winit::window::init TOP false
winit::window::init BOTTOM true
on taskbar
winit::WindowFlags::diff.intersects insects WindowFlags::ALWAYS_ON_TOP(false) or WindowFlags::ALWAYS_ON_BOTTOM(true)
HWND_BOTTOM
winit::set_window_level level=TOP false
winit::set_window_level level=BOTTOM true

I'm stumped and I'm actually not interested in this feature in particular. I removed the enum for AlwaysOnBottom.

This PR can be merged for AlwaysOnTop support.
If the lack of OnBottom means it doesn't go; that's fine. I had fun spending time with alacritty.

@davehorner davehorner changed the title #8126 Always on top feature, update winit to 30.5, Window level Always OnTop, Start on Bottom, or Normal #8126 Always on top feature, update winit to 30.5, Window level Always OnTop or Normal Oct 26, 2024
@davehorner davehorner marked this pull request as ready for review October 26, 2024 15:27
@davehorner davehorner changed the title #8126 Always on top feature, update winit to 30.5, Window level Always OnTop or Normal #8126 Always on top feature, update winit to 30.5, Window level Always OnTop Oct 26, 2024
@davehorner
Copy link
Author

i don't own a mac to test; I will look for a friend, or let me know what you find if you've got one.

Copy link
Member

@chrisduerr chrisduerr left a comment

Choose a reason for hiding this comment

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

Missing a changelog entry.

alacritty/src/display/window.rs Outdated Show resolved Hide resolved
alacritty/src/config/window.rs Outdated Show resolved Hide resolved
alacritty/src/config/window.rs Outdated Show resolved Hide resolved
alacritty/src/config/window.rs Show resolved Hide resolved
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.

You should document your config option inside the alacritty.5.scd.

alacritty/src/config/window.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
alacritty/src/config/window.rs Show resolved Hide resolved
@davehorner
Copy link
Author

davehorner commented Nov 9, 2024

The PR I'm submitting doesn't include toggling in realtime; I'd like to see that in the future. Not sure how to do that; it looks like IPC is not supported in windows.

rust-windowing/winit#1944 -timing ontop and is really where we end up and its not pretty. then people want to toggle in realtime.

I see we just changed focus behavior on mac.
39ea727

        // Always focus new windows, even if no Alacritty window is currently focused.
        #[cfg(target_os = "macos")]
        window.focus_window();

Why is this only for MacOs? I think this should be something under config option?

@kchibisov
Copy link
Member

Why is this only for MacOs? I think this should be something under config option?

other platforms behave sane here, so they don't need such thing.

@davehorner
Copy link
Author

davehorner commented Nov 9, 2024

other platforms behave sane here, so they don't need such thing.

stealing focus might be an OK default on mac and not needed on other platforms.
Might it be nice to have an option to steal focus when desired on those platforms that are sane?
Or to allow for not focus'ing at all.

This focus_window() may cause issues with AlwaysOnBottom. I'm not sure if focusing brings to the top or not.
It would be interesting to see a window start on bottom and focus while still under all the other windows.

I suspect there is something similar downstream from the window creation that is bringing the window above it's HWND_BOTTOM placement.

@kchibisov
Copy link
Member

If you open a window yourself, it should be focused by default. On macOS it's not the case in some cases, so you have to find where this window got opened.

The main problem is that you do it, and you don't have any idea where the window is, since macOS thinks that it shouldn't be focused.

@davehorner
Copy link
Author

davehorner commented Nov 9, 2024

I see complicated and there's all sorts of history...
rust-windowing/winit#1764 - request_user_attention

"un-requesting" user attention seems like a rare use case

start ontop without focus, or ontop with focus for a period and drop back to HWND_BOTTOM placing focus back to source.

as you said, open a window and focus is given by default. tracking window focus source to "un-request" user attention isn't easy; it seems there's been some thought into it. Hard coding a focus_window() on mac would make ontop without focus not possible if I understand correctly.

@kchibisov
Copy link
Member

set_focus is only called once you built a window, once you focus something else, it'll lose focus. It's only a matter whether you have initial focus or not here on macOS when you run it in some weird way. I really don't see what's the problem is here and what breaks, adding options for every setting is not an option though.

User attention has nothing to do with focus or anything, it's an indicator that something happened in the app and it should be focused.

@davehorner
Copy link
Author

I think limiting the change to AlwaysOnTop reduces some of my concerns about focus and placement interacting.
I can add AlwaysOnBottom back; it adds some complexity and its not working now.

@davehorner
Copy link
Author

davehorner commented Nov 17, 2024

I have a mac now. Starting cargo run -- -o window.level="'AlwaysOnBottom'"
it shows a running icon in the dock, if I try to show all windows it says "No Available Windows".
you get initial focus and you can get focus by clicking on the application icon; but I do not know how to make the terminal visible. Seems not a z-order placement but hidden or no window at all.

AlwaysOnTop runs fine in my tests. If the behavior is so radically different (no window)...I'd prefer calling that "Hidden" over "AlwaysOnBottom" and making it work the same across windows and mac. Or, not exposing it as I had proposed to limit this chase.

I wasn't looking to contribute AlwaysOnBottom or implement a custom Hidden. If you want it to provide it for others to use OK - a hidden but focusable terminal is interesting idea, but that's something only for the mac users to explore. The behavior out of the box doesn't match the label on the tin though -- that option is not well defined or working.

I don't think AlwaysOnBottom should continue blocking the PR. I'd revert c75b011. I'd rather not contribute code that has divergent behavior across platforms for the same option. This PR is three weeks old and I'd like to spend time on other things. Please don't gate this over AlwaysOnBottom; I don't have a use for it and there might be better uses of my time.

@davehorner davehorner changed the title #8126 Always on top feature, update winit to 30.5, Window level Always OnTop window.level sets preferred window level (Normal, AlwaysOnTop). Nov 17, 2024
@davehorner davehorner changed the title window.level sets preferred window level (Normal, AlwaysOnTop). Add window.level to set window level (Normal, AlwaysOnTop). Nov 17, 2024
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.

4 participants