[PATCH 3/5] shell: add managed_surface interface, request and events
Manuel Bachmann
manuel.bachmann at open.eurogiciel.org
Wed Feb 26 06:48:25 PST 2014
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,
https://github.com/Tarnyko/weston-taskbar/commits/HEAD-taskbar. I could
refactor and repost the whole set here.
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.
Regards,
Manuel
2014-02-19 14:04 GMT+01:00 Manuel Bachmann <
manuel.bachmann at open.eurogiciel.org>:
>
> 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*
>
--
Regards,
*Manuel BACHMANN Tizen Project VANNES-FR*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140226/b0495881/attachment-0001.html>
More information about the wayland-devel
mailing list