[PATCH v3] protocol: Extend wl_touch with touchpoint shape event

Fri Apr 15 01:54:38 UTC 2016

On Thu, Apr 14, 2016 at 07:04:25PM -0500, Yong Bakos wrote:
> On Apr 14, 2016, at 3:23 PM, Dennis Kempin <denniskempin at google.com> wrote:
> > 
> > This CL updates the wl_touch interface with a shape and
> > orientation event.
> > The shape/orientation of a touch point is not relevant for most UI
> > applications, but allows a better experience in some cases
> > such as drawing apps.
> > 
> > The events are used by the compositor to inform the client
> > about changes in the shape and orientation of a touchpoint, which is
> > approximated by an ellipse and it's angle to the y-axis.
> > 
> > The event is optional and only sent when compositor and the
> > touch device support this type of information. The client is
> > responsible for making a reasonable assumption about the
> > touch shape if no shape is reported.
> > 
> > Signed-off-by: Dennis Kempin <denniskempin at google.com>
> 
> Hi Dennis,
> I've noted some concerns inline below. I'm a bit inexperienced here, so
> please do correct/educate me where I'm wrong.
> 
> 
> > ---
> > protocol/wayland.xml | 75 ++++++++++++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 64 insertions(+), 11 deletions(-)
> > 
> > diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> > index 12a6efd..d19d898 100644
> > --- a/protocol/wayland.xml
> > +++ b/protocol/wayland.xml
> > @@ -910,11 +910,6 @@
> >       copy-and-paste and drag-and-drop.  These mechanisms are tied to
> >       a wl_seat and this interface lets a client get a wl_data_device
> >       corresponding to a wl_seat.
> > -
> > -      Depending on the version bound, the objects created from the bound
> > -      wl_data_device_manager object will have different requirements for
> > -      functioning properly. See wl_data_source.set_actions,
> > -      wl_data_offer.accept and wl_data_offer.finish for details.
> 
> What does the deletion of this paragraph have to do with adding the
> two new touchpoint shape events?
> 
> 
> >     </description>
> > 
> >     <request name="create_data_source">
> > @@ -1656,7 +1651,7 @@
> >     </request>
> >    </interface>
> > 
> > -  <interface name="wl_seat" version="5">
> > +  <interface name="wl_seat" version="6">
> 
> Why are you bumping this instead of just bumping the version of the 
> wl_touch interface?

Hi,

Just going to reply to your comments regarding the version bumps.

In Wayland, objects (except wl_display) are created from some other
object, and most "concepts" (such as input, surfaces, data
device/clipboard) have a single "global" object where all the other
objects are created from.

When you create an object from another object, except for global
objects, the new object will inherit the version of the object it was
created from. So for example if you have a global wl_compositor object
with version N, all wl_surface's created from that object will also have
the version N. If you need to bump the version of wl_surface, if you
want to be able to create an object with the bumped version, you also
need to bump the version of the interface the wl_surface is created
from, i.e. wl_compositor.

In this patch, the interface bumped is wl_touch, and the interface of
the object a wl_touch object is created from is wl_seat. Thus in order
to be able to get a wl_touch object with the bumped version, you also
need to bump the version of each parent interface up to the global
object.

Note that when you bump the version of the interface of the global
object, when creating an object from that global, it'll inherit the
version. This means that when you bump wl_touch and wl_seat, you'll
effectively get wl_pointer and wl_keyboard objects with the bumped
version as well.

While it is required to bump the parents up to the global object, it is
completely optional to actually bump the interface versions of all the
other interfaces in an "interface tree". The reason for doing so may be
to make it clear the actual highest version an object created with this
interface may be created at, even though no new events nor requests were
added for some particular version.

Hope this cleares things up.

Jonas

