Skip to content

Implement the keyboard shortcuts inhibitor protocol#42

Merged
any1 merged 2 commits intoany1:masterfrom
stacyharper:wbarraco/push-ulmtkslyyzmm
Feb 12, 2025
Merged

Implement the keyboard shortcuts inhibitor protocol#42
any1 merged 2 commits intoany1:masterfrom
stacyharper:wbarraco/push-ulmtkslyyzmm

Conversation

@stacyharper
Copy link
Contributor

@stacyharper stacyharper commented Feb 8, 2025

This is a continuation of: #5

I got good Wayland knowledge, I should be able to tackle on remaining issues, if there are.

For now I just rebased this over master, fixed conflicts, and compiled. It works so far, and use the most recent protocol.

I rebased this over master, and adapted it to handle multiple seat correctly. We inhibit shortcuts only for the seats with a pointer in the surface.

@stacyharper
Copy link
Contributor Author

@stacyharper
Copy link
Contributor Author

stacyharper commented Feb 8, 2025

I am already improving it, to support multiple seats better. I also want to handle better keyboard_enter inhibit/release. But afaik this patch works greats for now.

edit: something like this:

https://git.sr.ht/~stacyharper/wlvncc/commit/860f0b90e7927209222b91a78cb50121d1b20018

https://git.sr.ht/~stacyharper/wlvncc/commit/6d5a26d03849b9f7a6e7a7c085fe212a9b1d107d

I'll sit on this for a while

@any1
Copy link
Owner

any1 commented Feb 8, 2025

LGTM. Seats can be improved in a later PR.

There should probably be an option to enable/disable this though.

@stacyharper
Copy link
Contributor Author

stacyharper commented Feb 8, 2025

Also, I haven't discussed it yet, but we should note that <F12> can not be forwarded at all. This seems a bit odd to me, but maybe it is following some wlvncc convention? If nothing has been discussed before, maybe it is time to codify the internal keybinds? Ofc could be done later.

@stacyharper
Copy link
Contributor Author

Maybe we can wait a bit. I think I almost finish the multi-seat patch. I should probably squash them directly here?

@stacyharper stacyharper force-pushed the wbarraco/push-ulmtkslyyzmm branch from 3944c1b to 3b34fb2 Compare February 9, 2025 08:06
@stacyharper
Copy link
Contributor Author

Maybe we can wait a bit. I think I almost finish the multi-seat patch. I should probably squash them directly here?

I squashed both commits. Let's do the good job directly :)

@stacyharper stacyharper force-pushed the wbarraco/push-ulmtkslyyzmm branch from 3b34fb2 to f4a7b7f Compare February 10, 2025 10:42
@any1
Copy link
Owner

any1 commented Feb 12, 2025

I would very much prefer for this to be explicitly enabled via a command line argument.

@any1
Copy link
Owner

any1 commented Feb 12, 2025

I would very much prefer for this to be explicitly enabled via a command line argument.

Or, well, actually, this is fine as long as it's off by default.

src/inhibitor.c Outdated
bool inhibitor_init(struct shortcuts_inhibitor* self, struct wl_surface* surface,
struct wl_list* seats)
{
if ( self->surface != NULL )
Copy link
Owner

Choose a reason for hiding this comment

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

Style.

src/inhibitor.c Outdated
}
}

assert(false);
Copy link
Owner

Choose a reason for hiding this comment

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

Let's just say abort() instead of assert(false). Also, better return false to pacify the compiler or otherwise annotate this as being unreachable.

src/inhibitor.c Outdated
self->manager, self->surface, seat_inhibitor->seat->wl_seat);
return;
}
assert(false);
Copy link
Owner

Choose a reason for hiding this comment

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

abort()

src/inhibitor.c Outdated
seat_inhibitor->inhibitor = NULL;
return;
}
assert(false);
Copy link
Owner

Choose a reason for hiding this comment

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

abort()

src/inhibitor.c Outdated
struct shortcuts_seat_inhibitor* seat_inhibitor;
struct shortcuts_seat_inhibitor* tmp;

wl_list_for_each_safe(seat_inhibitor, tmp, &self->seat_inhibitors, link) {
Copy link
Owner

Choose a reason for hiding this comment

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

I would write a function that finds the first matching inhibitor based on seat and returns it. Then use that here and in all the other places. That one would be fine to assert() on.

I am one of those people who believe in writing small functions. So, it makes me happy when I see PRs following the mantra of "write small functions".

@stacyharper stacyharper force-pushed the wbarraco/push-ulmtkslyyzmm branch from f4a7b7f to 9a5fb19 Compare February 12, 2025 11:05
@stacyharper
Copy link
Contributor Author

I've done most of the requested changes.

Only, about the recommandation to disable this feature by default:

Atm there is no way to easily disable the feature. F12 disable it temporarily, but as soon as pointer leave, and enter back, the inhibitor is re-created. It looks okay to me, as an easy way to get out of a fullscreen wlvncc window by example.

I think we can easily desactivate the feature completely, by skipping the global inhibitor bind. But then there is no way to enable it later, without restarting the program. Is it okay? It is for me, I'll always use wlvncc with the extra parameter then.

@any1
Copy link
Owner

any1 commented Feb 12, 2025

Looks good now.

I think we can easily desactivate the feature completely, by skipping the global inhibitor bind. But then there is no way to enable it later, without restarting the program. Is it okay? It is for me, I'll always use wlvncc with the extra parameter then.

The problem with leaving this on by default is that it's maybe not so nice when you run an application for the first time and it does something rather intrusive like blocking your shortcuts.

@stacyharper
Copy link
Contributor Author

The problem with leaving this on by default is that it's maybe not so nice when you run an application for the first time and it does something rather intrusive like blocking your shortcuts.

Right, but only when the pointer is on the surface. It doesn't look that intrusive to me. That is also what the virtualization program does, even if the behavior is more clarified, with focus/unfocus dedicated shortcuts.

As I said, I'm also okay with adding a parameter to enable this.

@any1
Copy link
Owner

any1 commented Feb 12, 2025

As I said, I'm also okay with adding a parameter to enable this.

Well, it's settled then. :)

To do so, we also have to store how seats are associated to keyboards
and pointers.
@stacyharper stacyharper force-pushed the wbarraco/push-ulmtkslyyzmm branch from 9a5fb19 to 05afa6f Compare February 12, 2025 12:27
@stacyharper
Copy link
Contributor Author

While doing so, I figured out we was poorly handling the case the compositor was not supporting the protocol. We have to check self instead of self->manager on the inhibitor method implementations. This way we don't have to check inhibitor before every call from main.c or pointer.c

@stacyharper stacyharper force-pushed the wbarraco/push-ulmtkslyyzmm branch from 05afa6f to 1c50ee0 Compare February 12, 2025 12:30
@any1 any1 merged commit ec58d6a into any1:master Feb 12, 2025
@any1
Copy link
Owner

any1 commented Feb 12, 2025

Thanks!

@stacyharper stacyharper deleted the wbarraco/push-ulmtkslyyzmm branch February 26, 2025 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants