[PATCH 3/5] shell: add managed_surface interface, request and events

[Pekka Paalanen]

On Wed, 19 Feb 2014 06:18:18 +0100
Manuel Bachmann <manuel.bachmann at open.eurogiciel.org> wrote:

> We create a new "managed_surface" object which will track
> a surface compositor-side, and receive events shell-side
> to handle 3 cases :
> - a toplevel surface has been created ;
> - a toplevel surface has been destroyed ;
> - a toplevel surface has a new title ;
> 
> Signed-off-by: Manuel Bachmann <manuel.bachmann at open.eurogiciel.org>
> ---
>  clients/desktop-shell.c    |   69 ++++++++++++++++++++++++++++++++++++++++++++
>  desktop-shell/shell.c      |   63 ++++++++++++++++++++++++++++++++++++++++
>  desktop-shell/shell.h      |    8 +++++
>  protocol/desktop-shell.xml |   49 +++++++++++++++++++++++++++++++
>  4 files changed, 189 insertions(+)

Hi,

I'll comment only on the protocol bits, I didn't check the code.

> diff --git a/protocol/desktop-shell.xml b/protocol/desktop-shell.xml
> index 3ae5d33..2262ade 100644
> --- a/protocol/desktop-shell.xml
> +++ b/protocol/desktop-shell.xml
> @@ -68,6 +68,14 @@
>        </description>
>      </event>
>  
> +    <event name="add_managed_surface">
> +      <description summary="tell client to manage a new surface">
> +	Tell the shell there is a new surface to manage, informing some
> +	GUI components such as the taskbar.
> +      </description>
> +      <arg name="id" type="new_id" interface="managed_surface"/>
> +    </event>
> +

New events should be added last, so they do not change the opcodes of
existing events. Same goes for requests. But this is only if we are
concerned about backwards compatibility, and if we are, you need to
bump the interface version.

Since desktop-shell is already at version 2, I think we are concerned
about backwards compatibility.

Of course then the code will need to handle interface versions lower
than what it was written for.

>      <event name="grab_cursor">
>        <description summary="tell client what cursor to show during a grab">
>  	This event will be sent immediately before a fake enter event on the
> @@ -98,6 +106,47 @@
>      </enum>
>    </interface>
>  
> +  <interface name="managed_surface" version="1">
> +    <description summary="interface for handling managed surfaces">
> +      Only one client can bind this interface at a time.

Copy-pasta? This is not a global interface, is it?

> +    </description>
> +
> +     <request name="set_state">
> +      <description summary="set managed surface state">
> +	The client can ask the state of the shell surface linked to a managed
> +	surface to be changed. It currently supports minimization.
> +      </description>
> +       <arg name="state" type="uint"/>
> +     </request>
> +
> +     <event name="state_changed">
> +      <description summary="tell client managed surface state was changed">
> +	This event will be sent immediately after the shell suface linked to a
> +	managed surface had its state changed. It currently supports minimization.
> +      </description>
> +       <arg name="state" type="uint"/>
> +     </event>
> +
> +     <event name="title_changed">
> +      <description summary="tell client managed surface title was changed">
> +	This event will be sent immediately after the title of the shell surface
> +	linked to a managed surface has been changed.
> +      </description>
> +       <arg name="title" type="string"/>
> +     </event>
> +
> +     <event name="removed">
> +      <description summary="remove a managed surface">
> +	This event will be sent when a managed surface is removed.
> +      </description>
> +     </event>

You need protocol for destroying managed_surface objects, IOW you need
to add a request that is a destructor. Otherwise, the server can never
destroy these objects (it essentially leaks them as the related windows
are destroyed), or if you thought that sending "removed" event means the
server itself destroys this protocol object, then you have a race. The
client may send "set_state" request before it sees the "removed" event,
which means the compositor will kill it for using an invalid object.

> +
> +     <enum name="state">
> +       <entry name="normal" value="0"/>
> +       <entry name="minimized" value="1"/>
> +     </enum>
> +  </interface>
> +
>    <interface name="screensaver" version="1">
>      <description summary="interface for implementing screensavers">
>        Only one client can bind this interface at a time.

Thanks,
pq