[PATCH weston 5/6] xdg-shell: Rewrite documentation

[Pekka Paalanen]

On Sat, 22 Nov 2014 12:28:29 -0800
"Jasper St. Pierre" <jstpierre at mecheye.net> wrote:

> This rewrites basically all of the text inside xdg-shell to be up to
> date, clearer, and rid of wl_shell and X11 terminology.
> ---
>  protocol/xdg-shell.xml | 256 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 156 insertions(+), 100 deletions(-)
> 
> diff --git a/protocol/xdg-shell.xml b/protocol/xdg-shell.xml
> index 360179d..3359cf7 100644
> --- a/protocol/xdg-shell.xml
> +++ b/protocol/xdg-shell.xml
> @@ -31,11 +31,15 @@
>  
>    <interface name="xdg_shell" version="1">
>      <description summary="create desktop-style surfaces">
> -      This interface is implemented by servers that provide
> -      desktop-style user interfaces.
> -
> -      It allows clients to associate a xdg_surface with
> -      a basic surface.
> +      xdg_shell allows clients to turn a wl_surface into a "real window"
> +      which can be dragged, resized, stacked, and moved around by the
> +      user. Everything about this interface is suited towards traditional
> +      desktop environments.
> +
> +      Destroying a bound xdg_shell object while there are surfaces
> +      still alive with roles from this interface is illegal and will
> +      result in a protocol error. Make sure to destroy all surfaces
> +      before destroying this object.

Ok. We don't have a destroy request on xdg_shell interface currently,
and you are not adding one in this series, so this cannot be enforced.
Is that intentional, or should there be a destroy request?

I think having a destroy request on global interfaces should be
mandatory, so I'd recommend adding one.

What about clients that create multiple xdg_shell objects and then
further xdg_* objects from each? Do xdg_shell objects track which xdg_*
objects have been created from them?


>      </description>
>  
>      <enum name="version">
> @@ -65,33 +69,26 @@
>  
>      <request name="get_xdg_surface">
>        <description summary="create a shell surface from a surface">
> -	Create a shell surface for an existing surface.
> -
> -	This request gives the surface the role of xdg_surface. If the
> -	surface already has another role, it raises a protocol error.
> -
> -	Only one shell or popup surface can be associated with a given
> -	surface.
> +	This creates an xdg_surface for the given surface and gives it the
> +	xdg_surface role. See the documentation of xdg_surface for more details.
>        </description>
>        <arg name="id" type="new_id" interface="xdg_surface"/>
>        <arg name="surface" type="object" interface="wl_surface"/>
>      </request>
>  
>      <request name="get_xdg_popup">
> -      <description summary="create a shell surface from a surface">
> -	Create a popup surface for an existing surface.
> -
> -	This request gives the surface the role of xdg_popup. If the
> -	surface already has another role, it raises a protocol error.
> +      <description summary="create a popup for a surface">
> +	This creates an xdg_popup for the given surface and gives it the
> +	xdg_popup role. See the documentation of xdg_surface for more details.

ITYM xdg_popup instead of xdg_surface.

