[PATCH wayland 1/2] protocol: replace pointer.attach with pointer.set_cursor

Tue Jun 19 00:40:31 PDT 2012

On Mon, 18 Jun 2012 16:02:34 +0300
Ander Conselvan de Oliveira <conselvan2 at gmail.com> wrote:

> On 06/18/2012 12:06 PM, Pekka Paalanen wrote:
> > On Fri, 15 Jun 2012 17:27:32 +0300
> > Ander Conselvan de Oliveira<ander.conselvan.de.oliveira at intel.com>
> > wrote:
> >
> >> ---
> >>   protocol/wayland.xml |   27 +++++++++++++++++++++------
> >>   1 files changed, 21 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> >> index e9c6787..23b244f 100644
> >> --- a/protocol/wayland.xml
> >> +++ b/protocol/wayland.xml
> >> @@ -773,15 +773,30 @@
> >>     </interface>
> >>
> >>     <interface name="wl_pointer" version="1">
> >> -<request name="attach">
> >> -<description summary="set the pointer image">
> >> -	Set the pointer's image.  This request only takes effect if
> >> -	the pointer focus for this device is one of the requesting
> >> -	clients surfaces.
> >> +<request name="set_cursor">
> >> +<description summary="set the pointer surface">
> >> +	Set the pointer surface, i.e., the surface that contains the
> >> +	pointer image. This request only takes effect if the pointer
> >> +	focus for this device is one of the requesting client surfaces.
> >> +	If there was a previous surface set with this request it is
> >> +	replaced. If surface is NULL, the pointer image is hidden.
> >> +
> >> +	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.
> >
> > I wonder if we could phrase this paragraph even more clearly...
> >
> > "The parameters hotspot_x,hotspot_y define the cursor hotspot in the
> > cursor surface, given in cursor surface-local coordinates. In other
> > words, if x,y is the cursor position on a (window) surface, then the
> > top-left corner pixel of the cursor is at the window surface
> > coordinates (x - hotspot_x, y - hotspot_y)."
> 
> Well, the wording across the protocol is not very consistent when it 
> comes to this. There's no definition of what is the cursor hotspot, 
> although this term is used in data_device.start_drag. Now this patch 
> made things worse, since pointer.enter says that the pointer image is 
> unset but we don't have a request that sets the pointer image anymore. 
> Now I'm wondering if this request should be called pointer.set_cursor in 
> the first place. Maybe set_surface would be better.
> 
> Anyway, I think we first need to add a general description to wl_pointer 
> defining what a pointer have (location, surface, image, hotspot, 
> whatnot) otherwise I don't think we can actually be precise.
> 
> > We are talking about the corner pixel, and not corner, right? There is
> > a half-pixel difference between the two definitions.
> >
> > I've understood that integer pixel coordinates fall in the middle of a
> > pixel, and the border is at +-0.5 pixels from that.
> >
> > (the above definition leads to the conclusion, that a 2x3 pixel sized
> > area has coordinate extents from (-0.5,-0.5) to (1.5, 2.5).)
> 
> We do want that the center of the pixel (hotspot_x, hotspot_y) in the 
> surface to be precisely at the pointer location. So I guess we are 
> talking about the corner pixel in this case. Maybe the following would 
> make things better:
> 
>   "Parameters hotspot_x and hotspot_y define a pixel in pointer surface 
> coordinates that is guaranteed to have the same location in the 
> compositor coordinate system as that of the pointer. The pointer surface 
> location is updated whenever the pointer location changes to keep this 
> valid."
> 
> But then for the surface.attach case I'm not sure what to write. Maybe:
> 
> "The location of the pointer surface is updated on surface.attach 
> requests as described above. This will also cause the hotspot to be 
> updated to the pixel in the coordinate space of the pointer surface in 
> its new location that has the same compositor coordinate as the pointer 
> location."
> 
> (Sigh. The more I try to reword this the more confusing it gets.)
> 
> Since surface.attach has integer dx and dy I guess we can have the 
> hotspot be a pixel, but this is all quite messy.
> 
> Anyway, I think it would be better if we could get rid of
> "(x - hotspot_x, y - hotspot_y)" and just say that a coordinate here 
> matches a coordinate there or something.

I think much of the confusion with non-integer (pixel) coordinates
comes from the fact that we only recenly actually got them. Before,
we were always dealing with integers in some sense, in the protocol.

IMO a first step in clarifying everything is defining how we think
about pixels: are they squares of width and height 1.0, or do we think
of them as points, with unit spacing. Then, we need to define the
sub-pixel coordinates:
- how to convert from a real coordinate into the nearest pixel (index)?
- does a real coordinate with a 0 fraction hit the pixel at the center
or at the leading edge of entering the pixel's area?

This will become even more important, if we start pushing real
(well, wl_fixed_t) coordinates into pixman regions, like krh has
thought about.

Perhaps the definition should be chosen such, that pixman region
code, especially the "contains point" function, works naturally?
After all, it's just a convention, like in OpenGL with texture sampling.

> >> +	On surface.attach requests to the pointer surface, hotspot_x
> >> +	and hotspot_y are decremented by the x and y parameters
> >> +	passed to the request.
> > + This corresponds to removing x pixels from the top, and y pixels from
> > + the left in the cursor image, compared to the previous image.
> >
> > Would that addition make this paragraph any clearer?
> 
> We're not necessarily removing pixels. If the buffer being attached has 
> the same size as the previous one, the surface is being moved. I think 
> that adding "removing" here will only add to the confusion.

But the very definition (the intent, really, as krh explained to me when
I asked about making dx,dy wl_fixed_t) of the dx,dy parameters of
wl_surface::attach is *not* about moving the surface. It is about
adding or removing rows/columns of pixels from the top-left.

Remove -2 rows from top, size remains the same => remove 2 rows
from the bottom. It appears as moving the surface, but it really means
adding 2 rows on top and removing 2 at the bottom. That is why dx,dy
will stay as integers. You cannot remove or add fractional pixels.

So, another thing to fix in the docs...

> > Hmm... I just realised that the surface::attach dx,dy are defined as
> > moving the surface more naturally than "removing pixels from top and
> > left sides".
> >
> >> +
> >> +	The hotspot can also be updated by passing the current set
> >> +	pointer surface to this request with new values for hotspot_x
> >> +	and/or hotspot_y.
> >>         </description>
> >>
> >>         <arg name="serial" type="uint"/>
> >> -<arg name="buffer" type="object" interface="wl_buffer"/>
> >> +<arg name="surface" type="object" interface="wl_surface"/>
> >
> >>         <arg name="hotspot_x" type="int"/>
> >>         <arg name="hotspot_y" type="int"/>
> >
> > Should these be wl_fixed_t? Just pondering, since all coordinates are
> > fixed. Or are these supposed to be pixel counts, just like in the
> > surface::attach request?
> 
> I think the best definition would be the pixel within the surface that 
> should be centered in the pointer location. But in reality we allow that 
> to be outside of the pointer surface, so I don't know.

I don't think talking about pixels makes more sense than talking about
a point on the surface. The point, the hotspot, will always coincide with
the pointer position (which is not necessarily integer coordinates).

Thanks,
pq