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.

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

Layer-shell: don't drop focus when clicking layer-shell #770

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+47 −18
Open

Layer-shell: don't drop focus when clicking layer-shell #770

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 47 additions & 18 deletions src/input/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,17 +793,27 @@ impl State {
// (in case of a window the toplevel) surface for the focus.
// see: https://gitlab.freedesktop.org/wayland/wayland/-/issues/294
if !seat.get_pointer().unwrap().is_grabbed() {
enum NewFocusTarget {
Change(KeyboardFocusTarget),
Remove,
KeepOld,
}
use NewFocusTarget::*;
let output = seat.active_output();

let pos = seat.get_pointer().unwrap().current_location().as_global();
let relative_pos = pos.to_local(&output);
let mut under: Option<KeyboardFocusTarget> = None;
let under: NewFocusTarget;

if let Some(session_lock) = shell.session_lock.as_ref() {
under = session_lock
under = match session_lock
.surfaces
.get(&output)
.map(|lock| lock.clone().into());
.map(|lock| lock.clone().into())
{
Some(target) => Change(target),
None => Remove,
};
} else if let Some(window) =
shell.active_space(&output).get_fullscreen()
{
Expand All @@ -820,12 +830,15 @@ impl State {
)
.is_some()
{
under = Some(layer.clone().into());
under = Change(layer.clone().into());
} else {
under = Remove;
Copy link
Member

Choose a reason for hiding this comment

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

Honestly I think this case should be KeepOld as well. It is probably highly confusing for users for the full-screen app to loose keyboard focus, even if they clicked on an Overlay layer surface.

}
} else {
under = Some(window.clone().into());
under = Change(window.clone().into());
}
} else {
let target: NewFocusTarget;
let done = {
let layers = layer_map_for_output(&output);
if let Some(layer) = layers
Expand All @@ -838,24 +851,32 @@ impl State {
})
{
let layer_loc = layers.layer_geometry(layer).unwrap().loc;
if layer.can_receive_keyboard_focus()
&& layer
if !layer.can_receive_keyboard_focus() {
target = KeepOld;
true
} else {
if layer
.surface_under(
relative_pos.as_logical() - layer_loc.to_f64(),
WindowSurfaceType::ALL,
)
.is_some()
{
under = Some(layer.clone().into());
true
} else {
false
{
target = Change(layer.clone().into());
true
} else {
target = Remove;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is somewhat confusing to read. We set the target to Remove implying the focus to be taken away, but since we return false, we never really evaluate target.

Can we please just make target mut and default initialize it with Remove and then skip these lines? (same goes for line 873)

Copy link
Author

Choose a reason for hiding this comment

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

Tbh I found the entire function confusing. It's not explained what the "done" is and why it's necessary. Without knowing that, I settled on doing a mechanical translation.
I decided to drop the "mut" because it makes the choices implicit when they are not altered, making the purpose of each branch harder to figure out (need to backtrack).
I think the it can be structured better, rather than fal but I'd need a deeper explanation.

Copy link
Member

@Drakulix Drakulix Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fair.

I don't think done is used to great extent any more and is mostly an artifact of previous code. It was introduced as part of this commit to break apart a never ending cascade of if-else statements to figure out the focus target. The previous approach had the problem of never continuing down the tree, if any layer-surface was in the way, no matter if it actually had an input_region there or even took keyboard-focus.

I decided to drop the "mut" because it makes the choices implicit when they are not altered, making the purpose of each branch harder to figure out (need to backtrack).

Also fair, but in that case I feel like we might want to combine done and target (possibly also under), as the state is far too spread out at the moment imo.

I think the it can be structured better, rather than fal but I'd need a deeper explanation.

I hope that is helpful, do you have any further questions? Do you feel like tackling this or would you prefer me doing that?

Copy link
Author

Choose a reason for hiding this comment

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

I can do it, but I'm away for a week soon.

Copy link
Member

Choose a reason for hiding this comment

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

I can do it, but I'm away for a week soon.

No rush. I'll ping you, if I happen to find some time to look into this earlier than you do.

false
}
}
} else {
target = Remove;
false
}
};
if !done {
under = if done {
target
} else {
// Don't check override redirect windows, because we don't set keyboard focus to them explicitly.
// These cases are handled by the XwaylandKeyboardGrab.
if let Some(target) = shell.element_under(pos, &output) {
Expand Down Expand Up @@ -955,7 +976,7 @@ impl State {
}
}
}
under = Some(target);
Change(target)
} else {
let layers = layer_map_for_output(&output);
if let Some(layer) = layers
Expand All @@ -981,14 +1002,22 @@ impl State {
)
.is_some()
{
under = Some(layer.clone().into());
Change(layer.clone().into())
} else {
Remove
}
};
} else {
Remove
}
}
}
};
}
std::mem::drop(shell);
Shell::set_focus(self, under.as_ref(), &seat, Some(serial));
match under {
Change(new_focus) => { Shell::set_focus(self, Some(&new_focus), &seat, Some(serial)); },
Remove => { Shell::set_focus(self, None, &seat, Some(serial)); },
KeepOld => {},
};
} else {
std::mem::drop(shell);
}
Expand Down
Loading