>  
> -	Only one shell or popup surface can be associated with a given
> -	surface.
> +	This request must be used in response to some sort of user action
> +	like a button press, key press, or touch down event.
>        </description>
>        <arg name="id" type="new_id" interface="xdg_popup"/>
>        <arg name="surface" type="object" interface="wl_surface"/>
>        <arg name="parent" type="object" interface="wl_surface"/>
> -      <arg name="seat" type="object" interface="wl_seat" summary="the wl_seat whose pointer is used"/>
> -      <arg name="serial" type="uint" summary="serial of the implicit grab on the pointer"/>
> +      <arg name="seat" type="object" interface="wl_seat" summary="the wl_seat of the user event"/>
> +      <arg name="serial" type="uint" summary="the serial of the user event"/>
>        <arg name="x" type="int"/>
>        <arg name="y" type="int"/>
>      </request>
> @@ -107,7 +104,7 @@
>          respond to the ping request, or in what timeframe. Clients should
>          try to respond in a reasonable amount of time.
>        </description>
> -      <arg name="serial" type="uint" summary="pass this to the callback"/>
> +      <arg name="serial" type="uint" summary="pass this to the pong request"/>
>      </event>
>  
>      <request name="pong">
> @@ -120,8 +117,7 @@
>    </interface>
>  
>    <interface name="xdg_surface" version="1">
> -
> -    <description summary="desktop-style metadata interface">
> +    <description summary="A desktop window">
>        An interface that may be implemented by a wl_surface, for
>        implementations that provide a desktop-style user interface.
>  
> @@ -136,20 +132,22 @@
>      </description>
>  
>      <request name="destroy" type="destructor">
> -      <description summary="remove xdg_surface interface">
> -	The xdg_surface interface is removed from the wl_surface object
> -	that was turned into a xdg_surface with
> -	xdg_shell.get_xdg_surface request. The xdg_surface properties,
> -	like maximized and fullscreen, are lost. The wl_surface loses
> -	its role as a xdg_surface. The wl_surface is unmapped.
> +      <description summary="Destroy the xdg_surface">
> +	Unmap and destroy the window. The window will be effectively
> +	hidden from the user's point of view, and all state like
> +	maximization, fullscreen, and so on, will be lost.

This is ok, but above this there is a paragraph that says:

	"On the server side the object is automatically destroyed when
	the related wl_surface is destroyed.  On client side,
	xdg_surface.destroy() must be called before destroying
	the wl_surface object."

I think that paragraph needs to have "On the server side the object is
automatically destroyed when the related wl_surface is destroyed."
deleted, because it contradicts with the existence of the destroy
request. This is likely a left-over from wl_shell_surface, where there
is no destroy request.


>        </description>
>      </request>
>  
>      <request name="set_parent">
> -      <description summary="surface is a child of another surface">
> -	Child surfaces are stacked above their parents, and will be
> -	unmapped if the parent is unmapped too. They should not appear
> -	on task bars and alt+tab.
> +      <description summary="set the parent of this surface">
> +	Set the "parent" of this surface. This window should be stacked
> +	above a parent. The parent surface must be mapped as long as this
> +	surface is mapped.
> +
> +	Parent windows should be set on dialogs, toolboxes, or other
> +	"auxilliary" surfaces, so that the parent is raised when the dialog
> +	is raised.
>        </description>
>        <arg name="parent" type="object" interface="xdg_surface" allow-null="true"/>
>      </request>

...

> @@ -311,14 +324,20 @@
>  
>      <request name="ack_configure">
>        <description summary="ack a configure event">
> -        When a configure event is received, a client should then ack it
> -        using the ack_configure request to ensure that the compositor
> -        knows the client has seen the event.
> -
> -        By this point, the state is confirmed, and the next attach should
> -        contain the buffer drawn for the configure event you are acking.
> +        When a configure event is received, if a client is updating
> +        and redrawing its state in response to the configure event,
> +        then the client needs to make an ack_configure request before
> +        point the commit to mark this next commit as being a
> +        reaction to the configure.

"...an ack_configure request before point the commit to mark this next
commit..." I feel there is something odd in that sentence. s/point// maybe?


> +
> +        The compositor might use this information to move a surface
> +        to the top left only when the client has drawn itself for
> +        the maximized or fullscreen state.
> +
> +        If the client receives multiple configure events before it
> +        can respond to one, it only has to ack the last configure event.
>        </description>
> -      <arg name="serial" type="uint" summary="a serial to configure for"/>
> +      <arg name="serial" type="uint" summary="the serial from the configure event"/>
>      </request>

No other comments to this patch, but I think should dig up my old
review of the whole xdg_shell.xml file, and check what comments I
would like to restate after these changes. I'll try to do that "soon".


Thanks,
pq