Created attachment 333875 [details] [review]
[PATCH] wayland: add min/max size from xdg-shell v6

Patch has been rebased on top of https://github.com/jadahl/mutter.git :wip/xdg-shell-v6

Anyone to review this patch?

Review of attachment 333875 [details] [review]:

::: src/wayland/meta-wayland-surface.h
@@ +148,3 @@
+  gboolean has_new_min_geometry;
+  MetaRectangle max_geometry;
+  gboolean has_new_max_geometry;

I'd rather see we use a more correct data type even if it means having two 'int'. It's also not a "min/max" geometry, its a min/max size. The geometry includes an position offset.

::: src/wayland/meta-wayland-xdg-shell.c
@@ +634,3 @@
+       check_valid_size_hints (window);
+       meta_window_wayland_update_size_hints (window);
+    }

I'd rather see this as something like

static gboolean
is_new_size_hints_valid (MetaWindow              *window,
                         MetaWaylandPendingState *pending)
{
  ...
}

if (is_new_size_hints_valid (window, pending))
  {
    actually_set_new_size_hints (window, pending);
  }
else
  {
    wl_resource_post_error (...);
  }

So that we don't actually set things it in a function called "check".

::: src/wayland/meta-window-wayland.c
@@ +54,3 @@
+  /* min/max size in window geometry coordinates */
+  MetaRectangle min_size;
+  MetaRectangle max_size;

Do we really need these? Whats wrong with the window->size_hints (except being an X11 data type)?

@@ +499,3 @@
   MetaScreen *scr = display->screen;
   MetaWindow *window;
+  MetaRectangle none = {0, 0, 0, 0};

nit: can be { 0 };

@@ +730,3 @@
+                        window->custom_frame_extents.bottom);
+      else
+        size.height = G_MAXINT;

Won't this potentially overflow if we scale up?

::: src/wayland/meta-window-wayland.h
@@ +65,3 @@
+
+void meta_window_wayland_set_min_size (MetaWindow    *window,
+                                       MetaRectangle *size);

Why is size a rectangle? Makes more sense to have it just 'int *with, int *height' if we don't have a data type that represents that.

(In reply to Jonas Ådahl from comment #4)
> Review of attachment 333875 [details] [review] [review]:
> [...]
> ::: src/wayland/meta-window-wayland.c
> @@ +54,3 @@
> +  /* min/max size in window geometry coordinates */
> +  MetaRectangle min_size;
> +  MetaRectangle max_size;
> 
> Do we really need these? Whats wrong with the window->size_hints (except
> being an X11 data type)?

Yes, I think we do, because min/max size here are in window geometry coordinates and need to be adjusted depending in the output scale.

So we need to keep those around when the window is move to another output with a different scaling factor, so that we can then adjust the window->size_hints which is in pixels.

(In reply to Olivier Fourdan from comment #5)
> (In reply to Jonas Ådahl from comment #4)
> > Review of attachment 333875 [details] [review] [review] [review]:
> > [...]
> > ::: src/wayland/meta-window-wayland.c
> > @@ +54,3 @@
> > +  /* min/max size in window geometry coordinates */
> > +  MetaRectangle min_size;
> > +  MetaRectangle max_size;
> > 
> > Do we really need these? Whats wrong with the window->size_hints (except
> > being an X11 data type)?
> 
> Yes, I think we do, because min/max size here are in window geometry
> coordinates and need to be adjusted depending in the output scale.
> 
> So we need to keep those around when the window is move to another output
> with a different scaling factor, so that we can then adjust the
> window->size_hints which is in pixels.

Can't those be scaled properly still? I mean we know the scale they were in, and we know the scale they are to be converted to, so we know how to scale them, without using the non-scaled dimensions. See how scale_rect_size() is used on window->rect, window->unconstrained_rect and window->saved_rect.

(In reply to Jonas Ådahl from comment #6)
> Can't those be scaled properly still? I mean we know the scale they were in,
> and we know the scale they are to be converted to, so we know how to scale
> them, without using the non-scaled dimensions. See how scale_rect_size() is
> used on window->rect, window->unconstrained_rect and window->saved_rect.

Yes, that's how I did it initially, but I found this adds needless complexity to a rather simple code otherwise. But I can change that if needed.

(In reply to Olivier Fourdan from comment #7)
> (In reply to Jonas Ådahl from comment #6)
> > Can't those be scaled properly still? I mean we know the scale they were in,
> > and we know the scale they are to be converted to, so we know how to scale
> > them, without using the non-scaled dimensions. See how scale_rect_size() is
> > used on window->rect, window->unconstrained_rect and window->saved_rect.
> 
> Yes, that's how I did it initially, but I found this adds needless
> complexity to a rather simple code otherwise. But I can change that if
> needed.

As it'd be just another scale_rect_size (window->..., scale_factor); next to all the others I'd say it'd be less complex doing it that way.

No, it's all that simple because mutter uses G_MAXINT when the max size is not set, so we need to test for those special cases and not scale them, which is ugly.

See meta_set_normal_hints()

   https://git.gnome.org/browse/mutter/tree/src/x11/window-props.c#n1223

Apparently, this way, it doesn;t have to check for the actual value of the size flags once this is done...

(In reply to Olivier Fourdan from comment #9)
> No, it's all that simple because mutter uses G_MAXINT when the max size is
> not set, so we need to test for those special cases and not scale them,
> which is ugly.
> 
> See meta_set_normal_hints()
> 
>    https://git.gnome.org/browse/mutter/tree/src/x11/window-props.c#n1223
> 
> Apparently, this way, it doesn;t have to check for the actual value of the
> size flags once this is done...

Ah, I see. Can't scale_rect_size be fixed to handle widtdh/height == INT_MAX as a special case?

(In reply to Jonas Ådahl from comment #10)
> Ah, I see. Can't scale_rect_size be fixed to handle widtdh/height == INT_MAX
> as a special case?

Yes, that would be the least ugly :) I'll try that instead (reason I initially ruled this out was to avoid changing code shared elsewhere)

Created attachment 334664 [details] [review]
[PATCH] wayland: add min/max size from xdg-shell v6

Updated patch - This ended up being a lot more work that anticipated actually, but that seems to work.

Created attachment 334665 [details] [review]
[PATCH] wayland: add min/max size from xdg-shell v6

Note: I tested only in mutter as I have some difficulties building/running gnome-shell from git master at the moment...

Review of attachment 334665 [details] [review]:

::: src/wayland/meta-wayland-xdg-shell.c
@@ +630,3 @@
+
+      return TRUE;
+    }

Couldn't this function be simplified to

int new_min_width, new_min_height;
int new_max_width, new_max_height;

if (pending->has_new_min_size)
  {
    new_min_width = pending->new_min_width;
    new_min_height = pending->new_min_height;
  }
else
  {
    meta_window_wayland_get_min_size (window, &new_min_width, &new_min_height);
  }


if (pending->has_new_max_size)
  {
    new_max_width = pending->new_max_width;
    new_max_height = pending->new_max_height;
  }
else
  {
    meta_window_wayland_get_min_size (window, &new_max_width, &new_max_height);
  }

return new_min_width <= new_max_width && new_min_height <= new_max_height;


?

@@ +687,3 @@
+
+          meta_window_recalc_features (window);
+        }

I think a "is_...()" function should still be side-effect less, i.e. post error here. I don't think we need any more error message than "size limit invalid".

::: src/wayland/meta-window-wayland.c
@@ +334,1 @@
 }

nit: Actually think it'd be nicer to have

static void
scale_size (int  *width
            int  *height,
            float scale)
{
  if (width < INT_MAX)
    *width = (int)(width * scale);
  if (height < INT_MAX)
    *height = (int)(height * scale);
}

static void
scale_rect_size (MetaRectangle *rect,
                 float          scale)
{
  scale_size (&rect->width, &rect->height, scale);
}

so that you don't have to create all those dummy rects.

@@ +704,3 @@
+    rect.width = MIN (G_MAXINT,
+                      rect.width + (window->custom_frame_extents.left +
+                                    window->custom_frame_extents.right));

Hmm. MIN (INT_MAX, ...) feels like ... will always be less (than the highest number possible). Or did you mean to make the right hand expression a (uint64_t) so that it can actually be greater?

(In reply to Jonas Ådahl from comment #15)
> Review of attachment 334665 [details] [review] [review]:
> 
> ::: src/wayland/meta-wayland-xdg-shell.c
> @@ +630,3 @@
> +
> +      return TRUE;
> +    }
> 
> Couldn't this function be simplified to
> 
> int new_min_width, new_min_height;
> int new_max_width, new_max_height;
> 
> if (pending->has_new_min_size)
>   {
>     new_min_width = pending->new_min_width;
>     new_min_height = pending->new_min_height;
>   }
> else
>   {
>     meta_window_wayland_get_min_size (window, &new_min_width,
> &new_min_height);
>   }
> 
> 
> if (pending->has_new_max_size)
>   {
>     new_max_width = pending->new_max_width;
>     new_max_height = pending->new_max_height;
>   }
> else
>   {
>     meta_window_wayland_get_min_size (window, &new_max_width,
> &new_max_height);
>   }
> 
> return new_min_width <= new_max_width && new_min_height <= new_max_height;

Tempting, but we still need to check for 0, as a value size of 0 in a new request means it's unlimited, so a client may request a new max size with one dimension set and the other not.

That doesn't matter for min size, but it does for max size. Another possibility is to set a max size of 0 in get_max_size for unlimited values so that we remain consistent in sizes semantics and the test would work in all cases.
 
> @@ +687,3 @@
> +
> +          meta_window_recalc_features (window);
> +        }
> 
> I think a "is_...()" function should still be side-effect less, i.e. post
> error here. I don't think we need any more error message than "size limit
> invalid".

That's very useful for debugging clients, the compositor tells exactly why the size is invalid - Making it a generic invalid size limit will makes debugging harder.
 
> ::: src/wayland/meta-window-wayland.c
> @@ +334,1 @@
>  }
> 
> nit: Actually think it'd be nicer to have
> 
> static void
> scale_size (int  *width
>             int  *height,
>             float scale)
> {
>   if (width < INT_MAX)
>     *width = (int)(width * scale);
>   if (height < INT_MAX)
>     *height = (int)(height * scale);
> }
> 
> static void
> scale_rect_size (MetaRectangle *rect,
>                  float          scale)
> {
>   scale_size (&rect->width, &rect->height, scale);
> }
> 
> so that you don't have to create all those dummy rects.

Good idea!
 
> @@ +704,3 @@
> +    rect.width = MIN (G_MAXINT,
> +                      rect.width + (window->custom_frame_extents.left +
> +                                    window->custom_frame_extents.right));
> 
> Hmm. MIN (INT_MAX, ...) feels like ... will always be less (than the highest
> number possible). Or did you mean to make the right hand expression a
> (uint64_t) so that it can actually be greater?

Yeah, it's probably overkill and useless, although I don't know exactly how the compiler deals with such intermediate cases internally.

(In reply to Olivier Fourdan from comment #16)
> (In reply to Jonas Ådahl from comment #15)
> > Review of attachment 334665 [details] [review] [review] [review]:
> > 
> > ::: src/wayland/meta-wayland-xdg-shell.c
> > @@ +630,3 @@
> > +
> > +      return TRUE;
> > +    }
> > 
> > Couldn't this function be simplified to
> > 
> > int new_min_width, new_min_height;
> > int new_max_width, new_max_height;
> > 
> > if (pending->has_new_min_size)
> >   {
> >     new_min_width = pending->new_min_width;
> >     new_min_height = pending->new_min_height;
> >   }
> > else
> >   {
> >     meta_window_wayland_get_min_size (window, &new_min_width,
> > &new_min_height);
> >   }
> > 
> > 
> > if (pending->has_new_max_size)
> >   {
> >     new_max_width = pending->new_max_width;
> >     new_max_height = pending->new_max_height;
> >   }
> > else
> >   {
> >     meta_window_wayland_get_min_size (window, &new_max_width,
> > &new_max_height);
> >   }
> > 
> > return new_min_width <= new_max_width && new_min_height <= new_max_height;
> 
> Tempting, but we still need to check for 0, as a value size of 0 in a new
> request means it's unlimited, so a client may request a new max size with
> one dimension set and the other not.

The main simplification was the "what-if" new_min/max_width/height.

> 
> That doesn't matter for min size, but it does for max size. Another
> possibility is to set a max size of 0 in get_max_size for unlimited values
> so that we remain consistent in sizes semantics and the test would work in
> all cases.
>  
> > @@ +687,3 @@
> > +
> > +          meta_window_recalc_features (window);
> > +        }
> > 
> > I think a "is_...()" function should still be side-effect less, i.e. post
> > error here. I don't think we need any more error message than "size limit
> > invalid".
> 
> That's very useful for debugging clients, the compositor tells exactly why
> the size is invalid - Making it a generic invalid size limit will makes
> debugging harder.

Personally I don't think complicating things is worth the better error messages for this particular case; its just size comparation. If you insist, then should reflect the side-effect-ness in the naming, (my is_...() suggestion was only if it was just checking, not disconnecting clients).

>  
> > ::: src/wayland/meta-window-wayland.c
> > @@ +334,1 @@
> >  }
> > 
> > nit: Actually think it'd be nicer to have
> > 
> > static void
> > scale_size (int  *width
> >             int  *height,
> >             float scale)
> > {
> >   if (width < INT_MAX)
> >     *width = (int)(width * scale);
> >   if (height < INT_MAX)
> >     *height = (int)(height * scale);
> > }
> > 
> > static void
> > scale_rect_size (MetaRectangle *rect,
> >                  float          scale)
> > {
> >   scale_size (&rect->width, &rect->height, scale);
> > }
> > 
> > so that you don't have to create all those dummy rects.
> 
> Good idea!
>  
> > @@ +704,3 @@
> > +    rect.width = MIN (G_MAXINT,
> > +                      rect.width + (window->custom_frame_extents.left +
> > +                                    window->custom_frame_extents.right));
> > 
> > Hmm. MIN (INT_MAX, ...) feels like ... will always be less (than the highest
> > number possible). Or did you mean to make the right hand expression a
> > (uint64_t) so that it can actually be greater?
> 
> Yeah, it's probably overkill and useless, although I don't know exactly how
> the compiler deals with such intermediate cases internally.

We could do it properly by using INT32_MAX and casting something to (int64_t) in the right-hand-side expression. A broken client can still pass a less than INT_MAX large value that will still cause issues, and for that it'd be good to clamp properly.

Created attachment 334801 [details] [review]
[PATCH] wayland: add min/max size from xdg-shell v6

Updated patch

Review of attachment 334801 [details] [review]:

Looks good to me.

Comment on attachment 334801 [details] [review]
[PATCH] wayland: add min/max size from xdg-shell v6

attachment 334801 [details] [review] pushed to git master as commit 4f58a46 - wayland: add min/max size from xdg-shell v6