[RFC Wayland 2/2] protocol: change input and opaque region interface

[Pekka Paalanen]

On Wed,  3 Oct 2012 16:14:16 +0300
Pekka Paalanen <ppaalanen at gmail.com> wrote:

> This change breaks the protocol.
> 
> Now that set_input_region and set_opaque_region no longer have to
> describe the region atomically in one request, we can simplify the
> protocol by not using wl_region for them.
> 
> Instead, the new requests mark_input and mark_opaque will take a
> simple rectangle, like damage already does. For describing complex forms
> clients will just send one rectangle at a time, and the server will
> accumulate the union of them. The wl_surface.commit request takes care,
> that no partial data is used.
> 
> Since the action is now union instead of replace, a way to clear the
> regions is needed. This is provided by the implicit clear on the first
> mark_* request after a wl_surface.commit.
> 
> As nothing uses wl_region interface anymore, it is removed from the
> core.
> 
> Signed-off-by: Pekka Paalanen <ppaalanen at gmail.com>
> ---
>  protocol/wayland.xml |   98 +++++++++++++++++--------------------------------
>  1 files changed, 34 insertions(+), 64 deletions(-)

The IRC conversations about this patch did not really bring up any
technical reasons why this would be better than using wl_region, nor
the other way around. Both have minor pros and cons.

This approach:
+ no need to create and destroy a temporary protocol object
+ => less protocol traffic
+ => less code to use it in toolkits
- the implicit clear on first mark_* after a commit is non-obvious and
  a little surprising
- only adding rectangles, no subtracting

Using wl_region:
+ the protocol is very clear and obvious, no implicit effects
+ adding and subtracting rects from the region
+ the region object can stay alive in the server and be re-used
+ keeping wl_region in the core protocol allows other extension to use
  it instead of reinventing it
- have to create an additional protocol object just to set a simple
  region
- => more traffic and object management involved
- => more code to use it in toolkits
- keeping region objects alive after being used is not useful? (since
  the server will keep the input/opaque regions intact by default)

So I think we are leaning towards keeping things clear, i.e. keep using
wl_regions for input and opaque.

That raises another question, though: should damage also be converted
to use wl_region?

IMHO, it might sound consistent at first thought, but damage region is
not like input or opaque. Damage naturally gets cleared, when the
server repaints, and you would expect that. Therefore it is logical for
the pending damage region to reset to empty on commit. The server will
repaint and the damage disappears, until you provide new surface
content with new damage. Using a temporary wl_region object to describe
damage is extra work, even more so since you likely damage a different
region every time.

Therefore, I will not propose to change the arguments for damage,
set_opaque_region, or set_input_region.

If toolkit developers have wishes to change anything here, I would
gladly hear their thoughts. Otherwise I think we should keep the
request arguments as they are.


Thanks,
pq

