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 < [email protected]>: > > 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 <[email protected]>: > > On Wed, 19 Feb 2014 06:18:18 +0100 >> Manuel Bachmann <[email protected]> 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 <[email protected]> >> > --- >> > 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*
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
