[PATCH wayland-protocols] Add the tablet protocol

[Daniel Stone]

Peter, Stephen,

On 6 November 2015 at 04:24, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> This is the revamped version of the tablet protocol for graphics tablets
> (e.g. Wacom tablets). Too many changes from the last version (a year ago or
> so), so I won't detail them, best to look at it with fresh eyes.

Thanks a lot for doing this! Looks really good to me, and I think
getting it in wayland-protocols would be good.

Do you have other implementations, e.g. GTK+/Mutter? If so, a pointer
(ha) to those would be great.

Obligatory nitpicks follow:

> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="wayland_tablet_unstable_v1">
> +  <description summary="Wayland protocol for graphics tablets">
> +    This description provides a high-level overview of the interplay between
> +    the interfaces defined this protocol. For details, see the protocol
> +    specification.
> +
> +    More than one tablet may exist, and device-specifics matter. Tablets are
> +    not represented by a single virtual device like wl_pointer. A client
> +    binds to the tablet manager object which is just a proxy object. From
> +    that, the client requests wp_tablet_manager.get_tablet_seat(wl_seat)
> +    and that returns the actual interface that has all the tablets. With
> +    this indirection, we can avoid merging wp_tablet into the actual wayland
> +    protocol, a long-term benefit.

Yes, it turns out the wl_seat.get_* model was probably a pretty bad
anti-pattern. Oh well. To avoid too much object proliferation though,
how about something like:
<interface name="wp_tablet_manager">
  <request name="add_seat">
    <description summary="Add seat of interest">
      Add a seat of interest to this tablet manager. The client will
receive events for all tablets currently on this seat, as well as
tablets added to this seat in future.
    </description>
    <arg name="seat" type="object" interface="wl_seat"/>
  </request>
  [inline wp_tablet_seat here ...]
</interface>

Then you can just bin wp_tablet_seat.

Not a pattern we've previously employed, but I think it's a good one
that avoids one extra step of indirection, as well as making it a bit
easier for clients to do the right thing.

> +    The wp_tablet_seat sends a "tablet added" for each tablet connected.
> +    That event is followed by descriptive events about the hardware;
> +    currently that includes events for name, vid/pid and
> +    internal/external/display type, and a wp_tablet.path event that
> +    describes a local path. This path can be used to uniquely identify a
> +    tablet, or get more information through libwacom.  Emulated or nested
> +    tablets can skip any of those, e.g. a virtual tablet may not have a
> +    vid/pid. The sequence of descriptive events is terminated by a
> +    wp_tablet.done event to signal that a client may now finalize any
> +    initialization for that tablet.

Is VID/PID in USB space? (Answers in diff form preferred.)

