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

[Pekka Paalanen]

On Tue, 15 Oct 2013 07:30:54 -0300
Rafael Antognolli <antognolli at gmail.com> wrote:

> Hi,
> 
> On Tue, Oct 8, 2013 at 4:07 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > 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.
> 
> OK, it makes sense. Is there consensus about this? If so, I'll just
> bring it back to get_xdg_surface.

I have only my opinion, and Rob disagreed, don't know about
others or concensus.

> >> +     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?
> 
> Not sure if it will help much from the toolkit point of view, I think
> we can live with any solution (speaking for EFL).
> 
> But I can change it to the "inert protocol object" to keep consistency
> with the other extensions.
> 
> >> +    </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.
> 
> Hmm... I don't know, I think I just copied what we had for wl_shell.
> Should it have a destructor too?

In hindsight, it probably should. I guess roles were originally thought
to be "set once and forever" with the rationale that wl_surfaces are
cheap to create, so it wasn't really needed at the time. Nowadays when
we are getting an increasing number of extensions to wl_surface and
more state, it is becoming heavier. One could also argue whether
re-purposing a surface is needed at all, or a good practice.

Actually, we probably did not even have the concept of a role when
wl_shell_surface was designed.

Personally I feel that the rule "define a destructor request unless
there is a good reason not to" is a safe guideline. Again, just my
opinion.

At least every protocol object should have a way to get destroyed,
other than the client disconnecting. I bet everyone can agree on that,
but I would also add "without introducing tricky error conditions".

> > 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.
> 
> OK, will look at this.

As a rule of thumb, anything that sets weston_surface::configure is a
role. That should let you find them all, including pointer cursors and
dnd icons (IIRC these roles are reversable).


Thanks,
pq