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

[Ander Conselvan de Oliveira]

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.


>> +	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.


> 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.

Thanks,
Ander