> +    Two special events (that don't exist in X) are down and up. They signal
> +    "tip touching the surface". For tablets without real proximity
> +    detection, the sequence is: proximity_in, motion, down, frame.
> +
> +    When the tool leaves proximity, a proximity_out event is sent, followed
> +    by button release events for any button still down. This signals to
> +    the client that the buttons were held when the tool left proximity.
> +    Those events are all sent within the same frame.

Can we please enforce a strict bracketing of down/up occurring in
matched pairs inside of proximity_in/out? So, the total event stream
would be:
  - proximity_in
    - motion
    - down
      - motion/motion/button/...
    - up
  - proximity_out

I assume the implementation does this, but it would be nice to have
encoded in the protocol.

> +    If the tool moves out of the surface but stays in proximity (i.e.
> +    between windows), compositor-specific grab policies apply. This usually
> +    means that the proximity-out is delayed until all buttons are released.

If no buttons are down, we'd _expect_ to just get a proximity_out
immediately, right? It almost seems like we'd want a subtype argument
here, enumerating what happened: did the user lift the tool entirely
(actually out of proximity), or is the tablet still in proximity but
just wandered off to another surface?

The doc for proximity_out also doesn't contradict this, but reading
the two together is a bit jarring:
> + [wp_tablet_tool1::proximity_out]
> +       When the tablet tool leaves proximity of the tablet, button release
> +       events are sent for each button that was held down at the time of
> +       leaving proximity. These events are sent in the same wp_tablet.frame
> +       of the proximity_out event.

It'd be nice to have discussion of the two cases - genuinely leaving
proximity vs. retaining proximity but leaving surface focus -
together.

> +    Moving a tool physically from one tablet to the other has no real effect
> +    on the protocol, since we already have the tool object from the "tool
> +    added" event. All the information is already there and the proximity_in
> +    is all a client needs to reconstruct what happened.

... also (up+)proximity_out from the old tablet.

> +  <interface name="zwp_tablet_tool1" version="1">
> +    <description summary="a physical tablet tool">
> +      An unique object that represents a physical tool that has been, or is
> +      currently in use with a tablet in this seat. Each wp_tablet_tool
> +      object stays valid until the client destroys it; the compositor
> +      reuses the wp_tablet_tool object to indicate that the object's
> +      respective physical tool has come into proximity of a tablet again.
> +      A client must not call wp_tablet_tool.destroy until it receives a
> +      wp_tablet_tool.release event.
> +
> +      A wp_tablet_tool object's uniqueness depends on the tablet's ability
> +      to report serial numbers. If the tablet doesn't support this
> +      capability, then the tool cannot be guaranteed to be unique.

'Unique' is a little confusing - perhaps 'a one-to-one mapping between
tool objects and physical tools' or similar. Also if we're not
providing some kind of tablet caps event, that you can't query this.
It's also pretty contradictory with the first sentence. :) A
'generally' there would be enough to weasel out of it.

> +    <request name="set_cursor">
> +      <description summary="set the tablet tool's surface">
> +       Sets the surface of the cursor used for this tool on the given
> +       tablet. The surface is only shown when this tool is in proximity of
> +       this tablet. If the surface is NULL, the pointer image is hidden
> +       completely.
> +
> +       The parameters hotspot_x and hotspot_y define the position of the
> +       pointer surface relative to the pointer location. Its top-left corner
> +       is always at (x, y) - (hotspot_x, hotspot_y), where (x, y) are the
> +       coordinates of the pointer location, in surface local coordinates.

(Something something transforms on the cursor surface ...)

> +    <request name="destroy" type="destructor">
> +      <description summary="destroy the tool object">
> +       This destroys the client's resource for this tool object.
> +
> +       A client must not issue this request until it receives a
> +       wp_tablet_tool.remove event.

Egads. What happens when a client no longer wants to know about tablet
events (e.g. you have an embedded drawing program in a browser but
then close the tab): it just has to ignore the stream of events
forever? A bit hostile.

> +    <enum name="type">
> +      <description summary="a physical tool type">
> +       Describes the physical type of a tool. The physical type of a tool
> +       generally defines its base usage.
> +
> +       The mouse tool represents a mouse-shaped tool that is not a relative
> +       device but bound to the tablet's surface, providing absolute
> +       coordinates.
> +
> +       The lens tool is a mouse-shaped tool with an attached lens to
> +       provide precision focus.
> +      </description>
> +      <entry name="pen" value="0x140" summary="Pen"/>
> +      <entry name="eraser" value="0x141" summary="Eraser"/>
> +      <entry name="brush" value="0x142" summary="Brush"/>
> +      <entry name="pencil" value="0x143" summary="Pencil"/>
> +      <entry name="airbrush" value="0x144" summary="Airbrush"/>
> +      <entry name="finger" value="0x145" summary="Finger"/>
> +      <entry name="mouse" value="0x146" summary="Mouse"/>
> +      <entry name="lens" value="0x147" summary="Lens"/>
> +    </enum>

*raises eyebrow* - magic numbers from Wacom protocol? (Fine if they are.)

> +    <event name="serial_id">
> +      <description summary="unique serial number of the tool">
> +       If the tool can be identified by a unique 64-bit serial number, this
> +       event notifies the client of the serial number.
> +
> +       If multiple tablets are available in the same seat and the tool is
> +       uniquely identifiable by the serial number, that tool may move
> +       between tablets.
> +
> +       Otherwise, if the tool has no serial number and this event is
> +       missing, the tool is tied to the tablet it first comes into
> +       proximity with. Even if the physical tool is used on multiple
> +       tablets, separate wp_tablet_tool objects will be created, one per
> +       tablet.
> +
> +       This event is sent in the initial burst of events before the
> +       wp_tablet_tool.done event.
> +      </description>
> +      <arg name="serial_id_msb" type="uint" summary="the unique serial number of the tool, most significant bits"/>
> +      <arg name="serial_id_lsb" type="uint" summary="the unique serial number of the tool, least significant bits"/>
> +    </event>

Would be really nice to not use 'serial' in the name, though using it
in the description absolutely makes sense. Just want to avoid
confusion with serial-as-place-in-event-stream-identifier ...

> +    <enum name="capability">
> +      <description summary="capability flags for a tool">
> +       Describes extra capabilities on a tablet.
> +
> +       Any tool must provide x and y values, extra axes are
> +       device-specific.
> +      </description>
> +      <entry name="tilt" value="1" summary="Tilt axes"/>
> +      <entry name="pressure" value="2" summary="Pressure axis"/>
> +      <entry name="distance" value="3" summary="Distance axis"/>
> +    </enum>
> +
> +    <event name="capability">
> +      <description summary="tool capability notification">
> +       This event notifies the client of any capabilities of this tool,
> +       beyond the main set of x/y axes and tip up/down detection.
> +
> +       One event is sent for each extra capability available on this tool.
> +
> +       This event is sent in the initial burst of events before the
> +       wp_tablet_tool.done event.
> +      </description>
> +      <arg name="capability" type="uint" summary="the capability"/>
> +    </event>

Bitmask?

> +    <event name="down">
> +      <description summary="tablet tool is making contact">
> +       Sent whenever the tablet tool comes in contact with the surface of the
> +       tablet. If the tablet tool moves out of a region while in contact with
> +       the surface of the tablet, the client owning said region will receive a
> +       wp_tablet::up event, followed by a wp_tablet::proximity_out event and a
> +       wp_tablet::frame event.
> +      </description>
> +      <arg name="serial" type="uint"/>
> +    </event>

Inconsistent use of protocol::event here, with protocol.event
elsewhere in the docs.

>+    <event name="distance">
>+      <description summary="distance change event">
>+       Sent whenever the distance axis on a tool changes. The value of this
>+       event is normalized to a value between 0 and 65535.
>+      </description>
>+      <arg name="distance" type="uint" summary="The current distance value"/>
>+    </event>

Units?

> +    <enum name="button_state">
> +      <description summary="physical button state">
> +       Describes the physical state of a button which provoked the button event
> +      </description>
> +      <entry name="released" value="0" summary="button is not pressed"/>
> +      <entry name="pressed" value="1" summary="button is pressed"/>
> +    </enum>

Imperative is better here: 'button pressed', 'button released'. Also
happens to match the doc in the button event itself. ;)