> 
> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> index 8b34084..81f4812 100644
> --- a/protocol/wayland.xml
> +++ b/protocol/wayland.xml
> @@ -126,13 +126,6 @@
>        </description>
>        <arg name="id" type="new_id" interface="wl_surface"/>
>      </request>
> -
> -    <request name="create_region">
> -      <description summary="create new region">
> -	Ask the compositor to create a new region.
> -      </description>
> -      <arg name="id" type="new_id" interface="wl_region"/>
> -    </request>
>    </interface>
>  
>    <interface name="wl_shm_pool" version="1">
> @@ -704,9 +697,9 @@
>        <arg name="callback" type="new_id" interface="wl_callback"/>
>      </request>
>  
> -    <request name="set_opaque_region">
> -      <description summary="set opaque region">
> -	This request sets the region of the surface that contains
> +    <request name="mark_opaque">
> +      <description summary="change opaque region for surface">
> +	This request is used to describe the regions of the surface that have
>  	opaque content.  The opaque region is an optimization hint for
>  	the compositor that lets it optimize out redrawing of content
>  	behind opaque regions.  Setting an opaque region is not
> @@ -717,40 +710,52 @@
>  
>  	Opaque region is double-buffered state, see wl_surface.commit.
>  
> -	wl_surface.set_opaque_region changes the pending opaque region.
> -	wl_surface.commit copies the pending region to the current region.
> +	wl_surface.commit copies the pending opaque region to the current region.
> +	The first wl_surface.mark_opaque request after a wl_surface.commit will
> +	clear the pending opaque region before adding the given rectangle.
> +	Further wl_surface.mark_opaque requests just add the given rectangle to
> +	the pending opaque region, until the next wl_surface.commit.
>  	Otherwise the pending and current regions are never changed.
>  
> -	The initial value for opaque region is empty. Setting the pending
> -	opaque region has copy semantics, and the wl_region object can be
> -	destroyed immediately. A nil wl_region causes the pending opaque
> -	region to be set to empty.
> +	The initial value for opaque region is empty. The pending opaque
> +	region can be set to empty by sending the first mark_opaque request
> +	after a wl_surface.commit with a zero width and/or height.
>        </description>
>  
> -      <arg name="region" type="object" interface="wl_region" allow-null="true"/>
> +      <arg name="x" type="int"/>
> +      <arg name="y" type="int"/>
> +      <arg name="width" type="int"/>
> +      <arg name="height" type="int"/>
>      </request>
>  
> -    <request name="set_input_region">
> -      <description summary="set input region">
> -	This request sets the region of the surface that can receive
> -	pointer and touch events. Input events happening outside of
> -	this region will try the next surface in the server surface
> -	stack. The compositor ignores the parts of the input region that
> +    <request name="mark_input">
> +      <description summary="change input sensitive region for surface">
> +	This request is used to describe the regions of the surface that can
> +	receive pointer and touch events. Input events initiating at
> +	coordinates outside of this region will ignore this surface, and
> +	input device enter/leave events obey the input region rather than the
> +	surface. The compositor ignores the parts of the input region that
>  	fall outside of the surface.
>  
>  	Input region is double-buffered state, see wl_surface.commit.
>  
> -	wl_surface.set_input_region changes the pending input region.
> -	wl_surface.commit copies the pending region to the current region.
> +	wl_surface.commit copies the pending input region to the current region.
> +	The first wl_surface.mark_input request after a wl_surface.commit will
> +	clear the pending input region before adding the given rectangle.
> +	Further wl_surface.mark_input requests just add the given rectangle to
> +	the pending input region, until the next wl_surface.commit.
>  	Otherwise the pending and current regions are never changed.
>  
>  	The initial value for input region is infinite. That means the whole
> -	surface will accept input. Setting the pending input region has copy
> -	semantics, and the wl_region object can be destroyed immediately. A
> -	nil wl_region causes the input region to be set to infinite.
> +	surface will accept input. The pending input region can be set to
> +	empty by sending the first mark_input request after a
> +	wl_surface.commit with a zero width and/or height.
>        </description>
>  
> -      <arg name="region" type="object" interface="wl_region" allow-null="true"/>
> +      <arg name="x" type="int"/>
> +      <arg name="y" type="int"/>
> +      <arg name="width" type="int"/>
> +      <arg name="height" type="int"/>
>      </request>
>  
>      <request name="commit">
> @@ -1143,39 +1148,4 @@
>      </event>
>    </interface>
>  
> -  <interface name="wl_region" version="1">
> -    <description summary="region interface">
> -      Region.
> -    </description>
> -
> -    <request name="destroy" type="destructor">
> -      <description summary="destroy region">
> -	Destroy the region.  This will invalidate the object id.
> -      </description>
> -    </request>
> -
> -    <request name="add">
> -      <description summary="add rectangle to region">
> -	Add the specified rectangle to the region
> -      </description>
> -
> -      <arg name="x" type="int"/>
> -      <arg name="y" type="int"/>
> -      <arg name="width" type="int"/>
> -      <arg name="height" type="int"/>
> -    </request>
> -
> -    <request name="subtract">
> -      <description summary="subtract rectangle from region">
> -	Subtract the specified rectangle from the region
> -      </description>
> -
> -      <arg name="x" type="int"/>
> -      <arg name="y" type="int"/>
> -      <arg name="width" type="int"/>
> -      <arg name="height" type="int"/>
> -    </request>
> -
> -  </interface>
> -
>  </protocol>