[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