[PATCH 1/2] xdg_shell: Add a new shell protocol.

[Pekka Paalanen]

Hi,

sorry for a late reply, I'm still around 600 emails behind of
wayland-devel at ...


On Fri, 19 Jul 2013 16:42:13 -0300
antognolli at gmail.com wrote:

> From: Rafael Antognolli <rafael.antognolli at intel.com>
> 
> xdg_shell is a protocol aimed to substitute wl_shell in the long term,
> but will not be part of the wayland core protocol. It starts as a
> non-stable API, aimed to be used as a development place at first, and
> once features are defined as required by several desktop shells, we can
> finally make it stable.
> ---
>  configure.ac                  |   1 +
>  protocol/Makefile.am          |   3 +
>  protocol/xdg-surface.xml      | 326 ++++++++++++++++++++++++++++++++++++++++++
>  src/Makefile.am               |   2 +-
>  src/wayland-xdg-surface.pc.in |  10 ++
>  5 files changed, 341 insertions(+), 1 deletion(-)
>  create mode 100644 protocol/xdg-surface.xml
>  create mode 100644 src/wayland-xdg-surface.pc.in
> 
> diff --git a/configure.ac b/configure.ac
> index 536df9e..61579c8 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -141,6 +141,7 @@ AC_CONFIG_FILES([Makefile
>  		 src/wayland-server.pc
>  		 src/wayland-client.pc
>  		 src/wayland-scanner.pc
> +		 src/wayland-xdg-surface.pc
>  		 src/wayland-version.h
>  		 protocol/Makefile
>  		 tests/Makefile])
> diff --git a/protocol/Makefile.am b/protocol/Makefile.am
> index 08690b3..69b5363 100644
> --- a/protocol/Makefile.am
> +++ b/protocol/Makefile.am
> @@ -1 +1,4 @@
>  EXTRA_DIST = wayland.xml
> +
> +protocoldir = $(datadir)/wayland/protocol
> +protocol_DATA = xdg-surface.xml
> diff --git a/protocol/xdg-surface.xml b/protocol/xdg-surface.xml
> new file mode 100644
> index 0000000..de64018
> --- /dev/null
> +++ b/protocol/xdg-surface.xml
> @@ -0,0 +1,326 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="xdg_surface">
> +
> +  <copyright>
> +    Copyright © 2008-2013 Kristian Høgsberg
> +    Copyright © 2013      Rafael Antognolli
> +    Copyright © 2010-2013 Intel Corporation
> +
> +    Permission to use, copy, modify, distribute, and sell this
> +    software and its documentation for any purpose is hereby granted
> +    without fee, provided that the above copyright notice appear in
> +    all copies and that both that copyright notice and this permission
> +    notice appear in supporting documentation, and that the name of
> +    the copyright holders not be used in advertising or publicity
> +    pertaining to distribution of the software without specific,
> +    written prior permission.  The copyright holders make no
> +    representations about the suitability of this software for any
> +    purpose.  It is provided "as is" without express or implied
> +    warranty.
> +
> +    THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> +    SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> +    FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> +    SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> +    WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN
> +    AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> +    ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF
> +    THIS SOFTWARE.
> +  </copyright>
> +
> +  <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.
> +    </description>
> +
> +    <request name="create_xdg_surface">
> +      <description summary="create a shell surface from a surface">
> +	Create a shell surface for an existing surface.

I know Rob suggested naming this as "create", but I think there is more
to think about.

wl_compositor.create_surface is definitely a "create" method, because
it creates a new independent object in both protocol and semantically
(a new surface).

Others like wl_shell.get_shell_surface, xdg_shell.create_xdg_surface,
wl_subcompositor.get_subsurface do create a new protocol object, but
not a new object semantically. These methods create an extension
interface to an existing "real" object, and this extension object
cannot function without the real object. That was my rationale for
using "get" in the method name: I am just retrieving a handle to the
extended interface.

> +	Only one shell surface can be associated with a given surface.
> +      </description>
> +      <arg name="id" type="new_id" interface="xdg_surface"/>
> +      <arg name="surface" type="object" interface="wl_surface"/>
> +    </request>
> +  </interface>
> +
> +  <interface name="xdg_surface" version="1">
> +
> +    <description summary="desktop-style metadata interface">
> +      An interface that may be implemented by a wl_surface, for
> +      implementations that provide a desktop-style user interface.
> +
> +      It provides requests to treat surfaces like toplevel, fullscreen
> +      or popup windows, move, resize or maximize them, associate
> +      metadata like title and class, etc.
> +
> +      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.

Would it make sense to change this to follow the "inert protocol
object" idiom?

Instead of making the wrong destruction order prone to fatal protocol
errors, destroying the real object would only make the interface object
inert until destroyed. Would this be for the better allowing easier
code in toolkits, or for the worse due to tolerating sloppier(?) code?

> +    </description>

I could not see a destructor request for this interface. I believe all
interfaces should have a destructor request defined, unless there is a
good reason to not define one (wl_callback is the rare example of
that). Otherwise it is possible that some later change makes us realize
we really do need it to avoid temporary leaking or other problems in the
compositor, like happened with some of the input related objects.

This extended interface (protocol object) gives the wl_surface a role
(doesn't it?). What should happen when the extended interface is
destroyed? Should the role be reset?

Would be nice to see consistency in behaviour between different
extension interfaces.


Thanks,
pq