smithay icon indicating copy to clipboard operation
smithay copied to clipboard

input: Split off input-handling and wayland-specific seat handling

Open Drakulix opened this issue 3 years ago • 1 comments

Continuation from https://github.com/Smithay/smithay/pull/575

Ready for review and testing with smallvil and anvil.

This is the second attempt at decoupling a large portion of the wayland/seat module from WlSurface as the only object to receive any input events and in the progress making the seat-abstractions usable without the wayland_frontend feature.

Motivation

Currently all custom input-handling code has to be build from ground-up in downstream compositors. smithay either provides raw input events or converts those (almost automatically, except for focus handling) into appropriate wayland-events.

There is no middle ground, where some pre-processing can take place other components could benefit from. Especially implementing window-decorations on the server-side is tedious currently. Custom shells are a giant pain.

The plan with this is to allow custom elements to be attached to a Window or even a Space, that can handle input events or trigger grabs like any wayland client inside smithay.

Drawbacks

  • Some motion-calls that set and explicit None focus, have to provide a *Handler-type (e.g. WlSurface to satisfy rustc type checks)..
  • The *Handler-traits have some cluncky as_any, same_handler_as and clone_handler functions to work around issues of object-safety and dynamic dispatch with Any, PartialEq and Clone.

Open Questions

  • [ ] Is a new toplevel input module a good place to put this?
    • Output still lives in crate::wayland, should we give it the same treatment (given that it is also used for rendering in desktop).
    • Should more types from backend::input moved into here? (Those don't require any feature to be enabled right now and will always be available.)

Drakulix avatar Aug 08 '22 17:08 Drakulix

Codecov Report

Merging #689 (f3fdee8) into master (3834958) will decrease coverage by 0.40%. The diff coverage is 23.12%.

@@            Coverage Diff             @@
##           master     #689      +/-   ##
==========================================
- Coverage   30.39%   29.98%   -0.41%     
==========================================
  Files          94       97       +3     
  Lines       14919    15196     +277     
==========================================
+ Hits         4535     4557      +22     
- Misses      10384    10639     +255     
Flag Coverage Δ
wlcs-core 27.36% <13.37%> (-0.59%) :arrow_down:
wlcs-output 10.47% <9.42%> (-0.26%) :arrow_down:
wlcs-pointer-input 29.63% <23.12%> (-0.40%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
anvil/src/drawing.rs 0.00% <ø> (ø)
anvil/src/input_handler.rs 11.15% <0.00%> (+0.04%) :arrow_up:
src/backend/input/mod.rs 0.00% <ø> (ø)
src/desktop/popup/grab.rs 0.00% <0.00%> (ø)
src/desktop/popup/mod.rs 0.00% <0.00%> (ø)
src/input/keyboard/keymap_file.rs 88.46% <ø> (ø)
src/input/keyboard/modifiers_state.rs 0.00% <0.00%> (ø)
src/lib.rs 100.00% <ø> (ø)
src/utils/alive_tracker.rs 100.00% <ø> (ø)
src/utils/mod.rs 0.00% <ø> (ø)
... and 44 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov-commenter avatar Aug 09 '22 09:08 codecov-commenter

Ok, so as a first pass, high-level review (mostly from cloning the PR, running cargo doc and looking at the result), the most obvious thing I see is that we have some seriously confusing names:

  • KeyboardHandler is a trait that describes objects that can receive keyboard events
  • KeyboardHandle is an object that represents a logical keyboard, that the compositor can use to send events to be processed by whatever has the focus

In addition, the documentation of KeyboardHandle describes it as "An handle to a keyboard handler", which it is definitely not given what KeyboardHandler has been defined to be. Overall, I think we're seriously over-using the words "handler/handle" in Smithay (me included), and we should try to find other names when possible, to make matters less confusing.

My suggestion would be to rename the KeyboardHandler trait to something more descriptive to what it is, like KeyboardEventTarget maybe?

(All the same applies to PointerHandler)

Other than this big thing, I quite like the overall shape of this PR. I'll take some time to get a little more nitpicky on the details.

elinorbgr avatar Aug 22 '22 10:08 elinorbgr