<div dir="ltr"><div>Just some info regarding these taskbar patches ; I have some more patches fixing bugs experienced on certain cases (multiples outputs, DRM backend...). They are all available on GitHub, <a href="https://github.com/Tarnyko/weston-taskbar/commits/HEAD-taskbar">https://github.com/Tarnyko/weston-taskbar/commits/HEAD-taskbar</a>. I could refactor and repost the whole set here.</div>
<div> </div><div>However, I also realized that having an UI is maybe too early. I have sized down a smaller set (2 patches) that only handles xdg_surface_set_minimized() and allows to unminimize by doing Mod-Tab. So no modification of desktop-shell is needed. Will post them in a few minutes.</div>
<div class="gmail_extra"><br>Regards,</div><div class="gmail_extra">Manuel<br></div><div class="gmail_quote">2014-02-19 14:04 GMT+01:00 Manuel Bachmann <span dir="ltr"><<a href="mailto:manuel.bachmann@open.eurogiciel.org" target="_blank">manuel.bachmann@open.eurogiciel.org</a>></span>:<br>
<blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid" class="gmail_quote"><div dir="ltr"><div><div><br></div>Hi Pekka, and thanks a lot for your review !<br>
</div><div><br></div><div>I changed my code, taking your comments into consideration. Here are the details :<br></div><div><div><br>> New events should be added last, so they do not change the opcodes of existing events.<br>
<br></div></div>Ouch. You are right. I just reversed the order, so "add_managed_surface" is now the last event :<br><br>--- a/clients/desktop-shell.c<br>+++ b/clients/desktop-shell.c<br>@@ -1239,21 +1239,6 @@ desktop_shell_prepare_lock_surface(void *data,<br>
}<br> <br> static void<br>-desktop_shell_add_managed_surface(void *data,<br>- struct desktop_shell *desktop_shell,<br>- struct managed_surface *managed_surface)<br>-{<br>- struct desktop *desktop = data;<br>
- struct output *output;<br>-<br>- wl_list_for_each(output, &desktop->outputs, link) {<br>- /* add a handler with default title */<br>- taskbar_add_handler(output->taskbar, managed_surface, "<Default>");<br>
- update_window(output->taskbar->window);<br>- }<div><br>-}<br>-<br>-static void<br> desktop_shell_grab_cursor(void *data,<br> struct desktop_shell *desktop_shell,<br> uint32_t cursor)<br>
</div>
@@ -1300,11 +1285,26 @@ desktop_shell_grab_cursor(void *data,<br> }<br> }<br> <br>+static void<div><br>+desktop_shell_add_managed_surface(void *data,<br>+ struct desktop_shell *desktop_shell,<br>+ struct managed_surface *managed_surface)<br>
+{<br>+ struct desktop *desktop = data;<br>+ struct output *output;<br>+<br>+ wl_list_for_each(output, &desktop->outputs, link) {<br></div>+ /* add a handler with default title */<br>+ taskbar_add_handler(output->taskbar, managed_surface, "<Default>");<div>
<br>
+ update_window(output->taskbar->window);<br>+ }<br>+}<br>+<br></div><div> static const struct desktop_shell_listener listener = {<br> desktop_shell_configure,<br> desktop_shell_prepare_lock_surface,<br>
</div>- desktop_shell_add_managed_surface,<br>
- desktop_shell_grab_cursor<br>+ desktop_shell_grab_cursor,<br>+ desktop_shell_add_managed_surface<div><br> };<br><div><br>> Copy-pasta? This is not a global interface, is it?<br><br></div></div><div>Correct. The description is now :<br>
<br>@@ -108,9 +108,16 @@<div><br> <br> <interface name="managed_surface" version="1"><br></div><div> <description summary="interface for handling managed surfaces"><br></div>
- Only one client can bind this interface at a time.<br>
+ Managed surfaces are abstractions for specific shell surfaces.<br> </description><div><br> <br><br>> You need protocol for destroying managed_surface objects<br><br></div></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 :<br>
<br>+ <request name="destroy" type="destructor"><br>+ <description summary="remove managed_surface interface"><br>+ The managed_surface interface is removed and ceases to refer<br>
+ to anything.<br>+ </description><br>+ </request><div><br>+<br><br>--- a/desktop-shell/shell.c<br>+++ b/desktop-shell/shell.c<br></div>@@ -4020,6 +4020,13 @@ static const struct desktop_shell_interface desktop_shell_implementation = {<br>
};<br> <br> static void<br>+managed_surface_destroy(struct wl_client *client,<br>+ struct wl_resource *resource)<br>+{<br>+ wl_resource_destroy(resource);<br>+}<br>+<br>+static void<br> managed_surface_set_state(struct wl_client *client,<br>
struct wl_resource *resource,<br> uint32_t state)<br>@@ -4033,6 +4040,7 @@ managed_surface_set_state(struct wl_client *client,<br> }<br> <br>--- a/clients/desktop-shell.c<br>+++ b/clients/desktop-shell.c<div>
<br>
static const struct managed_surface_interface managed_surface_implementation = {<br></div>+ managed_surface_destroy,<br> managed_surface_set_state<br> };<br><br>@@ -1344,6 +1344,8 @@ managed_surface_removed(void *data,<br>
/* destroy the handler */<br> taskbar_destroy_handler(handler);<br> update_window(handler->taskbar->window);<br>+<br>+ managed_surface_destroy(managed_surface);<br><br></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.<br>
<br></div><div>Thanks again. I'll be aware of any other suggestions , and will eventually repost a set of corrected modifications.<br><br>Regards,<br>Manuel<br></div><div><br></div></div><div class="gmail_extra"><br>
<br>
<div class="gmail_quote">2014-02-19 11:56 GMT+01:00 Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span>:<div><div class="h5"><br><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid" class="gmail_quote">
<div>On Wed, 19 Feb 2014 06:18:18 +0100<br>
Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org" target="_blank">manuel.bachmann@open.eurogiciel.org</a>> wrote:<br>
<br>
> We create a new "managed_surface" object which will track<br>
> a surface compositor-side, and receive events shell-side<br>
> to handle 3 cases :<br>
> - a toplevel surface has been created ;<br>
> - a toplevel surface has been destroyed ;<br>
> - a toplevel surface has a new title ;<br>
><br>
> Signed-off-by: Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org" target="_blank">manuel.bachmann@open.eurogiciel.org</a>><br>
> ---<br>
> clients/desktop-shell.c | 69 ++++++++++++++++++++++++++++++++++++++++++++<br>
> desktop-shell/shell.c | 63 ++++++++++++++++++++++++++++++++++++++++<br>
> desktop-shell/shell.h | 8 +++++<br>
> protocol/desktop-shell.xml | 49 +++++++++++++++++++++++++++++++<br>
> 4 files changed, 189 insertions(+)<br>
<br>
</div>Hi,<br>
<br>
I'll comment only on the protocol bits, I didn't check the code.<br>
<div><br>
> diff --git a/protocol/desktop-shell.xml b/protocol/desktop-shell.xml<br>
> index 3ae5d33..2262ade 100644<br>
> --- a/protocol/desktop-shell.xml<br>
> +++ b/protocol/desktop-shell.xml<br>
> @@ -68,6 +68,14 @@<br>
> </description><br>
> </event><br>
><br>
> + <event name="add_managed_surface"><br>
> + <description summary="tell client to manage a new surface"><br>
> + Tell the shell there is a new surface to manage, informing some<br>
> + GUI components such as the taskbar.<br>
> + </description><br>
> + <arg name="id" type="new_id" interface="managed_surface"/><br>
> + </event><br>
> +<br>
<br>
</div>New events should be added last, so they do not change the opcodes of<br>
existing events. Same goes for requests. But this is only if we are<br>
concerned about backwards compatibility, and if we are, you need to<br>
bump the interface version.<br>
<br>
Since desktop-shell is already at version 2, I think we are concerned<br>
about backwards compatibility.<br>
<br>
Of course then the code will need to handle interface versions lower<br>
than what it was written for.<br>
<div><br>
> <event name="grab_cursor"><br>
> <description summary="tell client what cursor to show during a grab"><br>
> This event will be sent immediately before a fake enter event on the<br>
> @@ -98,6 +106,47 @@<br>
> </enum><br>
> </interface><br>
><br>
> + <interface name="managed_surface" version="1"><br>
> + <description summary="interface for handling managed surfaces"><br>
> + Only one client can bind this interface at a time.<br>
<br>
</div>Copy-pasta? This is not a global interface, is it?<br>
<div><div><br>
> + </description><br>
> +<br>
> + <request name="set_state"><br>
> + <description summary="set managed surface state"><br>
> + The client can ask the state of the shell surface linked to a managed<br>
> + surface to be changed. It currently supports minimization.<br>
> + </description><br>
> + <arg name="state" type="uint"/><br>
> + </request><br>
> +<br>
> + <event name="state_changed"><br>
> + <description summary="tell client managed surface state was changed"><br>
> + This event will be sent immediately after the shell suface linked to a<br>
> + managed surface had its state changed. It currently supports minimization.<br>
> + </description><br>
> + <arg name="state" type="uint"/><br>
> + </event><br>
> +<br>
> + <event name="title_changed"><br>
> + <description summary="tell client managed surface title was changed"><br>
> + This event will be sent immediately after the title of the shell surface<br>
> + linked to a managed surface has been changed.<br>
> + </description><br>
> + <arg name="title" type="string"/><br>
> + </event><br>
> +<br>
> + <event name="removed"><br>
> + <description summary="remove a managed surface"><br>
> + This event will be sent when a managed surface is removed.<br>
> + </description><br>
> + </event><br>
<br>
</div></div>You need protocol for destroying managed_surface objects, IOW you need<br>
to add a request that is a destructor. Otherwise, the server can never<br>
destroy these objects (it essentially leaks them as the related windows<br>
are destroyed), or if you thought that sending "removed" event means the<br>
server itself destroys this protocol object, then you have a race. The<br>
client may send "set_state" request before it sees the "removed" event,<br>
which means the compositor will kill it for using an invalid object.<br>
<div><br>
> +<br>
> + <enum name="state"><br>
> + <entry name="normal" value="0"/><br>
> + <entry name="minimized" value="1"/><br>
> + </enum><br>
> + </interface><br>
> +<br>
> <interface name="screensaver" version="1"><br>
> <description summary="interface for implementing screensavers"><br>
> Only one client can bind this interface at a time.<br>
<br>
</div>Thanks,<br>
pq<br>
</blockquote></div></div></div><span class="HOEnZb"><font color="#888888"><br><br clear="all"><br>-- <br><div dir="ltr"><font>Regards,<br>
<br>
<i><b>Manuel BACHMANN</b><br>
Tizen Project<br>
VANNES-FR</i><br>
</font></div>
</font></span></div>
</blockquote></div><div class="gmail_extra"><br><br clear="all"><br>-- <br></div><div dir="ltr" class="gmail_extra"><font>Regards,<br>
<br>
<i><b>Manuel BACHMANN</b><br>
Tizen Project<br>
VANNES-FR</i><br>
</font></div><div class="gmail_extra">
</div></div>