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.

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

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

Merged
merged 28 commits into from
Aug 24, 2022

Conversation

Drakulix
Copy link
Member

@Drakulix Drakulix commented Aug 8, 2022

Continuation from #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.)

}

fn serialize_pressed_keys(keys: Vec<u32>) -> Vec<u8> {
let serialized = unsafe { ::std::slice::from_raw_parts(keys.as_ptr() as *const u8, keys.len() * 4) };
Copy link
Member

Choose a reason for hiding this comment

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

Could this unsafe be eliminated?

Copy link
Member Author

Choose a reason for hiding this comment

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

What are you suggesting? As far as I know, this is the recommended way to transmute a Vector.

Copy link
Member

@elinorbgr elinorbgr Aug 22, 2022

Choose a reason for hiding this comment

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

Except for the part that it does not compile because slice::from_raw_parts produces a &[T], not a Vec<T> 😅

but I suppose this is fixed in a later commit

Copy link
Member

Choose a reason for hiding this comment

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

We could use something like bytemuck and shift the blame elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

We could use something like bytemuck and shift the blame elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could use something like bytemuck and shift the blame elsewhere.

I mean in general... yes, but for this one case... really?

@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2022

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%) ⬇️
wlcs-output 10.47% <9.42%> (-0.26%) ⬇️
wlcs-pointer-input 29.63% <23.12%> (-0.40%) ⬇️

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%) ⬆️
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.

@Drakulix Drakulix force-pushed the feature/input_split_2 branch 2 times, most recently from 320388c to 7d0b31e Compare August 12, 2022 10:33
@elinorbgr
Copy link
Member

elinorbgr commented Aug 22, 2022

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.

@Drakulix Drakulix force-pushed the feature/input_split_2 branch from 7d0b31e to 771edca Compare August 22, 2022 16:41
@Drakulix Drakulix force-pushed the feature/input_split_2 branch from bb060bf to 50dfc57 Compare August 23, 2022 12:05
@Drakulix Drakulix force-pushed the feature/input_split_2 branch 3 times, most recently from c05c3f2 to 71a2fff Compare August 23, 2022 14:24
@Drakulix Drakulix force-pushed the feature/input_split_2 branch from 71a2fff to 66d004f Compare August 23, 2022 14:37
@Drakulix Drakulix requested a review from i509VCB August 23, 2022 14:48
Comment on lines +165 to +166
let focus = surface.and_then(|s| dh.get_client(s.id()).ok());
let focus2 = surface.and_then(|s| dh.get_client(s.id()).ok());
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 a workaround for Client not being clone right?

Looks like it could be clonable:

#[derive(Debug)]
pub struct Client {
    pub(crate) id: ClientId,
    pub(crate) data: Arc<dyn ClientData>,
}

@vberger Is there a hidden reason behind this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think @vberger already acknowledged, that Client could and should be Clone. This is mostly just a matter of somebody has to do that.

@Drakulix Drakulix force-pushed the feature/input_split_2 branch from 9806c31 to 34e162a Compare August 24, 2022 11:37
@Drakulix Drakulix merged commit 5511a93 into master Aug 24, 2022
@Drakulix Drakulix deleted the feature/input_split_2 branch February 22, 2023 18:58
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.

5 participants