input/tablet: Take Seat<Self> and TabletToolHandle in tablet_tool_image()

[ids1024]

Description

Trying to properly implement tablet tool cursor images for cosmic-comp, I wonder if a TabletToolDescriptor is the most useful thing to have as an argument. Left as a draft while I still figure out how to use this, and may want other changes. (Perhaps having userdata for the tool handle would be a good place to store the cursor?.)

I see TabletToolTarget also takes TabletToolDescriptor instead of TabletToolHandle. While TabletToolGrab instead takes a TabletToolInnerHandle.

Checklist

• I agree to smithay's Developer Certificate of Origin.

[Ph4ntomas]

I wonder if a TabletToolDescriptor is the most useful thing to have as an argument. [...]
(Perhaps having userdata for the tool handle would be a good place to store the cursor?.)

I don´t think it's a good idea. Other handles don't have an associated userdata (the cursor image for pointer is stored within the seat), and accessing the userdata might be problematic if cursor_image is called while the handle is locked. It's not currently done, but there is a precedent in PointerHandle:

smithay/src/input/pointer/mod.rs

Lines 814 to 817 in 347b2b3

	} else if let Some((old_focus, _)) = self.focus.take() {
	old_focus.leave(seat, data, event.serial, event.time);
	data.cursor_image(seat, CursorImageStatus::default_named());
	}

One goal I had was for the TabletSeatHandler API to match SeatHandler one, but it seems I forgot to pass the Seat as an argument to the cursor_image function. Adding the Seat as a parameter does make sense, since it avoid the boiler-plate of retrieving it, and is already done by the TabletToolHandle.

I see TabletToolTarget also takes TabletToolDescriptor instead of TabletToolHandle. While TabletToolGrab instead takes a TabletToolInnerHandle.

This was made on purpose to match the other *Target and *Grab implementations.

Grabs are meant to alter the behavior before events are forwarded to clients, and they are called by the handle so they need a locked version which is held by the InnerHandle.

Target on the other hand are meant to forward the event as-is and only abstract the underlying object (whether it's a wayland client interface, or something internal to the compositor). They should not get access to the TabletToolHandle since using it there would deadlock, and none of the *Target get access to the *InnerHandle, so I kept a similar API.

[ids1024]

I don´t think it's a good idea. Other handles don't have an associated userdata (the cursor image for pointer is stored within the seat), and accessing the userdata might be problematic if cursor_image is called while the handle is locked.

I'm not sure if it's a good idea, but the difference is that there is at most one pointer or keyboard (etc.) on a given seat, while there may (technically) be any number of tablet tools at a given time. So have a per-tool udata avoids needing to have an extra list of tools that needs to be cleaned up as tools are removed. (Which is mildly annoying, but not really an issue.)

This was made on purpose to match the other *Target and *Grab implementations.

I mean that the other implementations use a "handle" for *Target and an "inner handle" for *Grab. While tablet currently matches this for the latter, but not for targets.

I haven't looked into how the tablet code is currently handling the locks here, but it should be possible to behave the same way as pointer/keyboard/touch.

[Ph4ntomas]

So have a per-tool udata avoids needing to have an extra list of tools that needs to be cleaned up as tools are removed. (Which is mildly annoying, but not really an issue.)

I understand (and somewhat agree with the annoying part). I was just pointing out that the function may be called from the locked handle, so having a UserDataMap in the TabletToolHandle would cause more issues than it solves. IMO it would be best for you to store it in the TabletSeat or Seat directly, as is currently being done by cosmic for the pointer.

I mean that the other implementations use a "handle" for *Target

Unless you're speaking of another kind of target, they don't. All of them are called from their respective *Internal methods, which is stored behind a Mutex inside the *Handle. Passing the *Handle to the *Target would just be error prone, as you'd get an object you can't use without causing a deadlock.

PointerTarget gets the Seat, SeatHandler and event:

smithay/src/input/pointer/mod.rs

Lines 98 to 107 in 7ddcd17

	pub trait PointerTarget<D>: IsAlive + fmt::Debug + Send
	where
	D: SeatHandler,
	{
	/// A pointer of a given seat entered this handler
	fn enter(&self, seat: &Seat<D>, data: &mut D, event: &MotionEvent);
	/// A pointer of a given seat moved over this handler
	fn motion(&self, seat: &Seat<D>, data: &mut D, event: &MotionEvent);
	/// A pointer of a given seat that provides relative motion moved over this handler
	fn relative_motion(&self, seat: &Seat<D>, data: &mut D, event: &RelativeMotionEvent);

As does the TouchTarget:

smithay/src/input/touch/mod.rs

Lines 201 to 209 in 7ddcd17

	pub trait TouchTarget<D>: IsAlive + fmt::Debug + Send
	where
	D: SeatHandler,
	{
	/// A new touch point has appeared on the target.
	///
	/// This touch point is assigned a unique ID. Future events from this touch point reference this ID.
	/// The ID ceases to be valid after a touch up event and may be reused in the future.
	fn down(&self, seat: &Seat<D>, data: &mut D, event: &DownEvent);

The KeyboardTarget does get a 'Handle', but that's not a handle on the device, but on Keysyms:

smithay/src/input/keyboard/mod.rs

Lines 36 to 53 in 7ddcd17

	pub trait KeyboardTarget<D>: IsAlive + fmt::Debug + Send
	where
	D: SeatHandler,
	{
	/// Keyboard focus of a given seat was assigned to this handler
	fn enter(&self, seat: &Seat<D>, data: &mut D, keys: Vec<KeysymHandle<'_>>, serial: Serial);
	/// The keyboard focus of a given seat left this handler
	fn leave(&self, seat: &Seat<D>, data: &mut D, serial: Serial);
	/// A key was pressed on a keyboard from a given seat
	fn key(
	&self,
	seat: &Seat<D>,
	data: &mut D,
	key: KeysymHandle<'_>,
	state: KeyState,
	serial: Serial,
	time: u32,
	);

EDIT: For completeness sake, Smithay's impl for the *Target trait does retrieve the handle, but access private data behind another lock, as does the tablet code. The public interface doesn't expose the *Handle as it's generally not a good idea for Smithay's consumer to use it.

To go back to your original use-case, I guess you could add the UserDataMap to the TabletToolHandle in such a way that Smithay's dependent can access it while the tool is otherwise locked, but that would still give the wrong impression that the Handle is safe to use.

[ids1024]

PointerTarget gets the Seat, SeatHandler and event:
...

Ah right. I guess TabletToolTarget is consistent with other targets already then, using &Seat<D> and &TabletToolDescriptor.

To go back to your original use-case, I guess you could add the UserDataMap to the TabletToolHandle in such a way that Smithay's dependent can access it while the tool is otherwise locked, but that would still give the wrong impression that the Handle is safe to use.

Yeah, UserDataMap uses interior mutability so it could be stored in SeatRc outside of the mutex. Locking shouldn't really be an issue, though it might be odd if it isn't possible/easy to access the userdata in TabletToolTarget.

I'll probably just add it to the seat user data in cosmic-comp.