> +    <event name="button">
> +      <description summary="button event">
> +       Sent whenever a button on the stylus is pressed or released.
> +      </description>
> +
> +      <arg name="serial" type="uint"/>
> +      <arg name="button" type="uint" summary="The button whose state has changed"/>
> +      <arg name="state" type="uint" summary="Whether the button was pressed or released"/>
> +    </event>

> +    <event name="id">
> +      <description summary="tablet device vid/pid">
> +       This event is sent in the initial burst of events before the
> +       wp_tablet.done event.
> +      </description>
> +      <arg name="vid" type="uint" summary="vendor id"/>
> +      <arg name="pid" type="uint" summary="product id"/>
> +    </event>

VID/PID space ... ?

> +    <enum name="type">
> +      <description summary="tablet type">
> +       Describes the type of tablet
> +      </description>

Full stop

> +      <entry name="external" value="0" summary="The tablet is an external tablet, such as an Intuos"/>
> +      <entry name="internal" value="1" summary="The tablet is a built-in tablet, usually in a laptop"/>
> +      <entry name="display" value="2" summary="The tablet is a display tablet, such as a Cintiq"/>
> +    </enum>
> +
> +    <event name="type">
> +      <description summary="the tablet type">
> +       This event is sent in the initial burst of events before the
> +       wp_tablet.done event.
> +      </description>
> +      <arg name="type" type="uint" summary="the tablet type (internal, external, ...)"/>
> +    </event>

Are these two not redundant?

> +    <event name="path">
> +      <description summary="path to the device">
> +       A system-specific device path that indicates which device is behind
> +       this wp_tablet. This information may be used to gather additional
> +       information about the device, e.g. through libwacom.
> +
> +       A device may have more than one device path, if so, multiple
> +       wp_tablet.path events are sent. A device may be emulated and not
> +       have a device path, in that case this event will not be sent.
> +
> +       The format of the path is unspecified, it may be a device node, a
> +       sysfs path, or some other identifier. It is up to the client to
> +       identify the string provided.
> +
> +       This event is sent in the initial burst of events before the
> +       wp_tablet.done event.
> +      </description>
> +      <arg name="path" type="string" summary="path to local device"/>
> +    </event>

Mm, device path isn't really a good idea with logind. Should just be
sysfs path or nothing, I think.

Anyway, none of these are particularly fundamental to the protocol;
the biggest change is the wp_tablet_seat thing, but given that's not a
pattern we've previously used, it could go either way. So, with as
many or few of these comments addressed as you feel makes sense:
Reviewed-by: Daniel Stone <daniels at collabora.com>

Cheers,
Daniel