<div dir="ltr"><div><div> </div>Hi Pekka, and thanks a lot for your review ! </div><div> </div><div>I changed my code, taking your comments into consideration. Here are the details : </div><div> > New events should be added last, so they do not change the opcodes of existing events. </div>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 }; <div> > Copy-pasta? This is not a global interface, is it? </div><div>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 </div><div>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); </div><div>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. </div><div>Thanks again. I'll be aware of any other suggestions , and will eventually repost a set of corrected modifications. Regards, Manuel </div><div> </div></div><div class="gmail_extra"> <div class="gmail_quote">2014-02-19 11:56 GMT+01:00 Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>>: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="">On Wed, 19 Feb 2014 06:18:18 +0100 Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org">manuel.bachmann@open.eurogiciel.org</a>> 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 <<a href="mailto:manuel.bachmann@open.eurogiciel.org">manuel.bachmann@open.eurogiciel.org</a>> > --- > 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(+) </div>Hi, I'll comment only on the protocol bits, I didn't check the code. <div class=""> > 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> > + </div>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. <div class=""> > <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. </div>Copy-pasta? This is not a global interface, is it? <div><div class="h5"> > + </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> </div></div>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. <div class=""> > + > + <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. </div>Thanks, pq </blockquote></div> -- <div dir="ltr">Regards, Manuel BACHMANN Tizen Project VANNES-FR </div> </div>