Re: [PATCH weston] Revert "xwm: Add icon support to the frame" and friends

[Daniel Stone]

Hi,

On 29 March 2018 at 15:53, Derek Foreman  wrote:
> On 2018-03-29 09:10 AM, Derek Foreman wrote:
>> Perhaps I should've been more clear as to what "incomplete" means in
>> this commit log.
>>
>> The current code will pick the first available icon unconditionally,
>> regardless as to whether this fits on the titlebar, and no scaling is done.
>>
>> So, as an example, here running terminology under xwayland will result
>> in picking a 128x128 icon, and drawing it as a 16 high 128 wide piece of
>> the icon on the title bar.  when the window closes for some reason the
>> whole icon appears during fade out.
>>
>> It all looks pretty embarrassing.
>>
>> That said, all the known leaks have been fixed, it's just visually
>> disappointing.
>
> Quentin has suggested on IRC that it might be better to just land the
> xwayland/window-manager.c parts of this revert and keep the rest.

Either sounds reasonable to me, though I have a small preference for
just ripping the whole thing out, and later landing one patch which
works from the get-go with no leaks or visual errors. Scott seems to
be super-active on this, so hopefully we can land this pretty much
right after we branch.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] Revert "xwm: Add icon support to the frame" and friends

[Derek Foreman]

On 2018-03-29 09:10 AM, Derek Foreman wrote:
> On 2018-03-29 08:59 AM, Derek Foreman wrote:
>> This reverts commit bef761796c2ada6344d227142af4a0f40b9760dd.
>> This (partially) reverts commit 4d1cd36c9ea041688f92cd8981e43b5fe3b52409.
>>  - the frame_destroy in weston_wm_window_destroy() remains
>> This reverts commit 44fc1be913ab2faad0414f50e51d58310302d065.
>> This (partially) reverts commit e277276b850bad39ed6995be5a82f24aa6b17bf1.
>>  - only the custom icon bits are removed
>> This reverts commit 6b58ea8c43ac81e519bd418efbf24687a5d731b8.
>>
>> The new xwm icon code has proven to be leaky and incomplete, and while
>> we have patches under consideration to fix the rest of its known problems
>> they still require changes and review cycles.  Reverting this all for now
>> for the upcoming release.
>>
> 
> Perhaps I should've been more clear as to what "incomplete" means in
> this commit log.
> 
> The current code will pick the first available icon unconditionally,
> regardless as to whether this fits on the titlebar, and no scaling is done.
> 
> So, as an example, here running terminology under xwayland will result
> in picking a 128x128 icon, and drawing it as a 16 high 128 wide piece of
> the icon on the title bar.  when the window closes for some reason the
> whole icon appears during fade out.
> 
> It all looks pretty embarrassing.
> 
> That said, all the known leaks have been fixed, it's just visually
> disappointing.

Quentin has suggested on IRC that it might be better to just land the
xwayland/window-manager.c parts of this revert and keep the rest.

I'd like to see if anyone else has opinions before I resubmit half this
patch for review.

Thanks,
Derek

> Thanks,
> Derek
> 
>> Signed-off-by: Derek Foreman 
>> ---
>>  clients/window.c   |  4 +-
>>  libweston/compositor-wayland.c |  2 +-
>>  shared/cairo-util.h|  6 +--
>>  shared/frame.c | 76 +---
>>  xwayland/window-manager.c  | 88 
>> +-
>>  5 files changed, 24 insertions(+), 152 deletions(-)
>>
>> diff --git a/clients/window.c b/clients/window.c
>> index bcf2b017..82fbd572 100644
>> --- a/clients/window.c
>> +++ b/clients/window.c
>> @@ -2523,7 +2523,7 @@ window_frame_create(struct window *window, void *data)
>>  
>>  frame = xzalloc(sizeof *frame);
>>  frame->frame = frame_create(window->display->theme, 0, 0,
>> -buttons, window->title, NULL);
>> +buttons, window->title);
>>  
>>  frame->widget = window_add_widget(window, frame);
>>  frame->child = widget_add_widget(frame->widget, data);
>> @@ -5398,7 +5398,7 @@ create_menu(struct display *display,
>>  menu->user_data = user_data;
>>  menu->widget = window_add_widget(menu->window, menu);
>>  menu->frame = frame_create(window->display->theme, 0, 0,
>> -   FRAME_BUTTON_NONE, NULL, NULL);
>> +   FRAME_BUTTON_NONE, NULL);
>>  fail_on_null(menu->frame, 0, __FILE__, __LINE__);
>>  menu->entries = entries;
>>  menu->count = count;
>> diff --git a/libweston/compositor-wayland.c b/libweston/compositor-wayland.c
>> index 111c4c09..5629b7f4 100644
>> --- a/libweston/compositor-wayland.c
>> +++ b/libweston/compositor-wayland.c
>> @@ -869,7 +869,7 @@ wayland_output_set_windowed(struct wayland_output 
>> *output)
>>  return -1;
>>  }
>>  output->frame = frame_create(b->theme, 100, 100,
>> - FRAME_BUTTON_CLOSE, output->title, NULL);
>> + FRAME_BUTTON_CLOSE, output->title);
>>  if (!output->frame)
>>  return -1;
>>  
>> diff --git a/shared/cairo-util.h b/shared/cairo-util.h
>> index 6fd11f6b..9481e58c 100644
>> --- a/shared/cairo-util.h
>> +++ b/shared/cairo-util.h
>> @@ -126,7 +126,7 @@ enum {
>>  
>>  struct frame *
>>  frame_create(struct theme *t, int32_t width, int32_t height, uint32_t 
>> buttons,
>> - const char *title, cairo_surface_t *icon);
>> + const char *title);
>>  
>>  void
>>  frame_destroy(struct frame *frame);
>> @@ -135,10 +135,6 @@ frame_destroy(struct frame *frame);
>>  int
>>  frame_set_title(struct frame *frame, const char *title);
>>  
>> -/* May set FRAME_STATUS_REPAINT */
>> -void
>> -frame_set_icon(struct frame *frame, cairo_surface_t *icon);
>> -
>>  /* May set FRAME_STATUS_REPAINT */
>>  void
>>  frame_set_flag(struct frame *frame, enum frame_flag flag);
>> diff --git a/shared/frame.c b/shared/frame.c
>> index e8a5cad6..83bd7922 100644
>> --- a/shared/frame.c
>> +++ b/shared/frame.c
>> @@ -109,9 +109,9 @@ struct frame {
>>  };
>>  
>>  static struct frame_button *
>> -frame_button_create_from_surface(struct frame *frame, cairo_surface_t *icon,
>> - enum frame_status status_effect,
>> - enum 

Re: [PATCH weston] Revert "xwm: Add icon support to the frame" and friends

[Derek Foreman]

On 2018-03-29 08:59 AM, Derek Foreman wrote:
> This reverts commit bef761796c2ada6344d227142af4a0f40b9760dd.
> This (partially) reverts commit 4d1cd36c9ea041688f92cd8981e43b5fe3b52409.
>  - the frame_destroy in weston_wm_window_destroy() remains
> This reverts commit 44fc1be913ab2faad0414f50e51d58310302d065.
> This (partially) reverts commit e277276b850bad39ed6995be5a82f24aa6b17bf1.
>  - only the custom icon bits are removed
> This reverts commit 6b58ea8c43ac81e519bd418efbf24687a5d731b8.
> 
> The new xwm icon code has proven to be leaky and incomplete, and while
> we have patches under consideration to fix the rest of its known problems
> they still require changes and review cycles.  Reverting this all for now
> for the upcoming release.
> 

Perhaps I should've been more clear as to what "incomplete" means in
this commit log.

The current code will pick the first available icon unconditionally,
regardless as to whether this fits on the titlebar, and no scaling is done.

So, as an example, here running terminology under xwayland will result
in picking a 128x128 icon, and drawing it as a 16 high 128 wide piece of
the icon on the title bar.  when the window closes for some reason the
whole icon appears during fade out.

It all looks pretty embarrassing.

That said, all the known leaks have been fixed, it's just visually
disappointing.

Thanks,
Derek

> Signed-off-by: Derek Foreman 
> ---
>  clients/window.c   |  4 +-
>  libweston/compositor-wayland.c |  2 +-
>  shared/cairo-util.h|  6 +--
>  shared/frame.c | 76 +---
>  xwayland/window-manager.c  | 88 
> +-
>  5 files changed, 24 insertions(+), 152 deletions(-)
> 
> diff --git a/clients/window.c b/clients/window.c
> index bcf2b017..82fbd572 100644
> --- a/clients/window.c
> +++ b/clients/window.c
> @@ -2523,7 +2523,7 @@ window_frame_create(struct window *window, void *data)
>  
>   frame = xzalloc(sizeof *frame);
>   frame->frame = frame_create(window->display->theme, 0, 0,
> - buttons, window->title, NULL);
> + buttons, window->title);
>  
>   frame->widget = window_add_widget(window, frame);
>   frame->child = widget_add_widget(frame->widget, data);
> @@ -5398,7 +5398,7 @@ create_menu(struct display *display,
>   menu->user_data = user_data;
>   menu->widget = window_add_widget(menu->window, menu);
>   menu->frame = frame_create(window->display->theme, 0, 0,
> -FRAME_BUTTON_NONE, NULL, NULL);
> +FRAME_BUTTON_NONE, NULL);
>   fail_on_null(menu->frame, 0, __FILE__, __LINE__);
>   menu->entries = entries;
>   menu->count = count;
> diff --git a/libweston/compositor-wayland.c b/libweston/compositor-wayland.c
> index 111c4c09..5629b7f4 100644
> --- a/libweston/compositor-wayland.c
> +++ b/libweston/compositor-wayland.c
> @@ -869,7 +869,7 @@ wayland_output_set_windowed(struct wayland_output *output)
>   return -1;
>   }
>   output->frame = frame_create(b->theme, 100, 100,
> -  FRAME_BUTTON_CLOSE, output->title, NULL);
> +  FRAME_BUTTON_CLOSE, output->title);
>   if (!output->frame)
>   return -1;
>  
> diff --git a/shared/cairo-util.h b/shared/cairo-util.h
> index 6fd11f6b..9481e58c 100644
> --- a/shared/cairo-util.h
> +++ b/shared/cairo-util.h
> @@ -126,7 +126,7 @@ enum {
>  
>  struct frame *
>  frame_create(struct theme *t, int32_t width, int32_t height, uint32_t 
> buttons,
> - const char *title, cairo_surface_t *icon);
> +  const char *title);
>  
>  void
>  frame_destroy(struct frame *frame);
> @@ -135,10 +135,6 @@ frame_destroy(struct frame *frame);
>  int
>  frame_set_title(struct frame *frame, const char *title);
>  
> -/* May set FRAME_STATUS_REPAINT */
> -void
> -frame_set_icon(struct frame *frame, cairo_surface_t *icon);
> -
>  /* May set FRAME_STATUS_REPAINT */
>  void
>  frame_set_flag(struct frame *frame, enum frame_flag flag);
> diff --git a/shared/frame.c b/shared/frame.c
> index e8a5cad6..83bd7922 100644
> --- a/shared/frame.c
> +++ b/shared/frame.c
> @@ -109,9 +109,9 @@ struct frame {
>  };
>  
>  static struct frame_button *
> -frame_button_create_from_surface(struct frame *frame, cairo_surface_t *icon,
> - enum frame_status status_effect,
> - enum frame_button_flags flags)
> +frame_button_create(struct frame *frame, const char *icon,
> + enum frame_status status_effect,
> + enum frame_button_flags flags)
>  {
>   struct frame_button *button;
>  
> @@ -119,7 +119,12 @@ frame_button_create_from_surface(struct frame *frame, 
> cairo_surface_t *icon,
>   if (!button)
>   return NULL;
>  
> - 

[PATCH weston] Revert "xwm: Add icon support to the frame" and friends

[Derek Foreman]

This reverts commit bef761796c2ada6344d227142af4a0f40b9760dd.
This (partially) reverts commit 4d1cd36c9ea041688f92cd8981e43b5fe3b52409.
 - the frame_destroy in weston_wm_window_destroy() remains
This reverts commit 44fc1be913ab2faad0414f50e51d58310302d065.
This (partially) reverts commit e277276b850bad39ed6995be5a82f24aa6b17bf1.
 - only the custom icon bits are removed
This reverts commit 6b58ea8c43ac81e519bd418efbf24687a5d731b8.

The new xwm icon code has proven to be leaky and incomplete, and while
we have patches under consideration to fix the rest of its known problems
they still require changes and review cycles.  Reverting this all for now
for the upcoming release.

Signed-off-by: Derek Foreman 
---
 clients/window.c   |  4 +-
 libweston/compositor-wayland.c |  2 +-
 shared/cairo-util.h|  6 +--
 shared/frame.c | 76 +---
 xwayland/window-manager.c  | 88 +-
 5 files changed, 24 insertions(+), 152 deletions(-)

diff --git a/clients/window.c b/clients/window.c
index bcf2b017..82fbd572 100644
--- a/clients/window.c
+++ b/clients/window.c
@@ -2523,7 +2523,7 @@ window_frame_create(struct window *window, void *data)
 
frame = xzalloc(sizeof *frame);
frame->frame = frame_create(window->display->theme, 0, 0,
-   buttons, window->title, NULL);
+   buttons, window->title);
 
frame->widget = window_add_widget(window, frame);
frame->child = widget_add_widget(frame->widget, data);
@@ -5398,7 +5398,7 @@ create_menu(struct display *display,
menu->user_data = user_data;
menu->widget = window_add_widget(menu->window, menu);
menu->frame = frame_create(window->display->theme, 0, 0,
-  FRAME_BUTTON_NONE, NULL, NULL);
+  FRAME_BUTTON_NONE, NULL);
fail_on_null(menu->frame, 0, __FILE__, __LINE__);
menu->entries = entries;
menu->count = count;
diff --git a/libweston/compositor-wayland.c b/libweston/compositor-wayland.c
index 111c4c09..5629b7f4 100644
--- a/libweston/compositor-wayland.c
+++ b/libweston/compositor-wayland.c
@@ -869,7 +869,7 @@ wayland_output_set_windowed(struct wayland_output *output)
return -1;
}
output->frame = frame_create(b->theme, 100, 100,
-FRAME_BUTTON_CLOSE, output->title, NULL);
+FRAME_BUTTON_CLOSE, output->title);
if (!output->frame)
return -1;
 
diff --git a/shared/cairo-util.h b/shared/cairo-util.h
index 6fd11f6b..9481e58c 100644
--- a/shared/cairo-util.h
+++ b/shared/cairo-util.h
@@ -126,7 +126,7 @@ enum {
 
 struct frame *
 frame_create(struct theme *t, int32_t width, int32_t height, uint32_t buttons,
- const char *title, cairo_surface_t *icon);
+const char *title);
 
 void
 frame_destroy(struct frame *frame);
@@ -135,10 +135,6 @@ frame_destroy(struct frame *frame);
 int
 frame_set_title(struct frame *frame, const char *title);
 
-/* May set FRAME_STATUS_REPAINT */
-void
-frame_set_icon(struct frame *frame, cairo_surface_t *icon);
-
 /* May set FRAME_STATUS_REPAINT */
 void
 frame_set_flag(struct frame *frame, enum frame_flag flag);
diff --git a/shared/frame.c b/shared/frame.c
index e8a5cad6..83bd7922 100644
--- a/shared/frame.c
+++ b/shared/frame.c
@@ -109,9 +109,9 @@ struct frame {
 };
 
 static struct frame_button *
-frame_button_create_from_surface(struct frame *frame, cairo_surface_t *icon,
- enum frame_status status_effect,
- enum frame_button_flags flags)
+frame_button_create(struct frame *frame, const char *icon,
+   enum frame_status status_effect,
+   enum frame_button_flags flags)
 {
struct frame_button *button;
 
@@ -119,7 +119,12 @@ frame_button_create_from_surface(struct frame *frame, 
cairo_surface_t *icon,
if (!button)
return NULL;
 
-   button->icon = icon;
+   button->icon = cairo_image_surface_create_from_png(icon);
+   if (!button->icon) {
+   free(button);
+   return NULL;
+   }
+
button->frame = frame;
button->flags = flags;
button->status_effect = status_effect;
@@ -129,30 +134,6 @@ frame_button_create_from_surface(struct frame *frame, 
cairo_surface_t *icon,
return button;
 }
 
-static struct frame_button *
-frame_button_create(struct frame *frame, const char *icon_name,
-enum frame_status status_effect,
-enum frame_button_flags flags)
-{
-   struct frame_button *button;
-   cairo_surface_t *icon;
-
-   icon = cairo_image_surface_create_from_png(icon_name);
-   if (cairo_surface_status(icon) != CAIRO_STATUS_SUCCESS)
-  

Re: [PATCH v5 weston] xwm: Choose icon closest to target size

[Derek Foreman]

Since this is a little further from landing than I thought, as is the
scaling patch, I think the best move for the short term is to revert the
icon stuff entirely and move forward after the release.

Thanks,
Derek

On 2018-03-29 04:17 AM, Pekka Paalanen wrote:
> On Tue, 27 Mar 2018 12:43:36 -0500
> Derek Foreman  wrote:
> 
>> Xwayland clients can offer multiple icon sizes in no particular order.
>> Previously xwm was selecting the first one unconditionally. This patch
>> selects the icon that matches the size closest to the target size. The
>> target size is hard coded to 16 since there is only one theme and the
>> data used to create the theme is hard coded.
>>
>> Co-authored-by: Scott Moreau 
>>
> 
> Missing signed-off-by.
> 
> Scott, are you happy with your attribution here?
> 
> 
>> ---
>>
>> Changed in v2:
>>
>> - Fix typo setting width to height
>>
>> Changed in v3:
>>
>> - Move checks for malformed input into data handling function
>>
>> Changed in v4:
>>
>> - #define XWM_ICON_SIZE in this patch and do not #undef it
>> - Start with first icon found before choosing icon\
>> - Check for NULL data in get_icon_size_from_data()
>>
>> Changed in v5:
>> - remove allocations, process in a single pass
>> - rebase on top of memory leak fix
>>
>>  xwayland/window-manager.c | 51 
>> ---
>>  1 file changed, 44 insertions(+), 7 deletions(-)
>>
>> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
>> index dad117fa..5209ac66 100644
>> --- a/xwayland/window-manager.c
>> +++ b/xwayland/window-manager.c
>> @@ -127,6 +127,8 @@ struct motif_wm_hints {
>>  #define _NET_WM_MOVERESIZE_MOVE_KEYBOARD10   /* move via keyboard */
>>  #define _NET_WM_MOVERESIZE_CANCEL   11   /* cancel operation */
>>  
>> +#define XWM_ICON_SIZE 16 /* width and height of frame icon */
>> +
>>  struct weston_output_weak_ref {
>>  struct weston_output *output;
>>  struct wl_listener destroy_listener;
>> @@ -1352,6 +1354,44 @@ weston_wm_window_schedule_repaint(struct 
>> weston_wm_window *window)
>> weston_wm_window_do_repaint, window);
>>  }
>>  
>> +static uint32_t *
>> +get_icon_size_from_data(int target_width, int target_height,
>> +uint32_t *width, uint32_t *height,
>> +uint32_t length, uint32_t *data)
> 
> This may be a static function, but it really needs doxygen doc to
> explain all the arguments and return value, particularly for 'length',
> see below.
> 
>> +{
>> +uint32_t *d = data, *ret = NULL, w, h;
>> +int a1, a2, a3;
> 
> I'd really like better names than a1, a2, a3. There are so many
> variables in this function that they need descriptive names.
> 
>> +
>> +if (!data)
>> +return NULL;
>> +
>> +/* Choose closest match by comparing icon dimension areas */
>> +a1 = target_width * target_height;
>> +
>> +*width = *height = 0;
>> +
>> +while (d - data < length / 4) {
> 
> Maybe using
>   uint32_t *end = data + length;
> would make this easier to read.
> 
> Isn't length/4 wrong, because length is already in uint32_t units by
> the caller?
> 
>> +w = *d++;
>> +h = *d++;
>> +
>> +/* Some checks against malformed input. */
>> +if (w == 0 || h == 0 || length < 2 + w * h)
> 
> Checking against 'length' is incorrect, because we need to be checking
> against the remaining length, not against the whole length, as we may
> have skipped an icon alrady. Checking against 'end' would help here too.
> 
>> +return ret;
>> +
>> +a2 = w * h;
>> +a3 = *width * *height;
>> +if (!a3 || abs(a2 - a1) < abs(a3 - a1)) {
>> +*width = w;
>> +*height = h;
>> +ret = d;
>> +}
>> +
>> +d += a2;
> 
> The computations here seem to be correct.
> 
>> +}
>> +
>> +return ret;
>> +}
>> +
>>  static void
>>  handle_icon_surface_destroy(void *data)
>>  {
>> @@ -1367,9 +1407,6 @@ weston_wm_handle_icon(struct weston_wm *wm, struct 
>> weston_wm_window *window)
>>  uint32_t *data, width, height;
>>  cairo_surface_t *new_surface;
>>  
>> -/* TODO: icons don’t have any specified order, we should pick the
>> - * closest one to the target dimension instead of the first one. */
>> -
>>  cookie = xcb_get_property(wm->conn, 0, window->id,
>>wm->atom.net_wm_icon, XCB_ATOM_ANY, 0,
>>UINT32_MAX);
> 
> Type is XCB_ATOM_ANY.
> 
> The context here is missing to show this:
> 
>   reply = xcb_get_property_reply(wm->conn, cookie, NULL);
>   length = xcb_get_property_value_length(reply);
> 
>   /* This is in 32-bit words, not in bytes. */
>   if (length < 2) {
>   free(reply);
>   return;
>   }
> 
>   data = 

Re: [PATCH] xwm: Fix wrong input offset for certain clients

[Pekka Paalanen]

On Sun, 18 Mar 2018 12:22:17 -0600
Scott Moreau  wrote:

> Some windows might get a create_notify event without the
> override redirect flag set and then get a confiure_notify
> event before map_request is received. This means that when
> weston_wm_window_get_child_position is called in response
> to configure_notify, the wrong offsets are computed resulting
> in wrong input offsets for some clients like steam.

Hi,

do I understand correctly that the window is never set as
override-redirect?

> ---
>  xwayland/window-manager.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index c307e19..2307f1a 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -711,10 +711,12 @@ weston_wm_handle_configure_request(struct weston_wm 
> *wm, xcb_generic_event_t *ev
>   if (configure_request->value_mask & XCB_CONFIG_WINDOW_HEIGHT)
>   window->height = configure_request->height;
>  
> - if (window->frame)
> + if (window->frame) {
>   frame_resize_inside(window->frame, window->width, 
> window->height);
> + weston_wm_window_get_child_position(window, , );
> + } else
> + x = y = 0;

The else branch should have braces as well because the other branch
does.

>  
> - weston_wm_window_get_child_position(window, , );
>   values[i++] = x;
>   values[i++] = y;
>   values[i++] = window->width;

For a not yet mapped X11 window, this change looks correct. If there is
no 'window->frame', then the app window has not been reparented either,
which means there is no offset we could apply. In that case, the x, y
will be relative to the root window. We can set them to any number,
e.g. zero, because the window has not been mapped yet. Only when we get
MapRequest and content, the Wayland window manager may actually pick a
position and then we move the X11 window stack to its appropriate
coordinates.

However, if we get a ConfigureRequest for a window that has already
been mapped, and if that window happens to not have a frame, would this
not smash the window to the top-left corner of the screen?

That raises an interesting question: can we have a window that does not
have a frame and also is not override-redirect (O-R means we never see
the ConfigureRequest)?

If XWM sees the MapRequest, it practically always creates the frame. So
the only way to hit that corner case is, if the X11 client creates the
window as a normal window, changes it to O-R, maps it, changes it to
non-O-R, and then attempts to move. Maybe we can live with that, seems
like a pretty deep hole the X11 client is digging?

But if we have your patch to handle O-R state changes, then I suppose
we wouldn't have this corner-case at all, because it would create the
frame, right?

I have to say I don't quite understand how the wrong offsets this patch
fixes can confuse an X11 client's input. Nevertheless, I think I can
give:

Reviewed-by: Pekka Paalanen 

unless something I said here is not true.


I still wonder... the Wayland window manager picks the window position
when it maps it, so could this patch be just a bandaid for a very
specific case, and the real bug is actually somewhere in the Wayland
window manager side for forgetting to call ->send_position() in this
specific case. If that's true, then sending the zeroes like this patch
does is just accidentally correct for the app you tested with.


Thanks,
pq


pgpjhEx9DMzM_.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] xwm: Send configure event when moving

[Pekka Paalanen]

On Sun, 18 Mar 2018 12:22:16 -0600
Scott Moreau  wrote:

> Send a synthetic configure notify event to the reparented window to
> update the position in Xwayland. This fixes menu positioning in clients
> like VLC after moving the window.

Hi,

Signed-off-by missing, I would strongly recommend to add it to say you
comply with https://developercertificate.org/ . You can do it by simply
replying to this email with the tag.

It's not absolutely required by our contribution guidelines yet, but it
might in the future. It is what we have always assumed, S-o-b just
makes it explicit.

> ---
>  xwayland/window-manager.c | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index c307e19..542e9fb 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -659,13 +659,12 @@ weston_wm_window_get_input_rect(struct weston_wm_window 
> *window,
>  }
>  
>  static void
> -weston_wm_window_send_configure_notify(struct weston_wm_window *window)
> +weston_wm_window_send_configure_notify(struct weston_wm_window *window,
> + 
> int x, int y)

The first "int" should be aligned with "struct" on the previous line.

>  {
>   xcb_configure_notify_event_t configure_notify;
>   struct weston_wm *wm = window->wm;
> - int x, y;
>  
> - weston_wm_window_get_child_position(window, , );
>   configure_notify.response_type = XCB_CONFIGURE_NOTIFY;
>   configure_notify.pad0 = 0;
>   configure_notify.event = window->id;
> @@ -702,7 +701,7 @@ weston_wm_handle_configure_request(struct weston_wm *wm, 
> xcb_generic_event_t *ev
>   return;
>  
>   if (window->fullscreen) {
> - weston_wm_window_send_configure_notify(window);
> + weston_wm_window_send_configure_notify(window, 0, 0);
>   return;
>   }
>  
> @@ -2734,6 +2733,7 @@ send_position(struct weston_surface *surface, int32_t 
> x, int32_t y)
>   struct weston_wm_window *window = get_wm_window(surface);
>   struct weston_wm *wm;
>   uint32_t mask, values[2];
> + int sx, sy;
>  
>   if (!window || !window->wm)
>   return;
> @@ -2750,6 +2750,10 @@ send_position(struct weston_surface *surface, int32_t 
> x, int32_t y)
>   mask = XCB_CONFIG_WINDOW_X | XCB_CONFIG_WINDOW_Y;
>  
>   xcb_configure_window(wm->conn, window->frame_id, mask, values);
> +
> + weston_wm_window_get_child_position(window, , );
> + weston_wm_window_send_configure_notify(window, x + sx, y + sy);

https://tronche.com/gui/x/xlib/events/window-state-change/configure.html
says:
"The x and y members are set to the coordinates relative to the
parent window's origin and indicate the position of the
upper-left outside corner of the window."

I tested with kcachegrind, which without this patch gets combobox popup
windows positioned wrong, and this patch as is fixes it indeed. I also
tried sending just sx instead of x + sx, and it breaks it again. So
while this code seems to work right, I would like to know why Tronche
is seemingly wrong?

https://stackoverflow.com/questions/25391791/x11-configurenotify-always-returning-x-y-0-0
seems to agree with Tronche.

So perhaps this is not the right fix?

A quick look at
https://lists.freedesktop.org/archives/xorg/2008-November/040184.html
makes this ever more... interesting.

Is this one of those things that someone once got wrong in the ancient
days, and then literally everyone fixed their programs to comply with
the spec-violating behaviour?

> +
>   xcb_flush(wm->conn);
>   }
>  }

Thanks,
pq


pgp1ZdXXWNQsb.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] xwm: Handle changing override redirect flag

[Pekka Paalanen]

On Sun, 18 Mar 2018 12:22:15 -0600
Scott Moreau  wrote:

> Xwayland windows might be created with a different override redirect
> flag than is given on map or configure notify. This causes confusion
> about whether a window should be treated as override redirect or not.
> Here we handle the changing override redirect flag in relevant notify
> handlers so windows are treated appropriately. In particular, this
> fixes positioning menus in clients like VLC, though it is not a
> complete fix. If the window is moved, the menus are still positioned
> as if the window wasn't moved.
> ---
>  xwayland/window-manager.c | 43 +++
>  1 file changed, 35 insertions(+), 8 deletions(-)

Hi,

this patch looks big enough that I'd consider it more a new feature
than just a bug fix, so I'd like to postpone this after the 4.0.0
release.

I've marked this as deferred in Patchwork.


Thanks,
pq


pgpogUW16h_6N.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v4 2/2] xwm: Scale icon if size is not the target size

[Pekka Paalanen]

On Fri, 23 Mar 2018 13:47:33 -0600
Scott Moreau  wrote:

> This scales the icon cairo surface for the titlebar if it isn't the
> target size.
> 
> shared/cairo-util: Add surface resizing function to be used for this
> case and other potential cases.
> ---
> 
> Changed in v2:
> 
> - Rebase to [PATCH 1/1 v3] xwm: Choose icon closest to target size
> 
> Changed in v3:
> 
> - No changes
> 
> Changed in v4:
> 
> - Fixed whitespace problems
> - Renamed cairo_resize_surface() to resize_cairo_surface()
> 
>  shared/cairo-util.c   | 63 
> +++
>  shared/cairo-util.h   |  4 +++
>  xwayland/window-manager.c |  4 +++
>  3 files changed, 71 insertions(+)
> 

Hi Scott,

I appreciate the effort and this is a feature I would like to have in
Weston. I have come questions below, though.

> diff --git a/shared/cairo-util.c b/shared/cairo-util.c
> index d71e0ed..442182b 100644
> --- a/shared/cairo-util.c
> +++ b/shared/cairo-util.c
> @@ -365,6 +365,69 @@ load_cairo_surface(const char *filename)
>  width, height, stride);
>  }
>  
> +static cairo_surface_t *
> +scale_surface(cairo_surface_t *source, cairo_filter_t filter,
> + double width, double height)

Double seems an inappropriate type here, a surface size must always be
an integer.

The second line's first word "double" should be aligned to the previous
line's 'c' right after the opening parenthesis.

> +{
> + cairo_surface_t *dest;
> + cairo_t *cr;
> + int old_width, old_height;
> +
> + old_width = cairo_image_surface_get_width(source);
> + old_height = cairo_image_surface_get_height(source);
> +
> + dest = cairo_surface_create_similar(source,
> + CAIRO_CONTENT_COLOR_ALPHA,
> + width, height);
> + cr = cairo_create (dest);
> +
> + cairo_scale (cr, width / old_width, height / old_height);

We don't use a space between the function name and the opening
parenthesis.

> + cairo_set_source_surface (cr, source, 0, 0);
> +
> + cairo_pattern_set_extend (cairo_get_source(cr),
> + CAIRO_EXTEND_REFLECT);

Is there no need to set the filter as well? Looks like unused argument.

> +
> + cairo_set_operator (cr, CAIRO_OPERATOR_SOURCE);
> +
> + cairo_paint (cr);
> +
> + cairo_destroy (cr);
> +
> + cairo_surface_destroy(source);
> +
> + return dest;
> +}
> +
> +cairo_surface_t *
> +resize_cairo_surface(cairo_surface_t *source, cairo_filter_t filter,
> + int width, int height)
> +{
> + if (!filter)
> + filter = CAIRO_FILTER_BEST;
> +
> + while((cairo_image_surface_get_width(source) / 2.0f) > width)

Needs a space between a C keyword and an opening parenthesis.

> + source = scale_surface(source, filter,
> + cairo_image_surface_get_width(source) / 2.0f,
> + cairo_image_surface_get_height(source));
> +
> + while((cairo_image_surface_get_height(source) / 2.0f) > height)
> + source = scale_surface(source, filter,
> + cairo_image_surface_get_width(source),
> + cairo_image_surface_get_height(source) / 2.0f);
> +
> + while((cairo_image_surface_get_width(source) * 2.0f) < width)
> + source = scale_surface(source, filter,
> + cairo_image_surface_get_width(source) * 2.0f,
> + cairo_image_surface_get_height(source));
> +
> + while((cairo_image_surface_get_height(source) * 2.0f) < height)
> + source = scale_surface(source, filter,
> + cairo_image_surface_get_width(source),
> + cairo_image_surface_get_height(source) * 2.0f);

Why bother with all this? The answer should be in the commit message,
and "other potential cases" is not it if you cannot actually name a
case that will be used in Weston (e.g. you know you will write or have
already written a patch to use it).

I understand that it is to compute a good quality minification by doing
the convolution in factor of two steps, but do we really need that?
Especially when we pick a size closest to the wanted size, the code
complexity here seems unwarranted. I think the very least it requires a
comment explaining why this is done like it is.

I don't deny that there could be a good reason for this, I just want
you to explicitly record it.

For magnification, does this make any difference to a single-step
scaling?

> +
> + return scale_surface(source, filter, width, height);

I would have thought this last statement is all this function actually
needs to do, but I could accept a rationale for the step-wise
minification e.g. if you say it really actually looks bad if done in a
single step.

> +}
> +
>  void
>  theme_set_background_source(struct theme *t, 

Re: [PATCH v5 weston] xwm: Choose icon closest to target size

[Pekka Paalanen]

On Thu, 29 Mar 2018 12:17:46 +0300
Pekka Paalanen  wrote:

> On Tue, 27 Mar 2018 12:43:36 -0500
> Derek Foreman  wrote:
> 
> > Xwayland clients can offer multiple icon sizes in no particular order.
> > Previously xwm was selecting the first one unconditionally. This patch
> > selects the icon that matches the size closest to the target size. The
> > target size is hard coded to 16 since there is only one theme and the
> > data used to create the theme is hard coded.
> > 
> > Co-authored-by: Scott Moreau 

> > +static uint32_t *
> > +get_icon_size_from_data(int target_width, int target_height,
> > +   uint32_t *width, uint32_t *height,
> > +   uint32_t length, uint32_t *data)  

...

> > +   return ret;
> > +
> > +   a2 = w * h;
> > +   a3 = *width * *height;
> > +   if (!a3 || abs(a2 - a1) < abs(a3 - a1)) {
> > +   *width = w;
> > +   *height = h;
> > +   ret = d;
> > +   }
> > +
> > +   d += a2;  
> 
> The computations here seem to be correct.

Oh btw, wasn't the intention to pick the closest *larger* sized icon
from the list and then downscale? Only pick smaller if there is no
larger provided.

This seems to pick the closest larger or smaller in terms of area.


Thanks,
pq


pgpBzA5DvHpZa.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v5 weston] xwm: Choose icon closest to target size