> 
> 
> >     <description summary="group of input devices">
> >       A seat is a group of keyboards, pointer and touch devices. This
> >       object is published as a global during start up, or when such a
> > @@ -1765,7 +1760,7 @@
> > 
> >   </interface>
> > 
> > -  <interface name="wl_pointer" version="5">
> > +  <interface name="wl_pointer" version="6">
> 
> Why are you bumping this instead of just bumping the version of the 
> wl_touch interface?
> 
> 
> >     <description summary="pointer input device">
> >       The wl_pointer interface represents one or more input devices,
> >       such as mice, which control the pointer location and pointer_focus
> > @@ -2038,7 +2033,7 @@
> > 
> >  The timestamp is to be interpreted identical to the timestamp in the
> >  wl_pointer.axis event. The timestamp value may be the same as a
> > - preceding wl_pointer.axis event.
> > + preceeding wl_pointer.axis event.
> 
> Why are you introducing a misspelling?
> 
> 
> >       </description>
> >       <arg name="time" type="uint" summary="timestamp with
> > millisecond granularity"/>
> >       <arg name="axis" type="uint" enum="axis" summary="the axis
> > stopped with this event"/>
> > @@ -2078,7 +2073,7 @@
> >     </event>
> >   </interface>
> > 
> > -  <interface name="wl_keyboard" version="5">
> > +  <interface name="wl_keyboard" version="6">
> 
> Why are you bumping this instead of just bumping the version of the 
> wl_touch interface?
> 
> 
> >     <description summary="keyboard input device">
> >       The wl_keyboard interface represents one or more keyboards
> >       associated with a seat.
> > @@ -2192,7 +2187,7 @@
> >     </event>
> >   </interface>
> > 
> > -  <interface name="wl_touch" version="5">
> > +  <interface name="wl_touch" version="6">
> >     <description summary="touchscreen input device">
> >       The wl_touch interface represents a touchscreen
> >       associated with a seat.
> > @@ -2242,7 +2237,12 @@
> > 
> >     <event name="frame">
> >       <description summary="end of touch frame event">
> > - Indicates the end of a contact point list.
> > +  Indicates the end of a contract point list. The wayland protocol requires
> 
> contact point list.
> 
> 
> > +  touch point updates to be sent sequentially, however all events within a
> > +  frame should be considered one hardware event. A wl_touch.frames terminates
> 
> wl_touch.frame
> 
> 
> > +  at least one event but otherwise no guarantee is provided about the set of
> > +  events within a frame. A client must assume that any state not updated in a
> > +  frame is unchanged from the previously known state.
> 
> Am I misreading this or has indentation been obliterated?
> 
> 
> >       </description>
> >     </event>
> > 
> > @@ -2262,6 +2262,59 @@
> >     <request name="release" type="destructor" since="3">
> >       <description summary="release the touch object"/>
> >     </request>
> > +
> > +    <!-- Version 6 additions -->
> > +
> > +    <event name="shape" since="6">
> > +      <description summary="update shape of touch point">
> > +  Sent when a touchpoint has changed its shape. If the touch position
> > +  or orientation changed at the same time, the wl_touch.motion,
> > +  wl_touch.orientation and wl_touch.shape are sent within the same
> > +  wl_touch.frame.
> > +  Otherwise, only a wl_touch.shape is sent within this wl_touch.frame.
> 
> Same paragraph? And indentation?
> 
> 
> > +  The protocol does not guarantee specific ordering of wl_touch.orientation,
> > +  wl_touch.shape and wl_touch.motion events.
> > +
> > +  A touchpoint shape is approximated by an ellipse through the major and minor
> > +  axis length. The major axis length describes the longest diameter of the
> > +  ellipse, while the minor axis length describes the shortest diameter.
> > +  Both are specified in surface coordinates.
> > +  The center of the ellipse is always at the touchpoint location as reported
> 
> Same paragraph? And indentation?
> 
> 
> > +  by wl_touch.down or wl_touch.move.
> > +
> > +  This event is only sent by the compositor if the touch device supports shape
> > +  reports. The client has to make reasonable assumptions about the shape if
> > +  it did not receive this event.
> 
> This last sentence seems unnecessary.
> 
> 
> > +      </description>
> > +      <arg name="id" type="int" summary="the unique ID of this touch point"/>
> > +      <arg name="major" type="fixed" summary="length of the major
> > axis in surface coordinates"/>
> > +      <arg name="minor" type="fixed" summary="length of the minor
> > axis in surface coordinates"/>
> 
> I politely encourage "surface local coordinates."
> https://lists.freedesktop.org/archives/wayland-devel/2016-April/027804.html
> 
> 
> > +    </event>
> > +
> > +    <event name="orientation" since="6">
> > +      <description summary="update orientation of touch point">
> > +  Sent when a touchpoint has changed its orientation. If the touch position
> > +  or shape changed at the same time, the wl_touch.motion, wl_touch.orientation
> > +  and wl_touch.shape are sent within the same wl_touch.frame.
> > +  Otherwise, only a wl_touch.orientation is sent within this wl_touch.frame.
> > +  The protocol does not guarantee specific ordering of wl_touch.orientation,
> > +  wl_touch.shape and wl_touch.motion events.
> > +
> > +  The orientation describes the clockwise angle of touchpoints major axis to
> > +  the surface y-axis and is normalized to the -180 to +180 degrees range.
> > +  The granuality of orientation depends on the touch device, some devices only
> > +  support binary rotation values between 0 and 90 degrees.
> > +
> > +  This event is only sent by the compositor if the touch device supports
> > +  orientation reports.
> > +  The client has to make reasonable assumptions about the orientation if
> > +  it did not receive this event.
> 
> As mentioned in the shape event, I feel this last sentence is unnecessary.
> And, am I misreading or is the indentation missing?
> 
> > +      </description>
> > +      <arg name="id" type="int" summary="the unique ID of this touch point"/>
> > +      <arg name="orientation" type="fixed"
> > +           summary="angle between major axis and surface y-axis in degrees"/>
> > +    </event>
> > +
> 
> Omitting this line break would match the current convention.
> 
> >   </interface>
> > 
> >   <interface name="wl_output" version="2">
> > --
> > 2.8.0.rc3.226.g39d4020
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel