Re: [PATCH 3/5] shell: add managed_surface interface, request and events

2014-02-26 Thread Manuel Bachmann 
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.bachm...@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 ppaala...@gmail.com:

 On Wed, 19 Feb 2014 06:18:18 +0100
 Manuel Bachmann manuel.bachm...@open.eurogiciel.org wrote:

  We create a new managed_surface object which will track
  a

Re: [PATCH 3/5] shell: add managed_surface interface, request and events

2014-02-19 Thread Pekka Paalanen 
On Wed, 19 Feb 2014 06:18:18 +0100
Manuel Bachmann manuel.bachm...@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.bachm...@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
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH 3/5] shell: add managed_surface interface, request and events

2014-02-19 Thread Manuel Bachmann 
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 ppaala...@gmail.com:

 On Wed, 19 Feb 2014 06:18:18 +0100
 Manuel Bachmann manuel.bachm...@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.bachm...@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
  ---

[PATCH 3/5] shell: add managed_surface interface, request and events

2014-02-18 Thread Manuel Bachmann 
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.bachm...@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(+)

diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c
index c341a91..5f861b2 100644
--- a/clients/desktop-shell.c
+++ b/clients/desktop-shell.c
@@ -139,6 +139,7 @@ struct taskbar_handler {
struct taskbar *taskbar;
cairo_surface_t *icon;
int focused, pressed;
+   struct managed_surface *surface;
char *title;
int state;
struct wl_list link;
@@ -186,6 +187,14 @@ show_menu(struct panel *panel, struct input *input, 
uint32_t time)
 x - 10, y - 10, menu_func, entries, 4);
 }
 
+static void
+update_window(struct window *window)
+{
+   struct rectangle allocation;
+   window_get_allocation(window, allocation);
+   window_schedule_resize(window, allocation.width, allocation.height);
+}
+
 static int
 is_desktop_painted(struct desktop *desktop)
 {
@@ -1113,6 +1122,20 @@ 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) {
+   /* will follow in next patch : add the actual handler here */
+   update_window(output-taskbar-window);
+   }
+}
+
+static void
 desktop_shell_grab_cursor(void *data,
  struct desktop_shell *desktop_shell,
  uint32_t cursor)
@@ -1162,10 +1185,56 @@ desktop_shell_grab_cursor(void *data,
 static const struct desktop_shell_listener listener = {
desktop_shell_configure,
desktop_shell_prepare_lock_surface,
+   desktop_shell_add_managed_surface,
desktop_shell_grab_cursor
 };
 
 static void
+managed_surface_state_changed(void *data,
+   struct managed_surface *managed_surface,
+   uint32_t state)
+{
+   struct taskbar_handler *handler = data;
+
+   if (handler-surface == managed_surface) {
+   /* set the handler state */
+   handler-state = state;
+   }
+}
+
+static void
+managed_surface_title_changed(void *data,
+   struct managed_surface *managed_surface,
+   const char *title)
+{
+   struct taskbar_handler *handler = data;
+
+   if (handler-surface == managed_surface) {
+   /* change the handler title text */
+   handler-title = strdup(title);
+   update_window(handler-taskbar-window);
+   }
+}
+
+static void
+managed_surface_removed(void *data,
+   struct managed_surface *managed_surface)
+{
+   struct taskbar_handler *handler = data;
+
+   if (handler-surface == managed_surface) {
+   /* will follow in next patch : destroy the actual handler here 
*/
+   update_window(handler-taskbar-window);
+   }
+}
+
+static const struct managed_surface_listener managed_surface_listener = {
+   managed_surface_state_changed,
+   managed_surface_title_changed,
+   managed_surface_removed
+};
+
+static void
 background_destroy(struct background *background)
 {
widget_destroy(background-widget);
diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index b9b4ad9..57afe5b 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -2023,6 +2023,14 @@ set_title(struct shell_surface *shsurf, const char 
*title)
 {
free(shsurf-title);
shsurf-title = strdup(title);
+
+   if (shsurf-type == SHELL_SURFACE_TOPLEVEL) {
+   struct managed_surface *managed_surface;
+   wl_list_for_each(managed_surface, 
shsurf-shell-managed_surfaces_list, link) {
+   if (managed_surface-surface == shsurf-surface)
+   managed_surface_send_title_changed 
(managed_surface-resource, shsurf-title);
+   }
+   }
 }
 
 static void
@@ -2991,6 +2999,16 @@ destroy_shell_surface(struct shell_surface *shsurf)
remove_popup_grab(shsurf);
}
 
+   if (shsurf-type == SHELL_SURFACE_TOPLEVEL) {
+   struct managed_surface *managed_surface;
+   wl_list_for_each(managed_surface, 
shsurf-shell-managed_surfaces_list, link) {
+   if