-
Notifications
You must be signed in to change notification settings - Fork 14
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
wayland: implement wl_touch #177
Conversation
Thanks for working on this. I'm not familiar with how other compositors implement touch input, but here are some thoughts regarding how I would imagine it: There are two touch owners: struct DefaultTouchOwner;
struct GrabTouchOwner {
node: Rc<dyn Node>,
down_ids: AHashSet<i32>,
} The DefaultTouchOwner ignores all events except The down_ids field contains all ids that are currently down. When this set becomes empty, the GrabTouchOwner replaces itself by the DefaultTouchOwner. When the GrabTouchOwner receives a The first down event should also switch the keyboard focus to that node if it is an application window. (Using the same logic used by mouse button presses.) |
Alternatively, each touch id could have its own touch owner. This seems to be how it works on android. |
I've implemented your first suggestion, what do you think? Also, I was trying to hide the cursor with |
LGTM. Have you checked how other compositors handle multi-touch?
Why do you want to hide the cursor? |
I haven't looked at the code but KWin seems to do it the way you suggested. Sway only sends touch down events that fall within the boundaries of the initial surface after the first touch down, but it always sends motion events.
KWin, Sway and Windows all hide the cursor on touch. I find it can be distracting when you're not using it. |
Maybe it's best to simply not render the cursor in that case. See Edit: But this is not going to work for hardware cursors. Maybe functions |
I'm currently working on adding tablet support. I'll continue the review once that is done. |
The tablet PR #190 has significant overlap with this MR:
|
So should I wait for you to merge that PR and then rework this one? |
Sounds good. |
#190 has been merged. |
I've now implemented all the basic features. There's still some more future work but for now I think this is enough. |
pub enum TouchEvent { | ||
Down { pos: TouchPosition }, | ||
Up, | ||
Motion { pos: TouchPosition }, | ||
Cancel, | ||
Frame, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be flattened into the InputEvent enum and the fields of TouchPosition should be flattened into the Down and Motion variants.
@@ -272,6 +289,12 @@ pub enum InputEvent { | |||
state: KeyState, | |||
}, | |||
|
|||
Touch { | |||
seat_slot: i32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be named something else due to the existing meaning of seat.
@@ -583,6 +584,63 @@ async fn run(seat_test: Rc<SeatTest>) { | |||
} | |||
println!(); | |||
}); | |||
let st = seat_test.clone(); | |||
TouchDown::handle(tc, se, (), move |_, ev| { | |||
if all || ev.seat == seat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The events should be collected and only be printed on the next frame event. Similar to the tablet frame event.
let mut capabilities = POINTER | KEYBOARD; | ||
let handlers = &self.state.input_device_handlers; | ||
for (_, d) in handlers.borrow().iter() { | ||
let dev = &d.data.device; | ||
if dev.has_capability(InputDeviceCapability::Touch) { | ||
capabilities |= TOUCH; | ||
break; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The capabilities of a seat should only consider the input devices connected to a seat. These capabilities can also change at runtime when a device is attached or detached and then they will have to be sent to the client again.
self.client.protocol_error( | ||
self, | ||
MISSING_CAPABILITY, | ||
&format!( | ||
"wl_seat {} .get_touch called when no touch capability has existed", | ||
self.id | ||
), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed.
} | ||
|
||
fn motion(&self, seat: &Rc<WlSeatGlobal>, time_usec: u64, id: i32, x: Fixed, y: Fixed) { | ||
self.down_ids.borrow_mut().insert(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed.
pub fn x(&self) -> f64 { | ||
unsafe { libinput_event_touch_get_x(self.event) } | ||
} | ||
|
||
pub fn y(&self) -> f64 { | ||
unsafe { libinput_event_touch_get_y(self.event) } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need these.
fn node_on_touch_down( | ||
self: Rc<Self>, | ||
seat: &Rc<WlSeatGlobal>, | ||
time_usec: u64, | ||
id: i32, | ||
x: Fixed, | ||
y: Fixed, | ||
) { | ||
let _ = seat; | ||
let _ = time_usec; | ||
let _ = id; | ||
let _ = x; | ||
let _ = y; | ||
} | ||
|
||
fn node_on_touch_up(self: Rc<Self>, seat: &Rc<WlSeatGlobal>, time_usec: u64, id: i32) { | ||
let _ = seat; | ||
let _ = time_usec; | ||
let _ = id; | ||
} | ||
|
||
fn node_on_touch_motion( | ||
self: Rc<Self>, | ||
seat: &WlSeatGlobal, | ||
time_usec: u64, | ||
id: i32, | ||
x: Fixed, | ||
y: Fixed, | ||
) { | ||
let _ = seat; | ||
let _ = time_usec; | ||
let _ = id; | ||
let _ = x; | ||
let _ = y; | ||
} | ||
|
||
fn node_on_touch_frame(&self, seat: &WlSeatGlobal) { | ||
let _ = seat; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should work more like node_on_axis_event
where the contents of a frame are collected first and then passed via a single call.
} | ||
} | ||
|
||
impl TouchOwnerHolder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs to handle the case where the node that we're interacting with changes its position. See WlSeatGlobal::apply_changes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What changes should be applied in such a case? There's no touch cursor, and for the duration of the touch sequence the node should stay the same no matter where it is.
Edit: And the latest node position is used for every touch down or motion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll have to store the current absolute positions of the touch ids in compositor space and then send synthetic motion events to the node when the node moves.
x: Fixed, | ||
y: Fixed, | ||
) { | ||
self.pointer_move(CursorType::Touch(id), seat.pointer_cursor(), x, y, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pointer_move
should be refactor so that the cursor is optional.
I've opened #230 to fix the remaining issues with this PR. One thing I've removed is integration with the compositor UI. There were some serious issues with multi-touch and I'm only testing this with a graphics tablet hacked to work as a touch screen, so I don't have the fine control to properly design this. This can get added back in the future. |
I'm testing this on a touchscreen laptop and trying to implement basic support:
Future work:
Closes #115