[PATCH v2 weston 01/13] tablet: Add initial tablet support to weston

[Peter Hutterer]

On Sat, Apr 30, 2016 at 05:57:21PM +0800, Jonas Ådahl wrote:
> On Wed, Feb 03, 2016 at 03:28:05PM +1000, Peter Hutterer wrote:
> > From: Stephen Chandler Paul <thatslyude at gmail.com>
> > 
> > Introduces two new structs, weston_tablet and weston_tablet_tool with the
> > respective information as it's used on the protocol.
> > 
> > Note that tools are independent of tablets, many tools can be used across
> > multiple tablets.
> > 
> > The nesting on the protocol level requires a global tablet manager, a tablet
> > seat nested into weston_seat. The list of tablets and tools are also part of
> > the weston_seat.
> > 
> > Most functions are stubs except for the actual tablet and tablet tool
> > creation and removal.
> > 
> > Co-authored-by: Peter Hutterer <peter.hutterer at who-t.net>
> > Signed-off-by: Stephen Chandler Paul <thatslyude at gmail.com>
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> > Changes since v1:
> > - fixed freeing of tablets and tools, previous version caused the occasional
> >   segfault when a client released the resource. Split now into unlinking the
> >   tablet (during destroy) and actual freeing (during resource cleanup)
> > - shared code for initialization of tablets/tools, was duplicated before
> > 
> >  Makefile.am           |  13 +-
> >  configure.ac          |   2 +-
> >  src/compositor.c      |   1 +
> >  src/compositor.h      |  98 ++++++++++++
> >  src/input.c           | 416 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/libinput-device.c | 187 +++++++++++++++++++++++
> >  src/libinput-device.h |   4 +-
> >  7 files changed, 716 insertions(+), 5 deletions(-)
> > 

[...]

> > @@ -404,6 +405,34 @@ struct weston_touch {
> >  	uint32_t grab_time;
> >  };
> >  
> > +struct weston_tablet_tool {
> > +	struct weston_seat *seat;
> > +	enum zwp_tablet_tool_v1_type type;
> > +
> > +	struct wl_list resource_list;
> > +
> > +	struct wl_list link;
> > +
> > +	uint64_t serial;
> > +	uint64_t hwid;
> 
> Isn't this wacom specific?

yes. And now we're in the tricky territory of real implementation vs
reference implementation. Yes, it is wacom-specific but we need to cache it
for the protocol anyway. I've renamed this to wacom_hwid now and changed the
conditions to only send the event when the value is nonzero (same for the
serial)

[...]
> > @@ -348,6 +356,79 @@ handle_touch_frame(struct libinput_device *libinput_device,
> >  	notify_touch_frame(seat);
> >  }
> >  
> > +static void
> > +handle_tablet_proximity(struct libinput_device *libinput_device,
> > +			struct libinput_event_tablet_tool *proximity_event)
> > +{
> > +	struct evdev_device *device;
> > +	struct weston_tablet *tablet;
> > +	struct weston_tablet_tool *tool;
> > +	struct libinput_tablet_tool *libinput_tool;
> > +	enum libinput_tablet_tool_type libinput_tool_type;
> > +	uint32_t serial, type;
> > +	uint32_t time;
> > +	bool create = true;
> > +
> > +	device = libinput_device_get_user_data(libinput_device);
> > +	time = libinput_event_tablet_tool_get_time(proximity_event);
> > +	libinput_tool = libinput_event_tablet_tool_get_tool(proximity_event);
> > +	serial = libinput_tablet_tool_get_serial(libinput_tool);
> > +	libinput_tool_type = libinput_tablet_tool_get_type(libinput_tool);
> > +
> > +	tool = libinput_tablet_tool_get_user_data(libinput_tool);
> > +	tablet = device->tablet;
> > +
> > +	if (libinput_event_tablet_tool_get_proximity_state(proximity_event) ==
> > +	    LIBINPUT_TABLET_TOOL_PROXIMITY_STATE_OUT) {
> > +		notify_tablet_tool_proximity_out(tool, time);
> > +		return;
> > +	}
> > +
> > +	switch (libinput_tool_type) {
> > +	case LIBINPUT_TABLET_TOOL_TYPE_PEN:
> > +		type = ZWP_TABLET_TOOL_V1_TYPE_PEN;
> > +		break;
> > +	case LIBINPUT_TABLET_TOOL_TYPE_ERASER:
> > +		type = ZWP_TABLET_TOOL_V1_TYPE_ERASER;
> > +		break;
> > +	default:
> > +		fprintf(stderr, "Unknown libinput tool type %d\n",
> > +			libinput_tool_type);
> > +		return;
> > +	}
> > +
> 
> nit: would be clearer if you set create = false; here.

that would actually change the logic. Right now, for unknown tools we just
bail out and ignore the tool (yeah, could do more here). Whereas with a
create = false we'd still send the proximity event - for a tool we never
created. also, right now we probably crash if we get a prox out from such a
tool, I'll fix that up too.

> 
> > +	wl_list_for_each(tool, &device->seat->tablet_tool_list, link) {
> > +		if (tool->serial == serial && tool->type == type) {
> 
> What about the case where serial == 0, but they are different tools?

libinput promises that cannot happen. if the serial is 0, the types may be
different but we (and the hardware and the kernel) cannot distinguish
between multiple tools, they'll all look the same to us.

No comments on all the rest, amended locally as requested/needed. Thanks for
the review, much appreciated.

Cheers,
   Peter