On Thu, Oct 08, 2015 at 12:15:10PM -0500, Derek Foreman wrote: > On 07/10/15 07:41 PM, Jonas Ådahl wrote: > > On Wed, Oct 07, 2015 at 01:32:35PM -0500, Derek Foreman wrote: > >> On 29/07/15 01:39 AM, Jonas Ådahl wrote: > >>> A wl_relative_pointer object is an extension to the wl_pointer interface > >>> only used for emitting relative pointer events. It will only emit events > >>> when the parent pointer has focus. > >>> > >>> To get a relative pointer object, use the get_relative_pointer request > >>> of the global wl_relative_pointer_manager object. When stabilizing it > >>> might make more sense to just add it to wl_seat instead of having a > >>> single use global interface. > >>> > >>> All interface names are currently prefixed with underscore in order to > >>> avoid any future conflicts with stable protocol. > >>> > >>> Signed-off-by: Jonas Ådahl <jad...@gmail.com> > >> > >> Some comments below, and this series doesn't merge anymore - do you have > >> time to rebase it? > > > > Yes, but I've been avoiding reposting until some amount of review > > progress has been made so there won't be too many versions on the list. > > Seems reasonable. > > >> > >> As far as I'm concerned, the first 2 patches in the series are good to > >> land (once they compile again...) > >> > >> This one's > >> Acked-by: Derek Foreman <der...@osg.samsung.com> > >> > >> Would really like to see Daniel's review, time permitting. > >> > >> I think it would be nice to land this in the same release cycle (ie: > >> this one) as pointer confinement, because I think the two features > >> really go hand in hand. > > > > Indeed. Relative pointer events are quite useless if you loose focus > > almost immediately > > > >> > >>> --- > >>> > >>> Changes since v2: > >>> > >>> Time stamps are now 64 bit and in microseconds. Since Wayland can't > >>> represent 64 bit unsigned integers, it is split into two 32 bit unsigned > >>> integers, one for each half. > >>> > >>> The other change is that there is no more manual resource moving around > >>> as focus resources are now tracked with weston_pointer_client. > >>> > >>> > >>> Makefile.am | 7 +- > >>> protocol/relative-pointer.xml | 129 +++++++++++++++++++++++++++ > >>> src/compositor.c | 3 + > >>> src/compositor.h | 4 + > >>> src/input.c | 197 > >>> +++++++++++++++++++++++++++++++++++++----- > >>> 5 files changed, 317 insertions(+), 23 deletions(-) > >>> create mode 100644 protocol/relative-pointer.xml > >>> > >>> diff --git a/Makefile.am b/Makefile.am > >>> index 52c736c..af17e7e 100644 > >>> --- a/Makefile.am > >>> +++ b/Makefile.am > >>> @@ -109,7 +109,9 @@ nodist_weston_SOURCES = > >>> \ > >>> protocol/presentation_timing-protocol.c \ > >>> protocol/presentation_timing-server-protocol.h \ > >>> protocol/scaler-protocol.c \ > >>> - protocol/scaler-server-protocol.h > >>> + protocol/scaler-server-protocol.h \ > >>> + protocol/relative-pointer-protocol.c \ > >>> + protocol/relative-pointer-server-protocol.h > >>> > >>> BUILT_SOURCES += $(nodist_weston_SOURCES) > >>> > >>> @@ -1316,7 +1318,8 @@ EXTRA_DIST += > >>> \ > >>> protocol/presentation_timing.xml \ > >>> protocol/scaler.xml \ > >>> protocol/ivi-application.xml \ > >>> - protocol/ivi-hmi-controller.xml > >>> + protocol/ivi-hmi-controller.xml \ > >>> + protocol/relative-pointer.xml > >>> > >>> # > >>> # manual test modules in tests subdirectory > >>> diff --git a/protocol/relative-pointer.xml b/protocol/relative-pointer.xml > >>> new file mode 100644 > >>> index 0000000..dd993e4 > >>> --- /dev/null > >>> +++ b/protocol/relative-pointer.xml > >>> @@ -0,0 +1,129 @@ > >>> +<?xml version="1.0" encoding="UTF-8"?> > >>> +<protocol name="relative_pointer"> > >>> + > >>> + <copyright> > >>> + Copyright © 2014 Jonas Ådahl > >>> + Copyright © 2015 Red Hat Inc. > >>> + > >>> + Permission is hereby granted, free of charge, to any person > >>> obtaining a > >>> + copy of this software and associated documentation files (the > >>> "Software"), > >>> + to deal in the Software without restriction, including without > >>> limitation > >>> + the rights to use, copy, modify, merge, publish, distribute, > >>> sublicense, > >>> + and/or sell copies of the Software, and to permit persons to whom the > >>> + Software is furnished to do so, subject to the following conditions: > >>> + > >>> + The above copyright notice and this permission notice (including the > >>> next > >>> + paragraph) shall be included in all copies or substantial portions > >>> of the > >>> + Software. > >>> + > >>> + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > >>> EXPRESS OR > >>> + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > >>> MERCHANTABILITY, > >>> + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > >>> SHALL > >>> + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > >>> OTHER > >>> + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > >>> ARISING > >>> + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > >>> + DEALINGS IN THE SOFTWARE. > >>> + </copyright> > >>> + > >>> + <interface name="_wl_relative_pointer_manager" version="1"> > >> > >> Should there be a comment somewhere in here that this should probably be > >> part of wl_seat? > > > > Should it? It can of course; initially I considered it to be the one > > true path, but there is no reason why we it'd be better (except being lazy > > binding globals). > > Ah, you stated in the commit log that this was the ultimate intention, > so I didn't want that information lost along the way. I didn't realize > this changed.
That was the original idea, but I've come to change my mind, I think. I think that for these types of additions it makes sense to make them as extensions so we can leave them out of wayland.xml. > > >> > >>> + <description summary="get relative pointer objects"> > >>> + A global interface used for getting the relative pointer object > >>> for a > >>> + given seat. > >>> + > >>> + Warning! The protocol described in this file is experimental. Each > >>> version > >>> + of this protocol should be considered incompatible with any other > >>> version, > >>> + and a client binding to a version different to the one advertised > >>> will be > >>> + terminated. Once the protocol is declared stable, backward > >>> compatibility > >>> + is guaranteed, the '_' prefix will be removed from the name and the > >>> + version will be reset to 1. > >>> + </description> > >>> + > >>> + <request name="get_relative_pointer"> > >>> + <description summary="get a relative pointer object"> > >>> + Create a relative pointer interface given a wl_pointer object. > >>> See > >>> + the wl_relative_pointer interface for more details. > >>> + </description> > >>> + > >>> + <arg name="id" type="new_id" interface="_wl_relative_pointer"/> > >>> + <arg name="pointer" type="object" interface="wl_pointer"/> > >> > >> Really this is the only reason I acked instead of r-b, and it's probably > >> just due to my own misunderstandings, but: > >> > >> Why are we passing a pointer object here instead of the seat? If a game > >> wants relative pointer data it'll likely not want absolute pointer data, > >> so why does it have to bind an absolute pointer (and receive events on > >> it?) before it can get the relative stuff it's actually going to use? > > > > Initially this request took a seat, since then it has been more > > considered an extension to the pointer object (sharing its focus, etc) > > than its own seat device. It was brought up during an early review (on > > phabricator, so you might not have seen it) that it might be a good idea > > to make the extension-ness more obvious. > > I can't find any of this in phabricator - can you give me a url? Maybe > I'm just doing a poor search... > > > An besides that, the client will probably want to know the pointer > > focus, it might want to set the cursor and it will probably like to know > > about clicks etc. > > But if it's a 1000hz gaming mouse the compositor will generate 1000 > events per second for the client to discard? > > Why not just allow the pointer to be bound as either/both serial and > absolute, and have each be a fully functional interface? > > Perhaps I should read what's in phabricator before I continue to > comment, though. Hmm. I can't find it any more. It was in the fdo phabricator task <https://phabricator.freedesktop.org/T1> but all those comments are no longer there. I have no idea why. > > >> > >> Can the "parent" wl_pointer be safely released once the relative one is > >> acquired? > > > > Makes sense. > > (but only if you don't need clicks and focus...) > > >> > >>> + </request> > >>> + </interface> > >>> + > >>> + <interface name="_wl_relative_pointer" version="1"> > >>> + <description summary="relative pointer object"> > >>> + A wl_relative_pointer object is an extension to the wl_pointer > >>> interface > >>> + used for emitting relative pointer events. It shares the same > >>> focus as > >>> + wl_pointer objects of the same seat and will only emit events when > >>> it > >>> + has focus. > >>> + </description> > >>> + > >>> + <request name="release" type="destructor"> > >>> + <description summary="release the relative pointer object"/> > >>> + </request> > >>> + > >>> + <event name="relative_motion"> > >>> + <description summary="relative pointer motion"> > >>> + Relative x/y pointer motion in from the pointer of the seat > >>> associated > >>> + with this object. > >>> + > >>> + A relative motion is in the same dimension as regular wl_pointer > >>> motion > >>> + events, except they do not represent an absolute position. For > >>> example, > >>> + moving a pointer from (x, y) to (x', y') would have the > >>> equivalent > >>> + relative motion (x' - x, y' - y). If a pointer motion caused the > >>> + absolute pointer position to be clipped by for example the edge > >>> of the > >>> + monitor, the relative motion is unaffected by the clipping and > >>> will > >>> + represent the unclipped motion. > >>> + > >>> + This event also contains non-accelerated motion deltas. The > >>> + non-accelerated delta is, when applicable, the regular pointer > >>> motion > >>> + delta as it was before having applied motion acceleration > >>> + transformations. The compositor will have applied the same > >>> processing > >>> + (such as normalization) meaning the events will have roughly the > >>> same > >>> + magnitude as accelerated motion events. > >>> + > >>> + Note that the non-accelerated delta does not represent 'raw' > >>> events as > >>> + they were read from some device. Pointer motion acceleration is > >>> device- > >>> + and configuration-specific and non-accelerated deltas and > >>> accelerated > >>> + deltas may have the same value on some devices. > >>> + > >>> + Relative motions are not coupled to wl_pointer.motion events, > >>> and can > >>> + be sent in combination with such events, but also independently. > >>> There > >>> + may also be scenarious where wl_pointer.motion is sent, but > >>> there is no > >>> + relative motion. The order of an absolute and relative motion > >>> event > >>> + originating from the same physical motion is not guaranteed. > >>> + > >>> + The motion vectors are encoded as double fixed point values. > >>> + > >>> + A double fixed point value is a 64 bit data type encoded as two > >>> separate > >>> + signed 32 bit integers. The integral part of the value is stored > >>> in one > >>> + of the integers and the fractional part in the other. > >>> + > >>> + If the client needs button events, it can receive them from a > >>> wl_pointer > >>> + object of the same seat that the wl_relative_pointer object is > >>> + associated with. > >>> + </description> > >>> + > >>> + <arg name="utime_most" type="uint" > >>> + summary="32 most significant bits of a 64 bit timestamp with > >>> microsecond granularity"/> > >>> + <arg name="utime_least" type="uint" > >> > >> The presentation timing extension also splits a 64-bit value up like > >> this, but there it's "tv_sec_hi" and "tv_sec_lo" > >> > >> Personally (and deep inside I realize this is pedantic bike shed > >> painting...) I'd like to see consistency in how we name split 64 bit > >> types... > > > > True. My naming comes from "most/least significant bit", and I guess > > "high/low order bit" is just as correct. Any preference? > > I guess hi/lo is less to type, and already in use. (I don't > particularly LIKE it, mind you.) Good points. > >>> + wl_double_fixed_from_double(dx, &dx_int, &dx_frac); > >>> + wl_double_fixed_from_double(dy, &dy_int, &dy_frac); > >> > >> Do you have a patch somewhere for wl_double_fixed_from_double? > > > > http://patchwork.freedesktop.org/patch/49211/ > > Ah, yes, hung up indefinitely in the pointer confinement review. :/ > > It would be nice to unstick some of the preamble bits that require less > fisticuffs than the protocol bits... I wonder if we should put "wl_double_fixed" in wayland/ and declare that an "official mutli part type" thing so we don't have to reimplement the awkward from/to functions all over the place. Maybe even a wl_double_fixed_t type as was suggested at an earlier point? Jonas _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
