Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

Proposal for more options regarding keyboard interactivity for layer-shell clients #100

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

dkondor
Copy link
Contributor

@dkondor dkondor commented Nov 25, 2020

I'm opening this as a concrete proposal for #32

I believe the current protocol makes it hard to have layer-shell surfaces in the top layer (e.g. panels and docks) that have keyboard interaction on their main surface and not in popups.

Let me know if I should provide more motivation or use cases.

My intention for this is to be backward-compatible with the current protocol (0/1 values for keyboard interactivity keep their meaning).

@dkondor
Copy link
Contributor Author

dkondor commented Nov 27, 2020

wlroots branch for this (including updated layer-shell example):
https://github.com/dkondor/wlroots/tree/keyboard_interactivity_options2

Wayfire branch:
https://github.com/dkondor/wayfire/tree/layer_shell_keyboard

gtk-layer-shell branch (including updated demo):
https://github.com/dkondor/gtk-layer-shell/tree/new_keyboard_interactivity

example: cairo-dock branch using this to allow shift + click to start a new instance of a running program:
https://github.com/dkondor/cairo-dock-core/tree/layer_shell_keyboard_interactivity
(needs the updated gtk-layer-shell as well)

Copy link
Member

@ammen99 ammen99 left a comment

Choose a reason for hiding this comment

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

A few small nitpicks. Aside from that, LGTM, and I think this is a necessary addition if we want to grow layer-shell into the equivalent of _NET_WM_STATE_ABOVE and similar.

unstable/wlr-layer-shell-unstable-v1.xml Outdated Show resolved Hide resolved
unstable/wlr-layer-shell-unstable-v1.xml Outdated Show resolved Hide resolved
unstable/wlr-layer-shell-unstable-v1.xml Outdated Show resolved Hide resolved
@dkondor
Copy link
Contributor Author

dkondor commented Dec 16, 2020

Hi,
I'm wondering if there is a change to get this merged (or similar functionality -- I'm happy to work on alternate solutions as well). Let me know if I should open separate PRs with this functionality for wlroots and Wayfire (based on the above branches) or if I should give a longer overview of my motivation of this (extending the discussion in #32 ).

@ammen99
Copy link
Member

ammen99 commented Dec 18, 2020

@dkondor You should open PRs at least in wlroots, so that the implementation can be reviewed (this is the usual procedure for protocols, protocol is drafted, then implementation, and then both are reviewed at the same time to ensure theory matches practice)

@dkondor
Copy link
Contributor Author

dkondor commented Dec 18, 2020

@dkondor You should open PRs at least in wlroots, so that the implementation can be reviewed (this is the usual procedure for protocols, protocol is drafted, then implementation, and then both are reviewed at the same time to ensure theory matches practice)

Ok, sure :)

<description summary="request exclusive keyboard focus">
Request exclusive keyboard focus if this surface is above the shell surface layer.

For layers above the shell surface layers, the seat will always give
Copy link
Member

Choose a reason for hiding this comment

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

This sounds a little bit like this is writing down compositor policy. I'd like to make it clear that compositors can decide to implement a different policy, e.g. don't give focus to any keyboard-interactive layer surface when a lockscreen is active.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it seems like the wording comes from the original description for set_keyboard_interactivity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for this option, I took the current wording :)

I agree there is no guarantee to e.g. prevent stealing the focus from a lock screen either in the current protocol or this PR. I feel this could be a separate issue though.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you're right. We can fix it separately.

will receive keyboard focus (if ever).

For layers below the shell surface layer, the seat will use
normal focus semantics.
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem like this is backwards-compatible. I think e.g. Sway never gives focus in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Then sway isn't compliant with the current version of the protocol ;)
https://github.com/swaywm/wlr-protocols/blob/master/unstable/wlr-layer-shell-unstable-v1.xml#L209

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, you're correct. This makes the change in this PR look less scary, because it just allows top/overlay layers to get the same behaviour as bottom/background layers which have keyboard interactivity set to 1.

I'd still prefer to allow compositors to not give focus if they don't want to, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm changing this to optional. With this change, clients that want normal focus can just use the next option.

<description summary="request exclusive keyboard focus">
Request exclusive keyboard focus if this surface is above the shell surface layer.

For layers above the shell surface layers, the seat will always give
Copy link
Member

Choose a reason for hiding this comment

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

This is the first time this protocol refers to a concept of "shell surface layer", I think? I wouldn't want to force compositors to have a single "shell surface layer" (which contains the regular toplevel surfaces if I understand your intention?).

The existing paragraph about the layer enum just describes what a compositor typically does. Maybe a compositor has an always-on-top setting for a toplevel and will put the toplevel between the top and the overlay layer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe instead of "shell surface layer", it could say explicitly that this refers to the top and overlay layers?

Copy link
Member

Choose a reason for hiding this comment

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

This sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if there could be more explicit wording (1) specifying whether the compositor is allowed to put non-layer-shell surfaces in any of the layers in the "layer" enum; and (2) noting that compositors will have additional "layers" beside the ones listed in the layer enum, that have a well-defined z-order relative to them (so e.g. if a fullscreen toplevel is above the "top" layer, a layer shell surface in the top layer will not receive exclusive keyboard focus).

prompt.
</description>
</entry>
<entry name="user" value="2">
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshedding time: maybe we can come up with a better name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but so far I couldn't think of anything better :)

Copy link
Member

Choose a reason for hiding this comment

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

regular? on_demand? (just ideas)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like on_demand

@emersion emersion force-pushed the layer_shell_keyboard_interactivity branch 3 times, most recently from b6e4bb1 to 5630799 Compare January 6, 2021 13:18
@emersion
Copy link
Member

emersion commented Jan 6, 2021

I've taken the liberty to split your changes into 4 logical commits, to avoid having a single mega-commit that does many different things at once.

I've also changed these things:

  • Removed the since argument for the error. Protocol errors aren't versioned.
  • Moved the since argument from the enum to the on_demand entry. The other entries can be used with previous versions of the protocol.

Are you okay with these changes?

@dkondor
Copy link
Contributor Author

dkondor commented Jan 6, 2021

Sure, it all seems good. Thanks!

Currently only background and bottom layers can have surfaces with
normal focus semantics. Allow top and overlay layers to have these
semantics too (instead of exclusive).

Closes: swaywm#32
@emersion emersion force-pushed the layer_shell_keyboard_interactivity branch from 5630799 to f12bdac Compare January 6, 2021 16:40
@emersion
Copy link
Member

emersion commented Jan 6, 2021

OK, I've pushed the clarifications and minor improvements to master so that this PR only contains the new on_demand enum value.

This PR LGTM, I'll have a look at the compositor and client implementations linked in #100 (comment).

@emersion
Copy link
Member

emersion commented Jan 6, 2021

Can you open pull requests for these various branches? Would make it easier to add comments.

@dkondor
Copy link
Contributor Author

dkondor commented Jan 7, 2021

wlroots PR: swaywm/wlroots#2555
wayfire PR: WayfireWM/wayfire#962 (merged originally, as it should not break things)

I've just opened a PR for gtk-layer-shell:
wmww/gtk-layer-shell#105

Copy link
Contributor

@wmww wmww left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Thanks!

@emersion emersion merged commit d1598e8 into swaywm:master Jan 12, 2021
@dkondor dkondor deleted the layer_shell_keyboard_interactivity branch January 13, 2021 03:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants