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

Manuel Bachmann manuel.bachmann at open.eurogiciel.org
Wed Feb 19 05:04:34 PST 2014


Hi Pekka, and thanks a lot for your review !

I changed my code, taking your comments into consideration. Here are the
details :

> New events should be added last, so they do not change the opcodes of
existing events.

Ouch. You are right. I just reversed the order, so "add_managed_surface" is
now the last event :

--- a/clients/desktop-shell.c
+++ b/clients/desktop-shell.c
@@ -1239,21 +1239,6 @@ desktop_shell_prepare_lock_surface(void *data,
 }

 static void
-desktop_shell_add_managed_surface(void *data,
-                   struct desktop_shell *desktop_shell,
-                   struct managed_surface *managed_surface)
-{
-    struct desktop *desktop = data;
-    struct output *output;
-
-    wl_list_for_each(output, &desktop->outputs, link) {
-        /* add a handler with default title */
-        taskbar_add_handler(output->taskbar, managed_surface, "<Default>");
-        update_window(output->taskbar->window);
-    }
-}
-
-static void
 desktop_shell_grab_cursor(void *data,
               struct desktop_shell *desktop_shell,
               uint32_t cursor)
@@ -1300,11 +1285,26 @@ desktop_shell_grab_cursor(void *data,
     }
 }

+static void
+desktop_shell_add_managed_surface(void *data,
+                   struct desktop_shell *desktop_shell,
+                   struct managed_surface *managed_surface)
+{
+    struct desktop *desktop = data;
+    struct output *output;
+
+    wl_list_for_each(output, &desktop->outputs, link) {
+        /* add a handler with default title */
+        taskbar_add_handler(output->taskbar, managed_surface, "<Default>");
+        update_window(output->taskbar->window);
+    }
+}
+
 static const struct desktop_shell_listener listener = {
     desktop_shell_configure,
     desktop_shell_prepare_lock_surface,
-    desktop_shell_add_managed_surface,
-    desktop_shell_grab_cursor
+    desktop_shell_grab_cursor,
+    desktop_shell_add_managed_surface
 };

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

Correct. The description is now :

@@ -108,9 +108,16 @@

   <interface name="managed_surface" version="1">
     <description summary="interface for handling managed surfaces">
-      Only one client can bind this interface at a time.
+      Managed surfaces are abstractions for specific shell surfaces.
     </description>


> You need protocol for destroying managed_surface objects

Nice to have spotted that. More generally, I never called
wl_resource_destroy(), which was bad. Here is the additional request and
code :

+    <request name="destroy" type="destructor">
+      <description summary="remove managed_surface interface">
+    The managed_surface interface is removed and ceases to refer
+    to anything.
+      </description>
+    </request>
+

--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -4020,6 +4020,13 @@ static const struct desktop_shell_interface
desktop_shell_implementation = {
 };

 static void
+managed_surface_destroy(struct wl_client *client,
+            struct wl_resource *resource)
+{
+    wl_resource_destroy(resource);
+}
+
+static void
 managed_surface_set_state(struct wl_client *client,
             struct wl_resource *resource,
             uint32_t state)
@@ -4033,6 +4040,7 @@ managed_surface_set_state(struct wl_client *client,
 }

--- a/clients/desktop-shell.c
+++ b/clients/desktop-shell.c
 static const struct managed_surface_interface
managed_surface_implementation = {
+    managed_surface_destroy,
     managed_surface_set_state
 };

@@ -1344,6 +1344,8 @@ managed_surface_removed(void *data,
         /* destroy the handler */
         taskbar_destroy_handler(handler);
         update_window(handler->taskbar->window);
+
+        managed_surface_destroy(managed_surface);

I'm not sure if I need to do anything else. "managed_surface" only contains
a reference to an object which is destroyed somewhere else, so I just
destroy the wl_resource itself here.

Thanks again. I'll be aware of any other suggestions , and will eventually
repost a set of corrected modifications.

Regards,
Manuel



2014-02-19 11:56 GMT+01:00 Pekka Paalanen <ppaalanen at gmail.com>:

> 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
>



-- 
Regards,



*Manuel BACHMANN Tizen Project VANNES-FR*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140219/2af8ce9c/attachment.html>


More information about the wayland-devel mailing list