[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