[Pekka Paalanen]

On Tue, 27 Mar 2018 12:43:36 -0500
Derek Foreman  wrote:

> Xwayland clients can offer multiple icon sizes in no particular order.
> Previously xwm was selecting the first one unconditionally. This patch
> selects the icon that matches the size closest to the target size. The
> target size is hard coded to 16 since there is only one theme and the
> data used to create the theme is hard coded.
> 
> Co-authored-by: Scott Moreau 
> 

Missing signed-off-by.

Scott, are you happy with your attribution here?


> ---
> 
> Changed in v2:
> 
> - Fix typo setting width to height
> 
> Changed in v3:
> 
> - Move checks for malformed input into data handling function
> 
> Changed in v4:
> 
> - #define XWM_ICON_SIZE in this patch and do not #undef it
> - Start with first icon found before choosing icon\
> - Check for NULL data in get_icon_size_from_data()
> 
> Changed in v5:
> - remove allocations, process in a single pass
> - rebase on top of memory leak fix
> 
>  xwayland/window-manager.c | 51 
> ---
>  1 file changed, 44 insertions(+), 7 deletions(-)
> 
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index dad117fa..5209ac66 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -127,6 +127,8 @@ struct motif_wm_hints {
>  #define _NET_WM_MOVERESIZE_MOVE_KEYBOARD10   /* move via keyboard */
>  #define _NET_WM_MOVERESIZE_CANCEL   11   /* cancel operation */
>  
> +#define XWM_ICON_SIZE 16 /* width and height of frame icon */
> +
>  struct weston_output_weak_ref {
>   struct weston_output *output;
>   struct wl_listener destroy_listener;
> @@ -1352,6 +1354,44 @@ weston_wm_window_schedule_repaint(struct 
> weston_wm_window *window)
>  weston_wm_window_do_repaint, window);
>  }
>  
> +static uint32_t *
> +get_icon_size_from_data(int target_width, int target_height,
> + uint32_t *width, uint32_t *height,
> + uint32_t length, uint32_t *data)

