-
-
Notifications
You must be signed in to change notification settings - Fork 22
Window created/destroyed/layout_mode_change signals #368
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
Conversation
I think it's simpler if we have just one signal, |
You are right, that would be simpler. But I still think it makes sense to add the information about what layout mode it went from. One of my use cases that I plan for this is to add idle inhibitors to windows. Currently, when you are playing wine game via xwayland there is no way to get idle inhibitor on the window. This is usually fine as you are using mouse/keyboard. But if you play a game with just controller, that doesn't register as any activity. So I want to be able to add when window goes fullscreen add idle inhibitor, when it leaves remove it. So I think it might make sense to change it to one signal, something like |
You don't need to know the old mode for this to work (that doesn't mean it won't be useful for other stuff).
I think the Close should have its own signal, too. But sending one with a empty/None new_mode is still a good idea to allow for cleanup |
src/window/layout.rs
Outdated
match old_mode.current() { | ||
crate::window::window_state::LayoutModeKind::Floating => { | ||
self.signal_state.window_unset_floating.signal(window); | ||
} | ||
crate::window::window_state::LayoutModeKind::Tiled => { | ||
self.signal_state.window_unset_tiled.signal(window); | ||
} | ||
crate::window::window_state::LayoutModeKind::Maximized => { | ||
self.signal_state.window_unset_maximized.signal(window); | ||
} | ||
crate::window::window_state::LayoutModeKind::Fullscreen => { | ||
self.signal_state.window_unset_fullscreen.signal(window); | ||
} | ||
crate::window::window_state::LayoutModeKind::Spilled => { | ||
self.signal_state.window_unset_spilled.signal(window); | ||
} | ||
} | ||
|
||
match new_mode.current() { | ||
crate::window::window_state::LayoutModeKind::Floating => { | ||
self.signal_state.window_set_floating.signal(window); | ||
} | ||
crate::window::window_state::LayoutModeKind::Tiled => { | ||
self.signal_state.window_set_tiled.signal(window); | ||
} | ||
crate::window::window_state::LayoutModeKind::Maximized => { | ||
self.signal_state.window_set_maximized.signal(window); | ||
} | ||
crate::window::window_state::LayoutModeKind::Fullscreen => { | ||
self.signal_state.window_set_fullscreen.signal(window); | ||
} | ||
crate::window::window_state::LayoutModeKind::Spilled => { | ||
self.signal_state.window_set_spilled.signal(window); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the only place the layout can change.
For example (there might be other instances):
- src/layout.rs:108
- src/api/window.rs:{443, 453, 459, 469}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, it's better to handle this from State::on_event_loop_cycle_completion()
rather than sprinkle the call to self.signal_state.window_*
.
If you need the old value, it could be held in the window LayoutMode
until the end of the loop, then from the aforementioned function, you'd iterate over windows to clear this value and send the old/new pair.
If you don't need the old value, just a boolean would work.
This should prevent missing layout changes.
(You may want to wait for Ottatop input on that first)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I think having an old_layout_mode: LayoutMode
, checking to see if the current mode != the old mode to send the signal, and copying the current mode to the old mode in on_event_loop_cycle_completion
is the simplest solution.
You do need to know when the window stops being fullscreen, so you can remove the idle inhibitor. You could track it internally in your configuration, but that would be more complicated. |
I already said that it might still be useful, but I still don't see the need for that use-case. EDIT: Just to be clear, I'm not saying the
Just having the information doesn't solves your issue cleanly, so you'd still need to track some state in your config: Let say for the sake of argument you want to use one global inhibitor if a window is full-screen, and you have this initial state:
You swap WindowA & WindowB (or fullscreen B then un-fullscreen A). The following signal will be emitted:
There is no guarantee which signal will arrive first. If WindowB signal arrive first, the following sequence will happen:
You can solve this by:
|
Don't worry, I don't mind discussing this. What I plan to do is to add a way to manually insert idle inhibitor for the window itself. I think that would be cleaner than using one global idle inhibitor. Plus, it would work better if the fullscreen surface is not visible. At that point, the idle inhibitor shouldn't probably be active. |
I think it's best to just send the new mode for now. If we send both modes then the old has to be an Option and is only None for the first signal, which I don't like. |
Ok, in that case, how about this approach. I do need to know what is the window initial layout mode when it spawns, I need to know when it changes, and when it closes. So how about I add new signals, something like window created/destroyed. These would just give you For my use case, this would make it a bit more complicated to keep track of everything in my config, but it would be doable and it would address the issue. Theoretically we wouldn't even need window created signal, as |
Yea, an open and closed signal is good. |
0860c45
to
b839510
Compare
I changed PR to hopefully address all the comments here. |
- Add new signals: WindowLayoutModeChanged, WindowCreated, WindowDestroyed - Move LayoutMode enum to util.proto for shared usage - Add tracking of window layout mode changes in on_event_loop_cycle_completion
b839510
to
2326b3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed a few types when moving LayoutMode to util, but the lua changes are fine as far as I can tell
be35085
to
4531c42
Compare
- Replace unwrap_or with if let for safer conversion in signal.rs - Move window layout mode change check to dedicated method in window.rs - Remove redundant window_created signal in xwayland.rs
4531c42
to
e9ecda7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This PR adds signals to track when windows enter or exit different layout modes (tiled, floating, maximized, fullscreen, and spilled).When a window changes from one layout mode to another, the appropriate "unset" signal is fired for the previous state, followed by the "set" signal for the new state. This allows configurations to react to both entering and exiting specific window states. The signal is also fired when the window first spawns.
This PR adds signals for window creation and destruction. It also adds a layout mode change signal. This should address all the suggestions in this PR. I am not completely sure if the destroyed signal is correct like this in regard to unmap flag. But from my testing, this gave me the results I wanted.
I would also really appreciate it if someone checked my lua changes, I have basically no knowledge of lua, so I am just copying and pasting. I think it is right, but I am not sure how to actually check.
resolves #365