This may be a static function, but it really needs doxygen doc to
explain all the arguments and return value, particularly for 'length',
see below.

> +{
> + uint32_t *d = data, *ret = NULL, w, h;
> + int a1, a2, a3;

I'd really like better names than a1, a2, a3. There are so many
variables in this function that they need descriptive names.

> +
> + if (!data)
> + return NULL;
> +
> + /* Choose closest match by comparing icon dimension areas */
> + a1 = target_width * target_height;
> +
> + *width = *height = 0;
> +
> + while (d - data < length / 4) {

Maybe using
uint32_t *end = data + length;
would make this easier to read.

Isn't length/4 wrong, because length is already in uint32_t units by
the caller?

> + w = *d++;
> + h = *d++;
> +
> + /* Some checks against malformed input. */
> + if (w == 0 || h == 0 || length < 2 + w * h)

Checking against 'length' is incorrect, because we need to be checking
against the remaining length, not against the whole length, as we may
have skipped an icon alrady. Checking against 'end' would help here too.

> + return ret;
> +
> + a2 = w * h;
> + a3 = *width * *height;
> + if (!a3 || abs(a2 - a1) < abs(a3 - a1)) {
> + *width = w;
> + *height = h;
> + ret = d;
> + }
> +
> + d += a2;

The computations here seem to be correct.

> + }
> +
> + return ret;
> +}
> +
>  static void
>  handle_icon_surface_destroy(void *data)
>  {
> @@ -1367,9 +1407,6 @@ weston_wm_handle_icon(struct weston_wm *wm, struct 
> weston_wm_window *window)
>   uint32_t *data, width, height;
>   cairo_surface_t *new_surface;
>  
> - /* TODO: icons don’t have any specified order, we should pick the
> -  * closest one to the target dimension instead of the first one. */
> -
>   cookie = xcb_get_property(wm->conn, 0, window->id,
> wm->atom.net_wm_icon, XCB_ATOM_ANY, 0,
> UINT32_MAX);

Type is XCB_ATOM_ANY.

The context here is missing to show this:

reply = xcb_get_property_reply(wm->conn, cookie, NULL);
length = xcb_get_property_value_length(reply);

/* This is in 32-bit words, not in bytes. */
if (length < 2) {
free(reply);
return;
}

data = xcb_get_property_value(reply);

What are the units of 'length'?

The comment here says words, but if you look at the example in 'man
xcb_get_property', it is in bytes. If they are both right, then the
length unit must depend on something.

The type is not checked from the reply, and neither is 'format' checked
from the reply. My wild guess would be that 'format' determines the
units of 'length', and so it is controllable