Created attachment 313286 [details] [review]
gdkattachmentparameters: add GdkAttachmentParameters

Created attachment 313288 [details] [review]
gdkwindow: add gdk_window_set_attachment_parameters ()

Created attachment 313289 [details] [review]
gdkwindow-x11: implement set_attachment_parameters ()

Created attachment 313291 [details] [review]
gdkwindow-broadway: implement set_attachment_parameters ()

Created attachment 313292 [details] [review]
gdkwindow-quartz: implement set_attachment_parameters ()

Created attachment 313294 [details] [review]
gdkwindow-win32: implement set_attachment_parameters ()

Created attachment 313295 [details] [review]
gdkwindow-wayland: implement set_attachment_parameters ()

Created attachment 313296 [details] [review]
gdkwindow-mir: implement set_attachment_parameters ()

Created attachment 313297 [details] [review]
gtkmenu: add gtk_menu_popup_with_parameters ()

Created attachment 313298 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_parameters ()

Created attachment 313299 [details] [review]
gtkmenubutton: use gtk_menu_popup_with_parameters ()

Created attachment 313300 [details] [review]
gtktoolbar: use gtk_menu_popup_with_parameters ()

Created attachment 313301 [details] [review]
gtklabel: use gtk_menu_popup_with_parameters ()

Created attachment 313302 [details] [review]
gtkentry: use gtk_menu_popup_with_parameters ()

Created attachment 313303 [details] [review]
gtktextview: use gtk_menu_popup_with_parameters ()

Created attachment 313304 [details] [review]
gtklinkbutton: use gtk_menu_popup_with_parameters ()

Created attachment 313305 [details] [review]
gtknotebook: use gtk_menu_popup_with_parameters ()

Review of attachment 313286 [details] [review]:

::: gdk/gdkattachmentparameters.c
@@ +39,3 @@
+/**
+ * gdk_attachment_parameters_copy:
+ * @src: (transfer none) (nullable): the parameters to copy

These annotations are non-sensical; this is the instance argument.

@@ +71,3 @@
+/**
+ * gdk_attachment_parameters_free:
+ * @data: (transfer full) (nullable): the parameters to free

We don't annotate free functions like this. Also, our free functions are usually not NULL-safe.

@@ +98,3 @@
+/**
+ * gdk_attachment_parameters_set_attachment_origin:
+ * @parameters: (transfer none) (not nullable): a #GdkAttachmentParameters

Again, this is the instance argument. These annotations are pointless. Please, drop them from all other functions.

@@ +99,3 @@
+ * gdk_attachment_parameters_set_attachment_origin:
+ * @parameters: (transfer none) (not nullable): a #GdkAttachmentParameters
+ * @origin: (transfer none) (nullable): the attachment rectangle's origin

The `transfer none` annotation is the default for pointer in arguments. It's just visual noise.

Created attachment 313307 [details] [review]
gtkwindow: use gtk_menu_popup_with_parameters ()

Created attachment 313308 [details] [review]
gtkrecentchooserdefault: use gtk_menu_popup_with_parameters ()

Created attachment 313309 [details] [review]
gtkplacesview: use gtk_menu_popup_with_parameters ()

Created attachment 313310 [details] [review]
gtkappchooserwidget: use gtk_menu_popup_with_parameters ()

Created attachment 313311 [details] [review]
gtkmountoperation: use gtk_menu_popup_with_parameters ()

Created attachment 313312 [details] [review]
gtkcolorsel: use gtk_menu_popup_with_parameters ()

Created attachment 313313 [details] [review]
gtkcombobox: use gtk_menu_popup_with_parameters ()

Review of attachment 313286 [details] [review]:

::: gdk/gdkattachmentparameters.h
@@ +50,3 @@
+ * @GDK_ATTACHMENT_ATTACH_RIGHT_OF_CENTER: align left edge of window with center of attachment rectangle
+ * @GDK_ATTACHMENT_ATTACH_FORWARD_OF_CENTER: align backward edge of window with center of attachment rectangle
+ * @GDK_ATTACHMENT_ATTACH_BACKWARD_OF_CENTER: align forward edge of window with center of attachment rectangle

Given the combinatorial explosion of values, maybe we should use a flags type instead of a flat enumeration.

@@ +53,3 @@
+ *
+ * Constraints on the position of the window relative to its attachment
+ * rectangle.

Missing a Since annotation.

@@ +98,3 @@
+ * @bottom: space below
+ *
+ * The space around the perimeter of an attachment rectangle.

Missing a Since: annotation.

@@ +105,3 @@
+  gint left;
+  gint right;
+  gint bottom;

This screams to be converted to a cairo_rectangle_int_t.

@@ +135,3 @@
+ * of a window after gdk_window_set_attachment_parameters() is called. Since
+ * the position might be determined asynchronously, don't assume it will be
+ * called directly from gdk_window_set_attachment_parameters().

Missing a Since: annotation.

@@ +193,3 @@
+void                      gdk_attachment_parameters_add_primary_options      (GdkAttachmentParameters       *parameters,
+                                                                              GdkAttachmentOption            first_option,
+                                                                              ...) G_GNUC_NULL_TERMINATED;

You'll need to add a vectorised version of this function, for language bindings.

@@ +197,3 @@
+GDK_AVAILABLE_IN_3_20
+void                      gdk_attachment_parameters_add_secondary_options    (GdkAttachmentParameters       *parameters,
+                                                                              GdkAttachmentOption            first_option,

Same as above.

::: gdk/gdkattachmentparametersprivate.h
@@ +45,3 @@
+  gboolean is_right_to_left;
+
+  GList *primary_options;

Please, use an array instead of a list.

Review of attachment 313297 [details] [review]:

::: gtk/gtkmenu.h
@@ +176,3 @@
+                                           guint                    button,
+                                           guint32                  activate_time,
+                                           GdkAttachmentParameters *parameters);

Why does this api take an attachment_widget when we already have gtk_menu_attach_to_widget ?
What is the difference between the two ?

Created attachment 313426 [details] [review]
gdkattachmentparameters: add GdkAttachmentParameters

Created attachment 313427 [details] [review]
gdkwindow: add gdk_window_set_attachment_parameters ()

Created attachment 313428 [details] [review]
gdkwindow-mir: implement set_attachment_parameters ()

Created attachment 313429 [details] [review]
gtkmenu: add gtk_menu_popup_with_parameters ()

Created attachment 313430 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_parameters ()

Created attachment 313431 [details] [review]
gtkmenubutton: use gtk_menu_popup_with_parameters ()

Created attachment 313432 [details] [review]
gtktoolbar: use gtk_menu_popup_with_parameters ()

Created attachment 313433 [details] [review]
gtklabel: use gtk_menu_popup_with_parameters ()

Created attachment 313434 [details] [review]
gtkentry: use gtk_menu_popup_with_parameters ()

Created attachment 313435 [details] [review]
gtktextview: use gtk_menu_popup_with_parameters ()

Created attachment 313436 [details] [review]
gtklinkbutton: use gtk_menu_popup_with_parameters ()

Created attachment 313437 [details] [review]
gtknotebook: use gtk_menu_popup_with_parameters ()

Created attachment 313438 [details] [review]
gtkrecentchooserdefault: use gtk_menu_popup_with_parameters ()

Created attachment 313439 [details] [review]
gtkcolorsel: use gtk_menu_popup_with_parameters ()

Created attachment 313440 [details] [review]
gtkcombobox: use gtk_menu_popup_with_parameters ()

Matthias, that attachment_widget parameter is just a convenience to save the extra function call to gtk_menu_attach_widget () before gtk_menu_popup_with_parameters (). From what I can tell, gtk_menu_attach_widget () is only related to the widget hierarchy and doesn't have a lot to do with the actual position of the menu, whereas these patches uses it for positioning, but yes, I have no problem with reusing it.

Emmanuele, I'm not sure we should use cairo_rectangle_int_t for the border since the names of the fields have different meanings. But maybe instead we could rename GtkBorder to GdkBorder and typedef the first to the second?

Created attachment 313970 [details] [review]
gdkwindow: move GdkWindowTypeHint to gdktypes.h

Created attachment 313971 [details] [review]
gdkborder: add GdkBorder to gdktypes.h

Created attachment 313972 [details] [review]
gdkattachparams: add GdkAttachParams

Created attachment 313973 [details] [review]
gdkwindow: add gdk_window_set_attach_params ()

Created attachment 313974 [details] [review]
gdkwindow-x11: implement set_attach_params ()

Created attachment 313975 [details] [review]
gdkwindow-broadway: implement set_attach_params ()

Created attachment 313976 [details] [review]
gdkwindow-quartz: implement set_attach_params ()

Created attachment 313977 [details] [review]
gdkwindow-win32: implement set_attach_params ()

Created attachment 313978 [details] [review]
gdkwindow-wayland: implement set_attach_params ()

Created attachment 313979 [details] [review]
gdkwindow-mir: implement set_attach_params ()

Created attachment 313980 [details] [review]
gtkmenu: add gtk_menu_popup_with_params ()

Created attachment 313981 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_params ()

Created attachment 313982 [details] [review]
gtkmenubutton: use gtk_menu_popup_with_params ()

Created attachment 313983 [details] [review]
gtktoolbar: use gtk_menu_popup_with_params ()

Created attachment 313984 [details] [review]
gtklabel: use gtk_menu_popup_with_params ()

Created attachment 313985 [details] [review]
gtkentry: use gtk_menu_popup_with_params ()

Created attachment 313986 [details] [review]
gtktextview: use gtk_menu_popup_with_params ()

Created attachment 313987 [details] [review]
gtklinkbutton: use gtk_menu_popup_with_params ()

Created attachment 313988 [details] [review]
gtknotebook: use gtk_menu_popup_with_params ()

Created attachment 313989 [details] [review]
gtkwindow: use gtk_menu_popup_with_params ()

Created attachment 313990 [details] [review]
gtkrecentchooserdefault: use gtk_menu_popup_with_params ()

Created attachment 313991 [details] [review]
gtkplacesview: use gtk_menu_popup_with_params ()

Created attachment 313992 [details] [review]
gtkappchooserwidget: use gtk_menu_popup_with_params ()

Created attachment 313993 [details] [review]
gtkmountoperation: use gtk_menu_popup_with_params ()

Created attachment 313994 [details] [review]
gtkcolorsel: use gtk_menu_popup_with_params ()

Created attachment 313995 [details] [review]
gtkcombobox: use gtk_menu_popup_with_params ()

So I want to keep the attach_widget parameter in gtk_menu_popup_with_params () because there are a few cases where that attach_widget is different from the one passed into gtk_menu_attach_to_widget ():

GtkMenuButton aligns the menu with the align-widget property if it is set. This isn't the same as the attach_widget for the menu.

GtkNotebook sometimes aligns the menu with the label of the focused tab instead of the notebook which is the attach_widget.

GtkComboBox sometimes aligns the menu with the child of the combo box instead of the combo box itself.

GtkEntry aligns the menu with either the cursor or the pointer instead of the entry widget.

...and a bunch of other cases where the widget pops up the menu at the cursor instead of at the attach_widget. So we would have to set the attachment rectangle manually in those cases, which seems pretty inconvenient compared to just passing the widget we want the menu to stick to.

Review of attachment 313970 [details] [review]:

sure

Review of attachment 313971 [details] [review]:

ok, I guess.

Review of attachment 313972 [details] [review]:

::: gdk/gdkattachparams.c
@@ +15,3 @@
+ * A full description of how a window should be positioned relative to an
+ * attachment rectangle.
+ *

I think we need a bit more meat here. This should describe the general problem that is being solved here, what constraints can be expressed, and so on.

@@ +278,3 @@
+ *
+ * Sets the window text direction.
+ *

See my earlier comment. I'm not convinced that we really want to pass this down. Is there a rationale for why we need it on the compositor side ?

@@ +628,3 @@
+ * Finds the best position for a window of size @width and @height on a screen
+ * with @bounds using the given @params.
+ *

What you call window and screen here are just two rectangles in some coordinate space. Maybe we should advertise it like that ? I could see some possibility for reusing that in situations that are not window vs screen, but e.g. popover vs toplevel, etc

::: gdk/gdkattachparams.h
@@ +33,3 @@
+ */
+typedef enum _GdkAttachRule
+{

As a general comment, I prefer shift notation for flags like this: GDK_ATTACH_AXIS_Y = 1 << 2.

@@ +41,3 @@
+  GDK_ATTACH_RECT_MID    = 0x08,
+  GDK_ATTACH_RECT_MAX    = 0x0C,
+  GDK_ATTACH_RECT_MASK   = 0x0C,

How is this supposed to work, shouldn't the mask cover all the RECT bits ?

@@ +45,3 @@
+  GDK_ATTACH_WINDOW_MID  = 0x20,
+  GDK_ATTACH_WINDOW_MAX  = 0x30,
+  GDK_ATTACH_WINDOW_MASK = 0x30,

Same question here

@@ +46,3 @@
+  GDK_ATTACH_WINDOW_MAX  = 0x30,
+  GDK_ATTACH_WINDOW_MASK = 0x30,
+  GDK_ATTACH_FLIP_IF_RTL = 0x40

I haven't read the entire patchset yet, but I think it would be nicer to keep locale-dependencies at the GTK+ level, and translate to 'absolute' directions before going to GDK.

Review of attachment 313973 [details] [review]:

::: gdk/gdkwindow.c
@@ +11529,3 @@
+ * is decided by the backend.
+ *
+ * Since: 3.20

Hmm, this is a bit unclear to me - is this a one-short positioning, and the user can subsequently move the window elsewhere (assuming that is possible) ? Or is this meant to apply constraints on an ongoing bases ? What if I later resize the window ?

Review of attachment 313974 [details] [review]:

Seems odd to use a public function implemented in the frontend as a backend vfunc. And in fact, the public function should probably just be a wrapper around the vfunc, with the current body as the default implementation thereof.

Review of attachment 313974 [details] [review]:

Seems odd to use a public function implemented in the frontend as a backend vfunc. And in fact, the public function should probably just be a wrapper around the vfunc, with the current body as the default implementation thereof.

Review of attachment 313975 [details] [review]:

same comment as for the x11 patch

Review of attachment 313980 [details] [review]:

::: gtk/gtkmenu.c
@@ +1941,3 @@
+                            guint            button,
+                            guint32          activate_time,
+                            GdkAttachParams *params)

I think this new api may need to be combined with Carlos' GdkSeat work which also introduces a new menu popup api.

Review of attachment 313980 [details] [review]:

::: gtk/gtkmenu.c
@@ +1906,3 @@
+ * @button: the mouse button which was pressed to initiate the event
+ * @activate_time: the time at which the activation event occurred
+ * @params: (transfer full) (nullable): a description of how to position @menu

What does transfer full entail here ? If I keep the menu around and pop it up a second time, can I pass NULL and have the same attachment params take effect ?

Review of attachment 313978 [details] [review]:

::: gdk/wayland/gdkwindow-wayland.c
@@ +2490,1 @@
 

Would be nice to have a working Wayland implementation, but I guess that requires us to settle on a protocol first, so I won't block on it.

Review of attachment 313972 [details] [review]:

::: gdk/gdkattachparams.h
@@ +33,3 @@
+ */
+typedef enum _GdkAttachRule
+{

Sure

@@ +41,3 @@
+  GDK_ATTACH_RECT_MID    = 0x08,
+  GDK_ATTACH_RECT_MAX    = 0x0C,
+  GDK_ATTACH_RECT_MASK   = 0x0C,

GDK_ATTACH_RECT_MIN, GDK_ATTACH_RECT_MID, and GDK_ATTACH_RECT_MAX are all meant to be mutually exclusive, a GdkAttachRule will only contain one. So applying the 2-bit GDK_ATTACH_RECT_MASK to a rule will yield one of those possibilities (or the degenerate case of 0).

@@ +46,3 @@
+  GDK_ATTACH_WINDOW_MAX  = 0x30,
+  GDK_ATTACH_WINDOW_MASK = 0x30,
+  GDK_ATTACH_FLIP_IF_RTL = 0x40

True, it's not essential, but IMO it makes the API easier to consume since it removes the need for an extra check, and flipping MINs and MAXs, before setting the attach params on a window. The RTL information is still obtained through GTK and passed into the params, but it's also set through the convenience function gtk_menu_update_parameters (). If you still feel strongly about it, we can remove it, it'll just force the API consumer to write that extra bit of code every time.

Review of attachment 313980 [details] [review]:

::: gtk/gtkmenu.c
@@ +1906,3 @@
+ * @button: the mouse button which was pressed to initiate the event
+ * @activate_time: the time at which the activation event occurred
+ * @params: (transfer full) (nullable): a description of how to position @menu

gdk_window_set_attach_params () is a transfer-none, one-shot, "set the position now with these requirements", but the menu needs to take ownership of the params around for when gtk_menu_position () inevitably gets called multiple times, so that it has the right data to pass to gdk_window_set_attach_params (). I guess it could always do a copy, but that's somewhat expensive for this particular struct.

Passing in NULL to gtk_menu_popup_with_params () removes the previously set params, which causes it to position the menu at the pointer.

Yeah, I think I prefer to keep text-direction handling entirely on the widget side. We've also kept it out of the css machinery. As you say, there's a convenience api already that can handle setting it up so the burden will not be too great.

GdkSeat has landed now, so you should rebase this and figure out how the gtk_menu apis need to look with seats instead of devices.

Created attachment 318351 [details] [review]
gdkattachparams: add GdkAttachParams

Created attachment 318352 [details] [review]
gdkwindow: add gdk_window_move_using_params ()

Created attachment 318353 [details] [review]
gdkwindowimpl: implement move_using_params ()

Created attachment 318354 [details] [review]
gdkwindow-mir: implement move_using_params ()

Created attachment 318355 [details] [review]
gtkmenu: add gtk_menu_popup_with_params ()

Created attachment 318356 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_params ()

Created attachment 318357 [details] [review]
gtkmenubutton: use gtk_menu_popup_with_params ()

Created attachment 318358 [details] [review]
gtktoolbar: use gtk_menu_popup_with_params ()

Created attachment 318359 [details] [review]
gtklabel: use gtk_menu_popup_with_params ()

Created attachment 318360 [details] [review]
gtkentry: use gtk_menu_popup_with_params ()

Created attachment 318361 [details] [review]
gtktextview: use gtk_menu_popup_with_params ()

Created attachment 318362 [details] [review]
gtklinkbutton: use gtk_menu_popup_with_params ()

Created attachment 318363 [details] [review]
gtknotebook: use gtk_menu_popup_with_params ()

Created attachment 318364 [details] [review]
gtkwindow: use gtk_menu_popup_with_params ()

Created attachment 318365 [details] [review]
gtkrecentchooserdefault: use gtk_menu_popup_with_params ()

Created attachment 318366 [details] [review]
gtkplacesview: use gtk_menu_popup_with_params ()

Created attachment 318367 [details] [review]
gtkappchooserwidget: use gtk_menu_popup_with_params ()

Created attachment 318368 [details] [review]
gtkmountoperation: use gtk_menu_popup_with_params ()

Created attachment 318369 [details] [review]
gtkcolorsel: use gtk_menu_popup_with_params ()

Created attachment 318370 [details] [review]
gtkcombobox: use gtk_menu_popup_with_params ()

> ::: gtk/gtkmenu.c
> @@ +1906,3 @@
> + * @button: the mouse button which was pressed to initiate the event
> + * @activate_time: the time at which the activation event occurred
> + * @params: (transfer full) (nullable): a description of how to position
> @menu
> 
> gdk_window_set_attach_params () is a transfer-none, one-shot, "set the
> position now with these requirements", but the menu needs to take ownership
> of the params around for when gtk_menu_position () inevitably gets called
> multiple times, so that it has the right data to pass to
> gdk_window_set_attach_params (). I guess it could always do a copy, but
> that's somewhat expensive for this particular struct.
> 
> Passing in NULL to gtk_menu_popup_with_params () removes the previously set
> params, which causes it to position the menu at the pointer.

I renamed these functions to:

set_attach_params -> move_using_params (in GdkWindowImplClass)
gdk_window_set_attach_params -> gdk_attach_params_move_window

to better describe to the user what the functions are actually doing. Hopefully this should also make the reasoning for the transfer annotation more intuitive.



> What you call window and screen here are just two rectangles in some
> coordinate space. Maybe we should advertise it like that ? I could see some
> possibility for reusing that in situations that are not window vs screen, but 
> e.g. popover vs toplevel, etc

I tried to fix the description to use more generic terms like "anchoring rectangle" or "allocation", but this made the documentation a lot less reader-friendly imo...

Created attachment 318401 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_params ()

Review of attachment 318355 [details] [review]:

::: gtk/gtkmenu.c
@@ +2139,3 @@
+ * be used instead.
+ *
+ * Since: 3.0

Something went wrong with the Since tag here

@@ +2152,3 @@
+                           guint32              activate_time)
+{
+  g_return_if_fail (GTK_IS_MENU (menu));

I still want this to be switched over to GdkSeat.

@@ +4621,3 @@
 
+  /* Realize so we have the proper width and height to figure out
+   * the right place to popup the menu. */

I prefer the closing */ on the next line and lined up with the other stars

Created attachment 318541 [details] [review]
gtkmenu: add gtk_menu_popup_with_params ()

Created attachment 318542 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_params ()

Created attachment 318543 [details] [review]
gtkmenubutton: use gtk_menu_popup_with_params ()

Created attachment 318544 [details] [review]
gtkentry: use gtk_menu_popup_with_params ()

Created attachment 318545 [details] [review]
gtktextview: use gtk_menu_popup_with_params ()

Created attachment 318546 [details] [review]
gtkcombobox: use gtk_menu_popup_with_params ()

Review of attachment 318541 [details] [review]:

::: gtk/gtkmenu.c
@@ +1937,3 @@
+
+  gdk_attach_params_set_attach_origin (params, x, y);
+  gdk_attach_params_set_attach_rect (params, &allocation);

This part here is not clear to me: You set the attach origin in root coordinates. How is this going to work for backends like wayland which don't give you that information ? I have would have expected the params to store the attach rectangle relative to a toplevel, and pass the toplevel along to the backend.

Trying this out under X, I notice that menus don't properly line up to the blue underline in the menubar - there's some offset there (the shadow ?) that shouldn't be there...

Created attachment 318786 [details] [review]
gdkattachparams: add GdkAttachParams

Created attachment 318788 [details] [review]
gdkwindow: add gdk_window_move_using_params ()

Created attachment 318789 [details] [review]
gdkwindowimpl: implement move_using_params ()

Created attachment 318790 [details] [review]
gdkwindow-mir: implement move_using_params ()

Created attachment 318791 [details] [review]
gtkmenu: add gtk_menu_popup_with_params ()

Created attachment 318792 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_params ()

Created attachment 318793 [details] [review]
gtkmenubutton: use gtk_menu_popup_with_params ()

Created attachment 318794 [details] [review]
gtktoolbar: use gtk_menu_popup_with_params ()

Created attachment 318795 [details] [review]
gtklabel: use gtk_menu_popup_with_params ()

Created attachment 318796 [details] [review]
gtkentry: use gtk_menu_popup_with_params ()

Created attachment 318797 [details] [review]
gtktextview: use gtk_menu_popup_with_params ()

Created attachment 318798 [details] [review]
gtklinkbutton: use gtk_menu_popup_with_params ()

Created attachment 318799 [details] [review]
gtknotebook: use gtk_menu_popup_with_params ()

Created attachment 318800 [details] [review]
gtkwindow: use gtk_menu_popup_with_params ()

Created attachment 318801 [details] [review]
gtkrecentchooserdefault: use gtk_menu_popup_with_params ()

Created attachment 318802 [details] [review]
gtkplacesview: use gtk_menu_popup_with_params ()

Created attachment 318803 [details] [review]
gtkappchooserwidget: use gtk_menu_popup_with_params ()

Created attachment 318804 [details] [review]
gtkmountoperation: use gtk_menu_popup_with_params ()

Created attachment 318805 [details] [review]
gtkcolorsel: use gtk_menu_popup_with_params ()

Created attachment 318806 [details] [review]
gtkcombobox: use gtk_menu_popup_with_params ()

Review of attachment 318786 [details] [review]:

::: gdk/gdkattachparams.c
@@ +113,3 @@
+  g_array_append_vals (copy->secondary_rules,
+                       params->secondary_rules->data,
+                       params->secondary_rules->len);

You need to ref attach_parent here.

@@ +174,3 @@
+          if (parent)
+            params->attach_parent = g_object_ref (parent);
+        }

g_set_object (&params->attach_parent, parent);

Handles the refcounting for you more elegantly.

Apart from this, the patches look good to me. I'd like to hold off until next week with landing them, though - Jonas says that he is going to look at prototyping a Wayland implementation for this, which would be great to have as validation of the approach.

Created attachment 318880 [details] [review]
gdkattachparams: add GdkAttachParams

Created attachment 318882 [details] [review]
gtkmenu: add gtk_menu_popup_with_params ()

Hey, here are some initial comments regarding the overall approach of the API. I haven't implemented anything yet, but have a prototype of the Wayland protocol, and I see some issues which I also see with how it's currently used in the Mir backend. There are 3 major notes, and a couple of minor nits.

Note 1
======

API seems to allow moving after a window is showed. This behaviour is not intended to work with Wayland, as the positioning is done on mapping. I think it'd be good to design new API with this in mind, i.e. the windows should be created with a intended position, and the resulting position will not be availalbe until the compositor has replied, given the positioning information and the expected window geometry (i.e. size). It seems to be that for example the mir backend will destroy the surface if it is moved while already mapped, and the Wayland backend would have to do something similar. Wouldn't it be better to limit the API so that it is shown this way?

Note 2
======

Why can't we specify a attachment params without multiple levels of priorities? It doesn't seem to used by the mir backend at all (it simply finds the first rule with an axis), and it could probably not be used in any meaninful way in the Wayland backend as well. Why can't we just have a single attachment rule, that specifies just what happens when the child window is constrained?

For example the menu example in gdkattachparams.svg. The expected behaviour is for a menu to be mapped as the purple rectangle, where the blue rectangle is the attach rect. If the right edge ends up outside of the monitor, it should slide alongside the x axis, but if the bottom edge ends up outside of the monitor, it should be placed on the other side of the attach rect while still aligned the same way on the x axis.

In gtkmenu.c you specify this by adding 8 different rules (4 primary and 4 secondary).

The first rule, which is the one who will, if I read the code correctly, be the one used in the mir backend no matter what. It's defined as (GDK_ATTACH_AXIS_Y | GDK_ATTACH_RECT_MAX | GDK_ATTACH_WINDOW_MIN). I will read this as "Align the min-y edge of the attach rectangle with the min-y of the menu", which seems to be how the example is positioned. In the mir backend you also interpret the "GDK_ATTACH_AXIS_Y" as that the menu should slide alongside the x axis (mir_edge_attachment_horizontal) when constrained horizontally, which I think seems like the correct interpretation. What happens if the menu is constrained vertically, I cannot understand by reading the code.

Any how, given this, why can't we bake all that information about constraints into a single rule? Or how do you expect this to work if for the Mir backend? Do you expect to have an equivalent Mir API at a later point?

In Wayland, the currently prototyped protocol looks more similarly to how the Mir API looks, as in it only has a single rule, and assumptions about how constraining works is built into the protocol/API. It has the equivalent of the attach rect, the equivalent of the attachment edge, as well as an "anchor" and a "gravity". The anchor is meant to specify what side menu should align, while gravity is meant to specify what side of the attachment rect should align. Assuming such a protocol is used, it is not possible to make any use of multiple primary and secondary rules, as such rules cannot be translated to a single rule automatically.

So it makes me wonder, how many different menu positioning semantics do we have?

1) Regular menus. If we assume the semantics you already assume in the mir backend, and add the assumption that vertical constraint means putting the whole menu on the other side of the attach_rect, we can define menus with a single rule.

2) Combo boxes. Combo boxes have a position, and when constrained, they seem to be constrained in any direction without any alignment. This seems to be the easiest case.

3) Context menus. They never align with any edge, and always flips direction.

For 1) we can already do this if we make the assumptions describe above with the current first rule of the existing gtk_menu_popup_with_params.

For 2) I think this could be a simple params with no axis, and only a GDK_ATTACH_RECT_MAX with a GDK_ATTACH_WINDOW_MIN to get an initial position.

For 3) If we set both GDK_ATTACH_AXIS_Y and GDK_ATTACH_AXIS_X we can interpret this as flip in both direction, just as just GDK_ATTACH_AXIS_Y would mean flip in the y direction.

What other semantics do we need to support?

Note 3
======

Another issue I see is how to deal with position feedback. Some widgets require feedback about how it was positioned. An example of this is the Pickers (to test: open gtk3-demo, choose "Pickers", place window by the bottom edge of the monitor, click Evolution: the Evolution option is selected, but the element is not at the expected position). How is this supposed to work?


Other minor nits:

A lot of the API is exposed via gdkattachparams.h but many of them are used only in gdkattachparams.c. Why not make them static to that function.

All of the choose/move/... code in gdkattachparams.c seems to implement the semantics where all the global state (global window position etc) is available. How are these exposed right now? Are they internal to GDK? At least I think any API that depends on global positioning should not be exposed to the outside.

Hi Jonas, thanks for taking a look here,



> Note 1
> ======
> 
> API seems to allow moving after a window is showed. This behaviour is not
> intended to work with Wayland, as the positioning is done on mapping. I
> think it'd be good to design new API with this in mind, i.e. the windows
> should be created with a intended position, and the resulting position will
> not be availalbe until the compositor has replied, given the positioning
> information and the expected window geometry (i.e. size). It seems to be
> that for example the mir backend will destroy the surface if it is moved
> while already mapped, and the Wayland backend would have to do something
> similar. Wouldn't it be better to limit the API so that it is shown this way?

We still have gtk_menu_reposition () exposed as public API which still should be supported for X11. But anyways...

For the Wayland implementation of GdkWindowImpl's move_using_params (), you should still be able to set the initial position-on-map there instead of doing an actual move, as the first call happens between realization and mapping at that point. As for subsequent calls though, I guess you would have to ignore them completely if you're not able to actually move the surface itself after map. But the same goes for calls to gtk_menu_reposition ().

As far as the Mir backend goes, they have support for modifying the position of the surface, I just haven't been able to try it yet. The Mir backend code is actually in a bad state right now ever since rebasing this on top of the GdkSeat API...



> Note 2
> ======
> 
> Why can't we specify a attachment params without multiple levels of
> priorities? It doesn't seem to used by the mir backend at all (it simply
> finds the first rule with an axis), and it could probably not be used in any
> meaninful way in the Wayland backend as well. Why can't we just have a
> single attachment rule, that specifies just what happens when the child
> window is constrained?

My thoughts on this were to make the position description as expressive as possible on the GDK side (even though we're throwing away some of the information), so that if newer more flexible API on the Wayland/Mir-side were to be introduced, those backends could in theory still take advantage of it without any changes on the client-side.

You're right that it sacrifices simplicity though, but IMO, the user could always still just define a single X-rule and Y-rule if they don't need the flexibility.



> For 1) we can already do this if we make the assumptions describe above with
> the current first rule of the existing gtk_menu_popup_with_params.
> 
> For 2) I think this could be a simple params with no axis, and only a
> GDK_ATTACH_RECT_MAX with a GDK_ATTACH_WINDOW_MIN to get an initial position.
> 
> For 3) If we set both GDK_ATTACH_AXIS_Y and GDK_ATTACH_AXIS_X we can
> interpret this as flip in both direction, just as just GDK_ATTACH_AXIS_Y
> would mean flip in the y direction.
> 
> What other semantics do we need to support?

Within GTK+ itself, none I can think of, I think you've covered all the usual cases, well, and GtkComboBox also has the more complicated case when it has the pop-over style. I haven't given much thought to how popovers would be ported over.

Outside GTK+, I can't guarantee that there would never be a future use case for an application which doesn't conform to the above since gtk_menu_popup () is public API.



> Note 3
> ======
> 
> Another issue I see is how to deal with position feedback. Some widgets
> require feedback about how it was positioned. An example of this is the
> Pickers (to test: open gtk3-demo, choose "Pickers", place window by the
> bottom edge of the monitor, click Evolution: the Evolution option is
> selected, but the element is not at the expected position). How is this
> supposed to work?

GtkMenu and GtkComboBox already correctly support position feedback with GdkAttachCallback. The X11 backend uses this to overlay the selected element on top of the GtkComboBox. But we're going to need Wayland/Mir client API to pass the info up Wayland/Mir -> GDK -> GTK+.



> Other minor nits:
> 
> A lot of the API is exposed via gdkattachparams.h but many of them are used
> only in gdkattachparams.c. Why not make them static to that function.

Which functions are you concerned about?



> All of the choose/move/... code in gdkattachparams.c seems to implement the
> semantics where all the global state (global window position etc) is
> available. How are these exposed right now? Are they internal to GDK? At
> least I think any API that depends on global positioning should not be
> exposed to the outside.

Those functions are declared G_GNUC_INTERNAL in gdkattachparamsprivate.h.

(In reply to William Hua from comment #142)

> Outside GTK+, I can't guarantee that there would never be a future use case
> for an application which doesn't conform to the above since gtk_menu_popup
> () is public API.

I'm loathe to expose "flexibility" up front to cover future use cases that are currently not heavily defined nor well designed.

I'd start with the minimal API necessary to make GTK work, and then build on top of that. Application developers won't port their code immediately, and I'd rather have feedback from actual use cases when that happens.
 
> Those functions are declared G_GNUC_INTERNAL in gdkattachparamsprivate.h.

This is not necessary: if a symbol is not annotated with GDK_AVAILABLE_* then it's internal by default, and not exported.

(In reply to Emmanuele Bassi (:ebassi) from comment #143)
> (In reply to William Hua from comment #142)
> 
> > Outside GTK+, I can't guarantee that there would never be a future use case
> > for an application which doesn't conform to the above since gtk_menu_popup
> > () is public API.
> 
> I'm loathe to expose "flexibility" up front to cover future use cases that
> are currently not heavily defined nor well designed.
> 
> I'd start with the minimal API necessary to make GTK work, and then build on
> top of that. Application developers won't port their code immediately, and
> I'd rather have feedback from actual use cases when that happens.
>  

Yea, I'd much rather prefer an API which does exactly what we need and nothing more. A single description (like the bitmask combination you have together with the rects, margins etc, or separate as "attach edge", "anchor" fields that would work similarly to how the bitmask works. When there is need for more supported positioning logic, we can extend GdkAttachParams as needed, while at the same time figuring out how it would work for the Mir and Wayland backend.

If we create a new API in GDK that we cannot possibly ever implement without more or less making an equally flexible Wayland protocol/Mir API, we are kind of shooting ourself in the foot. Supporting such a complex API in Wayland is not at all desirable IMHO, and I will assume the same thing for the equivalent Mir API. The larger and more flexible the API, the more burden it is to maintain.

(In reply to William Hua from comment #142)
> Hi Jonas, thanks for taking a look here,
> 
> 
> 
> > Note 1
> > ======
> > 
> > API seems to allow moving after a window is showed. This behaviour is not
> > intended to work with Wayland, as the positioning is done on mapping. I
> > think it'd be good to design new API with this in mind, i.e. the windows
> > should be created with a intended position, and the resulting position will
> > not be availalbe until the compositor has replied, given the positioning
> > information and the expected window geometry (i.e. size). It seems to be
> > that for example the mir backend will destroy the surface if it is moved
> > while already mapped, and the Wayland backend would have to do something
> > similar. Wouldn't it be better to limit the API so that it is shown this way?
> 
> We still have gtk_menu_reposition () exposed as public API which still
> should be supported for X11. But anyways...
> 
> For the Wayland implementation of GdkWindowImpl's move_using_params (), you
> should still be able to set the initial position-on-map there instead of
> doing an actual move, as the first call happens between realization and
> mapping at that point. As for subsequent calls though, I guess you would
> have to ignore them completely if you're not able to actually move the
> surface itself after map. But the same goes for calls to gtk_menu_reposition
> ().
> 
> As far as the Mir backend goes, they have support for modifying the position
> of the surface, I just haven't been able to try it yet. The Mir backend code
> is actually in a bad state right now ever since rebasing this on top of the
> GdkSeat API...

The way to make it possible currently on Wayland is to unmap and then map again at the new position. But are there any reason for supporting moving?

I think a long term plan for the GDK and GTK API's should be to limit the applications to do things that make sense. Arbitrarily moving a menu or similar doesn't seem to be useful. I might be wrong though, so if I am, I'd be happy to see some situations where it would be needed. 

> 
> 

... snip ...

> 
> > Note 3
> > ======
> > 
> > Another issue I see is how to deal with position feedback. Some widgets
> > require feedback about how it was positioned. An example of this is the
> > Pickers (to test: open gtk3-demo, choose "Pickers", place window by the
> > bottom edge of the monitor, click Evolution: the Evolution option is
> > selected, but the element is not at the expected position). How is this
> > supposed to work?
> 
> GtkMenu and GtkComboBox already correctly support position feedback with
> GdkAttachCallback. The X11 backend uses this to overlay the selected element
> on top of the GtkComboBox. But we're going to need Wayland/Mir client API to
> pass the info up Wayland/Mir -> GDK -> GTK+.
> 

Ah, I see. Will take a look of how it works.

> 
> 
> > Other minor nits:
> > 
> > A lot of the API is exposed via gdkattachparams.h but many of them are used
> > only in gdkattachparams.c. Why not make them static to that function.
> 
> Which functions are you concerned about?
> 

Really any function that is not referenced from outside of that file should just be static.

Went through the functions.

gdk_attach_params_copy() is not used anywhere, but I guess it will be?

gdk_attach_params_set_window_margin() is not used anywhere.

gdk_attach_params_add_primary_rules_valist() is only referenced from the same file.

same for the _secondary_rules_valist() function.

> 
> 
> > All of the choose/move/... code in gdkattachparams.c seems to implement the
> > semantics where all the global state (global window position etc) is
> > available. How are these exposed right now? Are they internal to GDK? At
> > least I think any API that depends on global positioning should not be
> > exposed to the outside.
> 
> Those functions are declared G_GNUC_INTERNAL in gdkattachparamsprivate.h.

Ok, thats good!

Wrecking my brain a bit for a use case where repositioning might be useful, I've come up with entry completion: You start typing a few characters and get a lot of completions, the popup gets positioned for that size. As you type more, the list of completions will shrink, so will the popup, possibly making it fit in the best position when previously it wouldn't.

Of course, in this case, there's typically unmap and remap involved...

(In reply to Matthias Clasen from comment #145)
> Wrecking my brain a bit for a use case where repositioning might be useful,
> I've come up with entry completion: You start typing a few characters and
> get a lot of completions, the popup gets positioned for that size. As you
> type more, the list of completions will shrink, so will the popup, possibly
> making it fit in the best position when previously it wouldn't.
> 
> Of course, in this case, there's typically unmap and remap involved...

Hmm. This use case does seem reasonable though, so maybe my first point is invalid. The problem with unmap->remap is that there will be the occasional race where the compositor decides to draw between unmap and remap. Whether to solve that with a reposition call, or some other way to avoid the race, I guess would be up to the GDK backend.

Created attachment 319147 [details] [review]
gdkattachparams: add GdkAttachParams

Created attachment 319148 [details] [review]
gtkmenu: add gtk_menu_popup_with_parameters ()

Created attachment 319149 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_params ()

> > Those functions are declared G_GNUC_INTERNAL in gdkattachparamsprivate.h.
> 
> This is not necessary: if a symbol is not annotated with GDK_AVAILABLE_*
> then it's internal by default, and not exported.

Ok, removed.

> Really any function that is not referenced from outside of that file should
> just be static.
> 
> Went through the functions.
> 
> gdk_attach_params_copy() is not used anywhere, but I guess it will be?
> 
> gdk_attach_params_set_window_margin() is not used anywhere.
> 
> gdk_attach_params_add_primary_rules_valist() is only referenced from the
> same file.
> 
> same for the _secondary_rules_valist() function.

Removed copy and window_margin stuff. IIRC Matthias said we needed the valist functions for language bindings.

So if I understand correctly, we just want to remove the ability to specify multiple fallback attachment rules? The current GdkAttachRule enum is okay? Because I think we still do need to keep the idea that certain directions should remain prioritized, like for example popping up the menu left vs right for RTL locales.

(In reply to William Hua from comment #150)
> > > Those functions are declared G_GNUC_INTERNAL in gdkattachparamsprivate.h.
> > 
> > This is not necessary: if a symbol is not annotated with GDK_AVAILABLE_*
> > then it's internal by default, and not exported.
> 
> Ok, removed.
> 
> > Really any function that is not referenced from outside of that file should
> > just be static.
> > 
> > Went through the functions.
> > 
> > gdk_attach_params_copy() is not used anywhere, but I guess it will be?
> > 
> > gdk_attach_params_set_window_margin() is not used anywhere.
> > 
> > gdk_attach_params_add_primary_rules_valist() is only referenced from the
> > same file.
> > 
> > same for the _secondary_rules_valist() function.
> 
> Removed copy and window_margin stuff. IIRC Matthias said we needed the
> valist functions for language bindings.

I see. If we need copy and window margin, they are easy enough to add when needed.

> 
> So if I understand correctly, we just want to remove the ability to specify
> multiple fallback attachment rules? The current GdkAttachRule enum is okay?
> Because I think we still do need to keep the idea that certain directions
> should remain prioritized, like for example popping up the menu left vs
> right for RTL locales.

Yea, I think we should have a system where we only provide one rule which describes the positioning. Whether the current enum bitmask is enough, I'm not sure.

In any case, the current WIP approach on the Wayland side is this:

    <enum name="gravity">
      <entry name="none" value="0"/>
      <entry name="left" value="1"/>
      <entry name="right" value="2"/>
      <entry name="top" value="4"/>
      <entry name="bottom" value="8"/>
    </enum>
    
    <enum name="anchor">
      <entry name="none" value="0"/>
      <entry name="left" value="1"/>
      <entry name="right" value="2"/>
      <entry name="top" value="4"/>
      <entry name="bottom" value="8"/>
    </enum>
    
    <request name="set_anchor_point">
      <arg name="x" type="int"/>
      <arg name="y" type="int"/>
    </request>
    
    <request name="set_anchor_rect">
      <arg name="x" type="int"/>
      <arg name="y" type="int"/>
      <arg name="width" type="int"/>
      <arg name="height" type="int"/>
    </request>
    
    <request name="set_gravity">
      <arg name="gravity" type="uint"/>
    </request>
    
    <request name="set_anchor">
      <arg name="anchor" type="uint"/>
    </request>

(note this does not yet include position feedback).

"gravity" is a bitmask that specifies in what direction a menu should open (RTL horizontal menu: gravity=right|bottom, LTR horizontal menu: gravity=left|bottom)

"anchor" is the side of the anchor_rect or anchor_point (a pixel position) (RTL horizontal menu: anchor_rect=(position of [ File ] widget rect), anchor:bottom|left).

"anchor_rect": sets the rectangle where the menu window may not overlap with (if possible). Constraining the menu window slides alongside any side of the anchor_rect, but flips to the other side if it would cover the anchor_rect).

"anchor_point" a 1x1 anchor_rect that doesn't cause any flipping.

So some examples WxH+X+Y describes the rect of the "attach_rect")

LTR horizontal menu (menu window opening below horizontal menu bar):
    anchor_rect: WxH+X+Y, anchor: bottom|right, gravity: bottom|left

vertical menu (menu window opening left of vertial menu bar:
    anchor_rect: WxH+X+Y, anchor: top|right, gravity: bottom|right

combo box (menu opening covering triggering button, expanding below):
    anchor_point: (X, Y), anchor: top|left, gravity: bottom|right

combo box (menu opening covering triggeing button, expanding above):
    anchor_point: (X, Y+H), anchor: bottom|left, gravity: top|right

context menu (opens "around" mouse click, flipping in any direction):
    anchor_rect: 1x1+X+Y, anchor: bottom|right, gravity: bottom|left

With these I think cases I'm aware of in GTK+ should be declarable, but I haven't checked veryh rule enum be able to represent things like this?

I really quite like the approach, but just need a bit of clarification:



> "gravity" is a bitmask that specifies in what direction a menu should open
> (RTL horizontal menu: gravity=right|bottom, LTR horizontal menu:
> gravity=left|bottom)

An RTL menu pops up towards the left usually, so shouldn't the gravity be bottom|left instead?



> "anchor" is the side of the anchor_rect or anchor_point (a pixel position)
> (RTL horizontal menu: anchor_rect=(position of [ File ] widget rect),
> anchor:bottom|left).

Is the anchor ignored for the anchor_point case? Also, should the anchor be bottom|right for the RTL menu here?



> "anchor_rect": sets the rectangle where the menu window may not overlap with
> (if possible). Constraining the menu window slides alongside any side of the
> anchor_rect, but flips to the other side if it would cover the anchor_rect).
> 
> "anchor_point" a 1x1 anchor_rect that doesn't cause any flipping.
> 
> So some examples WxH+X+Y describes the rect of the "attach_rect")
> 
> LTR horizontal menu (menu window opening below horizontal menu bar):
>     anchor_rect: WxH+X+Y, anchor: bottom|right, gravity: bottom|left

Shouldn't this be anchor=bottom|left and gravity=bottom|right?



> vertical menu (menu window opening left of vertial menu bar:
>     anchor_rect: WxH+X+Y, anchor: top|right, gravity: bottom|right

If it's opening left, why is the gravity bottom|right?



> combo box (menu opening covering triggering button, expanding below):
>     anchor_point: (X, Y), anchor: top|left, gravity: bottom|right
>
> combo box (menu opening covering triggeing button, expanding above):
>     anchor_point: (X, Y+H), anchor: bottom|left, gravity: top|right
> 
> context menu (opens "around" mouse click, flipping in any direction):
>     anchor_rect: 1x1+X+Y, anchor: bottom|right, gravity: bottom|left
> 
> With these I think cases I'm aware of in GTK+ should be declarable, but I
> haven't checked veryh rule enum be able to represent things like this?

It seems like the anchor_point in general is derivable from the anchor_rect and anchor parameters together, except with the added meaning of shifting the menu instead of flipping it.

I can think of a very specific case where we want to flip on one axis and don't want to flip on the other.

Imagine there is a combo box (pop-down style) that is partially hanging off the left edge, near the bottom of the screen. If its menu is opened, then we want the menu to shift horizontally on screen instead of flipping. But we also want the menu to flip vertically as well.

Maybe instead we can have additional bit flags FLIP_X and FLIP_Y. Then anchor_point is redundant, and we can stick with anchor_rect, anchor and gravity with the extra flip flags? What do you think?

Created attachment 319279 [details]
GtkComboBox flip and shift example

This is the example I'm describing above. I think this could be easily solved with extra FLIP_X and FLIP_Y flags in the gravity parameter.

(In reply to William Hua from comment #152)
> I really quite like the approach, but just need a bit of clarification:
> 
> 
> 
> > "gravity" is a bitmask that specifies in what direction a menu should open
> > (RTL horizontal menu: gravity=right|bottom, LTR horizontal menu:
> > gravity=left|bottom)
> 
> An RTL menu pops up towards the left usually, so shouldn't the gravity be
> bottom|left instead?

Yes. I think I wrote inverted RTL and LTR in here. Sorry about that.

> 
> 
> 
> > "anchor" is the side of the anchor_rect or anchor_point (a pixel position)
> > (RTL horizontal menu: anchor_rect=(position of [ File ] widget rect),
> > anchor:bottom|left).
> 
> Is the anchor ignored for the anchor_point case? Also, should the anchor be
> bottom|right for the RTL menu here?
> 

The idea is that in the anchor_point case, the point is used only for positioning. So it is not ignored, but won't cause flipping.

> 
> 
> > "anchor_rect": sets the rectangle where the menu window may not overlap with
> > (if possible). Constraining the menu window slides alongside any side of the
> > anchor_rect, but flips to the other side if it would cover the anchor_rect).
> > 
> > "anchor_point" a 1x1 anchor_rect that doesn't cause any flipping.
> > 
> > So some examples WxH+X+Y describes the rect of the "attach_rect")
> > 
> > LTR horizontal menu (menu window opening below horizontal menu bar):
> >     anchor_rect: WxH+X+Y, anchor: bottom|right, gravity: bottom|left
> 
> Shouldn't this be anchor=bottom|left and gravity=bottom|right?
> 

Yes. I inverted RTL/LTR here too :P

> 
> 
> > vertical menu (menu window opening left of vertial menu bar:
> >     anchor_rect: WxH+X+Y, anchor: top|right, gravity: bottom|right
> 
> If it's opening left, why is the gravity bottom|right?

I seem to be messing up right and left a lot in my message. I meant this:

  [ File  ]
  [ Edit  ][ Edit thing A ]
  [ About ][ Edit thing B ]
           [ Edit thing C ]
           [ Edit thing D ]

> 
> 
> 
> > combo box (menu opening covering triggering button, expanding below):
> >     anchor_point: (X, Y), anchor: top|left, gravity: bottom|right
> >
> > combo box (menu opening covering triggeing button, expanding above):
> >     anchor_point: (X, Y+H), anchor: bottom|left, gravity: top|right
> > 
> > context menu (opens "around" mouse click, flipping in any direction):
> >     anchor_rect: 1x1+X+Y, anchor: bottom|right, gravity: bottom|left
> > 
> > With these I think cases I'm aware of in GTK+ should be declarable, but I
> > haven't checked veryh rule enum be able to represent things like this?
> 
> It seems like the anchor_point in general is derivable from the anchor_rect
> and anchor parameters together, except with the added meaning of shifting
> the menu instead of flipping it.
> 
> I can think of a very specific case where we want to flip on one axis and
> don't want to flip on the other.
> 
> Imagine there is a combo box (pop-down style) that is partially hanging off
> the left edge, near the bottom of the screen. If its menu is opened, then we
> want the menu to shift horizontally on screen instead of flipping. But we
> also want the menu to flip vertically as well.

This would already happen with the above semantics, since when constrained to the left it wouldn't overlap with the anchor_rect anyway. The idea with flipping is something like "position the menu window so that it doesn't overlap with the anchor_rect, at the position closest to the one declared using gravity".

So in your example, if the rule would be:
anchor_rect: WxH+X+Y, anchor: bottom|left, gravity: bottom|right

When hitting the left edge of the screen, it'd move horizontally, since it wouldn't overlap with the anchor_rect anyway.

When hitting the bottom edge fo the screen, it'd flip vertiaclly, since it would otherwise overlap with the anchor_rect.

> 
> Maybe instead we can have additional bit flags FLIP_X and FLIP_Y. Then
> anchor_point is redundant, and we can stick with anchor_rect, anchor and
> gravity with the extra flip flags? What do you think?

With that said, maybe FLIP_X, FLIP_Y is a better way.

Came to think of it, I think the context menu case won't work with these semantics after all, as it wouldn't overlap with the 1x1 anchor_rect if its only constrained in one direction. I wonder if we can fix this with something like FLIP_X/FLIP_Y where FLIP_X would be to flip the anchor left->right/right->left and the gravity left->right/right->left if constrained.

This'd mean that a context menu would be:
anchor_rect: 1x1+X+Y (clicked pixel), anchor: bottom|right, gravity: bottom|right, flip: x/y

A LTR menu would be:
anchor_rect: WxH+X+Y, anchor: bottom|left, gravity: bottom|right, flip: y

A RTL menu would be:
anchor_rect: WxH+X+Y, anchor: bottom|right, gravity: bottom|left, flip: y

A drop-down combo box covering the opening button:
anchor_rect: WxH+X+Y, anchor: top|left, gravity: bottom|right, flip: none

In other words, as you said skip the anchor_point, and have flips with these semantics. Was that what you had in mind or how would you define FLIP_X/FLIP_Y?

Yeah, thanks, this is starting to make a lot more sense now. So GdkAttachRule would look like this instead (since a window's gravity is really just the opposite of its anchor):

/**
 * GdkAttachRule:
 * @GDK_ATTACH_RECT_TOP: top edge of rectangle
 * @GDK_ATTACH_RECT_LEFT: left edge of rectangle
 * @GDK_ATTACH_RECT_RIGHT: right edge of rectangle
 * @GDK_ATTACH_RECT_BOTTOM: bottom edge of rectangle
 * @GDK_ATTACH_WINDOW_TOP: top edge of window
 * @GDK_ATTACH_WINDOW_LEFT: left edge of window
 * @GDK_ATTACH_WINDOW_RIGHT: right edge of window
 * @GDK_ATTACH_WINDOW_BOTTOM: bottom edge of window
 * @GDK_ATTACH_ALLOW_FLIP_X: flip horizontally if needed
 * @GDK_ATTACH_ALLOW_FLIP_Y: flip vertically if needed
 *
 * Constraints on the position of the window relative to its attachment
 * rectangle.
 *
 * ![](gdkattachrule.png)
 *
 * Since: 3.20
 */
typedef enum _GdkAttachRule
{
  GDK_ATTACH_RECT_TOP      = 1 << 0,
  GDK_ATTACH_RECT_LEFT     = 1 << 1,
  GDK_ATTACH_RECT_RIGHT    = 1 << 2,
  GDK_ATTACH_RECT_BOTTOM   = 1 << 3,
  GDK_ATTACH_WINDOW_TOP    = 1 << 4,
  GDK_ATTACH_WINDOW_LEFT   = 1 << 5,
  GDK_ATTACH_WINDOW_RIGHT  = 1 << 6,
  GDK_ATTACH_WINDOW_BOTTOM = 1 << 7,
  GDK_ATTACH_ALLOW_FLIP_X  = 1 << 8,
  GDK_ATTACH_ALLOW_FLIP_Y  = 1 << 9
} GdkAttachRule;



I'll refresh the patches tomorrow with these changes.

Maybe we should even separate the flip flags out of GdkAttachRule and into independent gbooleans in GdkAttachParams...

(In reply to William Hua from comment #155)
> Yeah, thanks, this is starting to make a lot more sense now. So
> GdkAttachRule would look like this instead (since a window's gravity is
> really just the opposite of its anchor):

Hmm? How do you mean opposite of its anchor? The anchor might be top|right when the gravity is bottom|left, but there are scenarios when the anchor is bottom|right and the gravity is also bottom|right.

> 
> /**
>  * GdkAttachRule:
>  * @GDK_ATTACH_RECT_TOP: top edge of rectangle
>  * @GDK_ATTACH_RECT_LEFT: left edge of rectangle
>  * @GDK_ATTACH_RECT_RIGHT: right edge of rectangle
>  * @GDK_ATTACH_RECT_BOTTOM: bottom edge of rectangle
>  * @GDK_ATTACH_WINDOW_TOP: top edge of window
>  * @GDK_ATTACH_WINDOW_LEFT: left edge of window
>  * @GDK_ATTACH_WINDOW_RIGHT: right edge of window
>  * @GDK_ATTACH_WINDOW_BOTTOM: bottom edge of window
>  * @GDK_ATTACH_ALLOW_FLIP_X: flip horizontally if needed
>  * @GDK_ATTACH_ALLOW_FLIP_Y: flip vertically if needed

Is GDK_ATTACH_RECT_* the equivalent of "anchor" above?

Is GDK_ATTACH_WINDOW_* the equivalent of "gravity" above, but inverted?

>  *
>  * Constraints on the position of the window relative to its attachment
>  * rectangle.
>  *
>  * ![](gdkattachrule.png)
>  *
>  * Since: 3.20
>  */
> typedef enum _GdkAttachRule
> {
>   GDK_ATTACH_RECT_TOP      = 1 << 0,
>   GDK_ATTACH_RECT_LEFT     = 1 << 1,
>   GDK_ATTACH_RECT_RIGHT    = 1 << 2,
>   GDK_ATTACH_RECT_BOTTOM   = 1 << 3,
>   GDK_ATTACH_WINDOW_TOP    = 1 << 4,
>   GDK_ATTACH_WINDOW_LEFT   = 1 << 5,
>   GDK_ATTACH_WINDOW_RIGHT  = 1 << 6,
>   GDK_ATTACH_WINDOW_BOTTOM = 1 << 7,
>   GDK_ATTACH_ALLOW_FLIP_X  = 1 << 8,
>   GDK_ATTACH_ALLOW_FLIP_Y  = 1 << 9
> } GdkAttachRule;
> 
> 
> 
> I'll refresh the patches tomorrow with these changes.

Sounds good. Please make it clear what the different enum values mean and what semantics they represent.

(In reply to William Hua from comment #156)
> Maybe we should even separate the flip flags out of GdkAttachRule and into
> independent gbooleans in GdkAttachParams...

I think its good to keep it an enum at least. You could consider splitting the catch-it-all enum into

GdkAttachRuleRect { GDK_ATTACH_RULE_RECT_(TOP|LEFT|RIGHT|BOTTOM) }
GdkAttachRuleWindow { GDK_ATTACH_RULE_WINDOW_(TOP|LEFT|RIGHT|BOTTOM) }
GdkAttachRuleFlip { GDK_ATTACH_RULE_(NONE|X|Y) }

though.

Also, "ALLOW_FLIP" I'm not sure is the best name. It's not about allowing, it's about preferring. Of course there are scenarios when flipping is impossible (rare, but if the menu is very tall and can't be y-flipped or something).

I wonder also if we should some how allow the client to constraining in a certain direction? A popover for example (while I suppose always within the window) always needs to be centered horizontally.

(In reply to Jonas Ådahl from comment #157)
> Hmm? How do you mean opposite of its anchor? The anchor might be top|right
> when the gravity is bottom|left, but there are scenarios when the anchor is
> bottom|right and the gravity is also bottom|right.

Basically, pick an anchor point of the attach rect and an anchor point of the window. Then we want to "pin" them together.

Looking at it this way, for example, if the gravity is bottom|right (i.e. window opens down and right), then this is equivalent to "pinning" the top-left of the window to the attach rect.

Not sure if I'm being completely clear here, but I'll definitely add some new diagrams to illustrate this.

(In reply to William Hua from comment #159)
> (In reply to Jonas Ådahl from comment #157)
> > Hmm? How do you mean opposite of its anchor? The anchor might be top|right
> > when the gravity is bottom|left, but there are scenarios when the anchor is
> > bottom|right and the gravity is also bottom|right.
> 
> Basically, pick an anchor point of the attach rect and an anchor point of
> the window. Then we want to "pin" them together.
> 
> Looking at it this way, for example, if the gravity is bottom|right (i.e.
> window opens down and right), then this is equivalent to "pinning" the
> top-left of the window to the attach rect.
> 
> Not sure if I'm being completely clear here, but I'll definitely add some
> new diagrams to illustrate this.

Now I think I got it. The window anchor is the opposite of gravity. I got confused by all the different anchors on the different rects :P

Maybe we should have "anchor" somewhere in the enum name as well?

(In reply to Jonas Ådahl from comment #158)
> (In reply to William Hua from comment #156)
> > Maybe we should even separate the flip flags out of GdkAttachRule and into
> > independent gbooleans in GdkAttachParams...
> 
> I think its good to keep it an enum at least. You could consider splitting
> the catch-it-all enum into
> 
> GdkAttachRuleRect { GDK_ATTACH_RULE_RECT_(TOP|LEFT|RIGHT|BOTTOM) }
> GdkAttachRuleWindow { GDK_ATTACH_RULE_WINDOW_(TOP|LEFT|RIGHT|BOTTOM) }
> GdkAttachRuleFlip { GDK_ATTACH_RULE_(NONE|X|Y) }
> 
> though.

> Maybe we should have "anchor" somewhere in the enum name as well?

Heh, sure, that sounds good to me as well. But I see that GdkGravity already exists though. Do you think we should reuse it? Have one GdkGravity for each rectangle?



> Also, "ALLOW_FLIP" I'm not sure is the best name. It's not about allowing,
> it's about preferring. Of course there are scenarios when flipping is
> impossible (rare, but if the menu is very tall and can't be y-flipped or
> something).

Just my personal opinion... I think "ALLOW FLIP" should be fine even if there are situations where it is impossible to flip, as it basically is a flag saying to try it if the first case doesn't work, as opposed to the alternative which is to not try it at all, i.e. disallow flipping.



> I wonder also if we should some how allow the client to constraining in a
> certain direction? A popover for example (while I suppose always within the
> window) always needs to be centered horizontally.

That case would be covered by not specifying GDK_ATTACH_WINDOW_LEFT or GDK_ATTACH_WINDOW_RIGHT (GDK_GRAVITY_NORTH if we switch to GdkGravity's).

(In reply to William Hua from comment #161)
> (In reply to Jonas Ådahl from comment #158)
> > (In reply to William Hua from comment #156)
> > > Maybe we should even separate the flip flags out of GdkAttachRule and into
> > > independent gbooleans in GdkAttachParams...
> > 
> > I think its good to keep it an enum at least. You could consider splitting
> > the catch-it-all enum into
> > 
> > GdkAttachRuleRect { GDK_ATTACH_RULE_RECT_(TOP|LEFT|RIGHT|BOTTOM) }
> > GdkAttachRuleWindow { GDK_ATTACH_RULE_WINDOW_(TOP|LEFT|RIGHT|BOTTOM) }
> > GdkAttachRuleFlip { GDK_ATTACH_RULE_(NONE|X|Y) }
> > 
> > though.
> 
> > Maybe we should have "anchor" somewhere in the enum name as well?
> 
> Heh, sure, that sounds good to me as well. But I see that GdkGravity already
> exists though. Do you think we should reuse it? Have one GdkGravity for each
> rectangle?
> 

Hmm, reusing GdkGravity could work I guess, but we'd have to redefine what the enum represents and the values mean (at least "static") since they talk about window decoration etc.

> 
> 
> > Also, "ALLOW_FLIP" I'm not sure is the best name. It's not about allowing,
> > it's about preferring. Of course there are scenarios when flipping is
> > impossible (rare, but if the menu is very tall and can't be y-flipped or
> > something).
> 
> Just my personal opinion... I think "ALLOW FLIP" should be fine even if
> there are situations where it is impossible to flip, as it basically is a
> flag saying to try it if the first case doesn't work, as opposed to the
> alternative which is to not try it at all, i.e. disallow flipping.
> 

The problem with "allow" though is that it doesn't state the fact that if window is constrained, it flip and not slide. Allow makes it ambiguous what happens. The default is to allow arbitrary movement, then allow flip makes both allowed, no?

> 
> 
> > I wonder also if we should some how allow the client to constraining in a
> > certain direction? A popover for example (while I suppose always within the
> > window) always needs to be centered horizontally.
> 
> That case would be covered by not specifying GDK_ATTACH_WINDOW_LEFT or
> GDK_ATTACH_WINDOW_RIGHT (GDK_GRAVITY_NORTH if we switch to GdkGravity's).

Not specifying a window_right|window_left, why wouldn't that just be centering? I don't see how it connects to being able to move or not. Maybe it can be baked into the same enum as FLIP. Something like GdkAttachRuleConstrainStrategy { FLIP_X, FLIP_Y, SLIDE_X, SLIDE_Y, FIXED_X, FIXED_Y }.

Created attachment 319410 [details] [review]
gdkwindow: move GdkWindowEdge to gdktypes.h

Created attachment 319411 [details] [review]
gdktypes: add GDK_WINDOW_EDGE_CENTER

Created attachment 319412 [details] [review]
gdkattachparams: add GdkAttachParams

Created attachment 319413 [details] [review]
gdkwindow: add gdk_window_move_using_params ()

Created attachment 319414 [details] [review]
gdkwindowimpl: implement move_using_params ()

Created attachment 319415 [details] [review]
gdkwindow-mir: implement move_using_params ()

Created attachment 319416 [details] [review]
gtkmenu: add gtk_menu_popup_with_params ()

Created attachment 319417 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_params ()

Created attachment 319418 [details] [review]
gtkmenubutton: use gtk_menu_popup_with_params ()

Created attachment 319419 [details] [review]
gtktoolbar: use gtk_menu_popup_with_params ()

Created attachment 319420 [details] [review]
gtklabel: use gtk_menu_popup_with_params ()

Created attachment 319421 [details] [review]
gtkentry: use gtk_menu_popup_with_params ()

Created attachment 319422 [details] [review]
gtktextview: use gtk_menu_popup_with_params ()

Created attachment 319423 [details] [review]
gtklinkbutton: use gtk_menu_popup_with_params ()

Created attachment 319424 [details] [review]
gtknotebook: use gtk_menu_popup_with_params ()

Created attachment 319425 [details] [review]
gtkwindow: use gtk_menu_popup_with_params ()

Created attachment 319426 [details] [review]
gtkrecentchooserdefault: use gtk_menu_popup_with_params ()

Created attachment 319427 [details] [review]
gtkplacesview: use gtk_menu_popup_with_params ()

Created attachment 319428 [details] [review]
gtkappchooserwidget: use gtk_menu_popup_with_params ()

Created attachment 319429 [details] [review]
gtkmountoperation: use gtk_menu_popup_with_params ()

Created attachment 319430 [details] [review]
gtkcolorsel: use gtk_menu_popup_with_params ()

Created attachment 319431 [details] [review]
gtkcombobox: use gtk_menu_popup_with_params ()

Hi Jonas,



(In reply to Jonas Ådahl from comment #162)
> > Heh, sure, that sounds good to me as well. But I see that GdkGravity already
> > exists though. Do you think we should reuse it? Have one GdkGravity for each
> > rectangle?
> > 
> 
> Hmm, reusing GdkGravity could work I guess, but we'd have to redefine what
> the enum represents and the values mean (at least "static") since they talk
> about window decoration etc.

I ended up re-using the GdkWindowEdge enum that already exists, since it seems to be almost exactly what we need here for defining the anchor points. The only change we needed there was to add an extra GDK_WINDOW_EDGE_CENTER.



> > > Also, "ALLOW_FLIP" I'm not sure is the best name. It's not about allowing,
> > > it's about preferring. Of course there are scenarios when flipping is
> > > impossible (rare, but if the menu is very tall and can't be y-flipped or
> > > something).
> > 
> > Just my personal opinion... I think "ALLOW FLIP" should be fine even if
> > there are situations where it is impossible to flip, as it basically is a
> > flag saying to try it if the first case doesn't work, as opposed to the
> > alternative which is to not try it at all, i.e. disallow flipping.
> > 
> 
> The problem with "allow" though is that it doesn't state the fact that if
> window is constrained, it flip and not slide. Allow makes it ambiguous what
> happens. The default is to allow arbitrary movement, then allow flip makes
> both allowed, no?

I don't quite understand the argument here, but anyways, I just ended up calling them flip_x and flip_y.



> > > I wonder also if we should some how allow the client to constraining in a
> > > certain direction? A popover for example (while I suppose always within the
> > > window) always needs to be centered horizontally.
> > 
> > That case would be covered by not specifying GDK_ATTACH_WINDOW_LEFT or
> > GDK_ATTACH_WINDOW_RIGHT (GDK_GRAVITY_NORTH if we switch to GdkGravity's).
> 
> Not specifying a window_right|window_left, why wouldn't that just be
> centering? I don't see how it connects to being able to move or not. Maybe
> it can be baked into the same enum as FLIP. Something like
> GdkAttachRuleConstrainStrategy { FLIP_X, FLIP_Y, SLIDE_X, SLIDE_Y, FIXED_X,
> FIXED_Y }.

If I understand this correctly, you want extra flags to indicate that the position is non-negotiable for that axis? As in, "force it to this position, even if it goes off-screen?"

Created attachment 319457 [details] [review]
gdktypes: add GDK_WINDOW_EDGE_CENTER

Created attachment 319458 [details] [review]
gdkattachparams: add GdkAttachParams

Created attachment 319459 [details] [review]
gdkwindow-mir: implement move_using_params ()

Created attachment 319460 [details] [review]
gtkmenu: add gtk_menu_popup_with_params ()

Created attachment 319461 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_params ()

Created attachment 319462 [details] [review]
gtkmenubutton: use gtk_menu_popup_with_params ()

Created attachment 319463 [details] [review]
gtktoolbar: use gtk_menu_popup_with_params ()

Created attachment 319464 [details] [review]
gtknotebook: use gtk_menu_popup_with_params ()

Created attachment 319466 [details] [review]
gdkattachparams: add GdkAttachParams

(In reply to William Hua from comment #185)
> Hi Jonas,
> 
> 
> > > > I wonder also if we should some how allow the client to constraining in a
> > > > certain direction? A popover for example (while I suppose always within the
> > > > window) always needs to be centered horizontally.
> > > 
> > > That case would be covered by not specifying GDK_ATTACH_WINDOW_LEFT or
> > > GDK_ATTACH_WINDOW_RIGHT (GDK_GRAVITY_NORTH if we switch to GdkGravity's).
> > 
> > Not specifying a window_right|window_left, why wouldn't that just be
> > centering? I don't see how it connects to being able to move or not. Maybe
> > it can be baked into the same enum as FLIP. Something like
> > GdkAttachRuleConstrainStrategy { FLIP_X, FLIP_Y, SLIDE_X, SLIDE_Y, FIXED_X,
> > FIXED_Y }.
> 
> If I understand this correctly, you want extra flags to indicate that the
> position is non-negotiable for that axis? As in, "force it to this position,
> even if it goes off-screen?"

Yea, but we don't need to add it now in order to land this. If we need it, we can be added later.

Created attachment 319773 [details] [review]
gdkattachparams: add GdkAttachParams

Created attachment 319774 [details] [review]
gtkmenu: add gtk_menu_popup_with_params ()

Created attachment 319775 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_params ()

Created attachment 319776 [details] [review]
gtkmenubutton: use gtk_menu_popup_with_params ()

Created attachment 319777 [details] [review]
gtktoolbar: use gtk_menu_popup_with_params ()

Created attachment 319778 [details] [review]
gtklabel: use gtk_menu_popup_with_params ()

Created attachment 319779 [details] [review]
gtkentry: use gtk_menu_popup_with_params ()

Created attachment 319780 [details] [review]
gtktextview: use gtk_menu_popup_with_params ()

Created attachment 319781 [details] [review]
gtklinkbutton: use gtk_menu_popup_with_params ()

Created attachment 319782 [details] [review]
gtknotebook: use gtk_menu_popup_with_params ()

Created attachment 319783 [details] [review]
gtkcolorsel: use gtk_menu_popup_with_params ()

Created attachment 319784 [details] [review]
gtkcombobox: use gtk_menu_popup_with_params ()

Sorry, I made a minor change to the API (adding a separate function for setting the flip flags) which ended up propagating through some of the other commits.

Review of attachment 319774 [details] [review]:

::: gtk/gtkmenuprivate.h
@@ +54,3 @@
   gint                position_y;
 
+  GdkAttachParams   *attach_params;

This doesn't seem to be freed on destruction, only on replacement.

Review of attachment 319773 [details] [review]:

::: gdk/gdkattachparams.c
@@ +22,3 @@
+ * requires knowledge of the geometry of the monitor workarea as well as the
+ * ability to position windows in absolute root window coordinates, which some
+ * GDK backends do not support.

nit: I don't particularly like the wording of this paragraph. "Explicit absolute positioning" sounds like global positioning, which no widget can rely on, so maybe change this to "explicit relative positioning". The case is also that those widgets require non-explicit positioning, not the other way around, since only GDK has the ability to implement the positioning. Lastly, is "root window" a thing in GTK+ in general? It sounds a bit GDK X11:y.

::: gdk/gdkattachparams.h
@@ +26,3 @@
+ * @params: (transfer none) (nullable): the #GdkAttachParams that was used
+ * @x: the final x-coordinate of @window
+ * @y: the final y-coordinate of @window

These coordinates should be relative to either the parent window or the attach rect, not global.

@@ +28,3 @@
+ * @y: the final y-coordinate of @window
+ * @offset_x: the horizontal displacement applied to keep @window on-screen
+ * @offset_y: the vertical displacement applied to keep @window on-screen

Relative to the originally intended position?

::: gdk/gdkattachparamsprivate.h
@@ +68,3 @@
+                                                   gint                  *offset_y,
+                                                   gboolean              *flipped_x,
+                                                   gboolean              *flipped_y);

Can't we just just move these to declarations to the .c file?

@@ +71,3 @@
+
+void gdk_attach_params_move_window                (const GdkAttachParams *params,
+                                                   GdkWindow             *window);

gdk_attach_params_move_window_default ? (to make it obvious that this is just one of many possible ways of moving the window, i.e. won't be called from Mir or Wayland like backends).

Review of attachment 319413 [details] [review]:

::: gdk/gdkwindow.c
@@ +11562,3 @@
+void
+gdk_window_move_using_params (GdkWindow             *window,
+                              const GdkAttachParams *params)

Since 'params' needs to be stored by the backend until the callback is invoked, should we maybe add some promise about validity of 'params'? Like, can it safely be assumed to be valid until the next "gdk_window_move_using_params()" call or until the destruction of GdkWindow?

Review of attachment 319466 [details] [review]:

::: gdk/gdkattachparams.h
@@ +76,3 @@
+GDK_AVAILABLE_IN_3_20
+void              gdk_attach_params_set_attach_margin     (GdkAttachParams       *params,
+                                                           const GdkBorder       *margin);

Whats the reason for adding a margin, instead of just letting gtk+ add that margin to the attach_rect?

@@ +80,3 @@
+GDK_AVAILABLE_IN_3_20
+void              gdk_attach_params_set_window_padding    (GdkAttachParams       *params,
+                                                           const GdkBorder       *padding);

The window padding isn't really padding on the window, but on the main content of the window, i.e. the shadow. (I only see this being the shaddow at least).

Since this information is already available to the backend, should we maybe drop this to avoid duplication?

Review of attachment 319773 [details] [review]:

::: gdk/gdkattachparams.c
@@ +107,3 @@
+ * @params: a #GdkAttachParams
+ * @rectangle: (nullable): the attachment rectangle
+ * @parent: (nullable): the #GdkWindow that @rectangle is relative to

Why are these (nullable)? Does this mean the backend should look at transient_for if there is no parent here? What is the rectangle if NULL was passed? What is the default rectangle if this function was not called at all, or is that not valid?

Tested gtk3-demo using X11 some. One thing I noticed, which I believe is different from before, is that menus now flip both horizontally and vertically, while I believe only horizontal menus flipped vertically, and only vertical menus flipped horizontally.

Review of attachment 319774 [details] [review]:

::: gtk/gtkmenu.c
@@ +1906,3 @@
+  if (widget)
+    {
+      gtk_widget_get_allocation (widget, &allocation);

The allocation here doesn't seem to be correct on Wayland. The allocation seems to have its origin within the CSD, i.e. the elements in the menu bar in gtk3-demo -> builder example, all have the Y coordinate 0. The coordinate you want here is some kind of coordinate the backend will understand, which means the origin needs to be either be left/top of the title bar, left/top of the shadow.

After some discussion on #gtk+ it doesn't seem to be trivial to calculate this (one needs to take the allocation, then add a title bar height, then maybe the shadow width which is not even available via the public API). I wonder, should we add a GtkWidget API that calculates the allocation relative to the GtkWindow/output-GdkWindow (including title bar and shadow margin etc)?

(In reply to Jonas Ådahl from comment #215)
> Review of attachment 319774 [details] [review] [review]:
> 
> ::: gtk/gtkmenu.c
> @@ +1906,3 @@
> +  if (widget)
> +    {
> +      gtk_widget_get_allocation (widget, &allocation);
> 
> The allocation here doesn't seem to be correct on Wayland. The allocation
> seems to have its origin within the CSD, i.e. the elements in the menu bar
> in gtk3-demo -> builder example, all have the Y coordinate 0. The coordinate
> you want here is some kind of coordinate the backend will understand, which
> means the origin needs to be either be left/top of the title bar, left/top
> of the shadow.
> 
> After some discussion on #gtk+ it doesn't seem to be trivial to calculate
> this (one needs to take the allocation, then add a title bar height, then
> maybe the shadow width which is not even available via the public API). I
> wonder, should we add a GtkWidget API that calculates the allocation
> relative to the GtkWindow/output-GdkWindow (including title bar and shadow
> margin etc)?

When trying, I can't seem to make the allocation offsets to have any consistency. Sometimes they are relative to the inner window ((0, 0) is just under the title bar, example: the menu bar in gtk3-demo-application) and sometimes they are relative to outside of the shadow region (example: the [v] button next to the open icon in gtk3-demo-application).

mclasen, what would be a reliable way to get an allocation in toplevel window coordinate space? I.e. a space including the title bar and shadow if there are any.

(In reply to Jonas Ådahl from comment #216)
> (In reply to Jonas Ådahl from comment #215)
> > Review of attachment 319774 [details] [review] [review] [review]:
> > 
> > ::: gtk/gtkmenu.c
> > @@ +1906,3 @@
> > +  if (widget)
> > +    {
> > +      gtk_widget_get_allocation (widget, &allocation);
> > 

...snip...

> When trying, I can't seem to make the allocation offsets to have any
> consistency. Sometimes they are relative to the inner window ((0, 0) is just
> under the title bar, example: the menu bar in gtk3-demo-application) and
> sometimes they are relative to outside of the shadow region (example: the
> [v] button next to the open icon in gtk3-demo-application).
> 
> mclasen, what would be a reliable way to get an allocation in toplevel
> window coordinate space? I.e. a space including the title bar and shadow if
> there are any.

Replying to myself here again. After some more IRC, this method seems to work:

      GtkAllocation allocation;
      GtkWidget *toplevel;

      gtk_widget_get_allocation (widget, &allocation);
      toplevel = gtk_widget_get_toplevel (widget);

      gtk_widget_translate_coordinates (widget, toplevel,
                                        0, 0,
                                        &allocation.x, &allocation.y);

      gdk_attach_params_set_attach_rect (params, &allocation,
                                         gtk_widget_get_window (widget));

Created attachment 319981 [details] [review]
gdkattachparams: add GdkAttachParams

Created attachment 319982 [details] [review]
gdkwindowimpl: implement move_using_params ()

Created attachment 319983 [details] [review]
gtkmenu: add gtk_menu_popup_with_params ()

Created attachment 319984 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_params ()

Created attachment 319985 [details] [review]
gtkmenubutton: use gtk_menu_popup_with_params ()

Created attachment 319986 [details] [review]
gtktoolbar: use gtk_menu_popup_with_params ()

Created attachment 319987 [details] [review]
gtklabel: use gtk_menu_popup_with_params ()

Created attachment 319988 [details] [review]
gtkentry: use gtk_menu_popup_with_params ()

Created attachment 319989 [details] [review]
gtktextview: use gtk_menu_popup_with_params ()

Created attachment 319990 [details] [review]
gtklinkbutton: use gtk_menu_popup_with_params ()

Created attachment 319991 [details] [review]
gtknotebook: use gtk_menu_popup_with_params ()

Created attachment 319992 [details] [review]
gtkcombobox: use gtk_menu_popup_with_params ()

(In reply to Jonas Ådahl from comment #209)
> Review of attachment 319774 [details] [review] [review]:
> 
> ::: gtk/gtkmenuprivate.h
> @@ +54,3 @@
>    gint                position_y;
>  
> +  GdkAttachParams   *attach_params;
> 
> This doesn't seem to be freed on destruction, only on replacement.

Thanks, fixed.



> ::: gdk/gdkattachparams.c
> @@ +22,3 @@
> + * requires knowledge of the geometry of the monitor workarea as well as the
> + * ability to position windows in absolute root window coordinates, which
> some
> + * GDK backends do not support.
> 
> nit: I don't particularly like the wording of this paragraph. "Explicit
> absolute positioning" sounds like global positioning, which no widget can
> rely on, so maybe change this to "explicit relative positioning". The case
> is also that those widgets require non-explicit positioning, not the other
> way around, since only GDK has the ability to implement the positioning.
> Lastly, is "root window" a thing in GTK+ in general? It sounds a bit GDK
> X11:y.

Changed it to just "explicit positioning" because I'm not sure relative is the right word as that's what we're trying to implement here. Also replaced "root window coordinates" with "screen coordinates" here.



> ::: gdk/gdkattachparams.h
> @@ +26,3 @@
> + * @params: (transfer none) (nullable): the #GdkAttachParams that was used
> + * @x: the final x-coordinate of @window
> + * @y: the final y-coordinate of @window
> 
> These coordinates should be relative to either the parent window or the
> attach rect, not global.

Fixed.



> @@ +28,3 @@
> + * @y: the final y-coordinate of @window
> + * @offset_x: the horizontal displacement applied to keep @window on-screen
> + * @offset_y: the vertical displacement applied to keep @window on-screen
> 
> Relative to the originally intended position?

Sure, fixed.



> ::: gdk/gdkattachparamsprivate.h
> @@ +68,3 @@
> +                                                   gint                 
> *offset_y,
> +                                                   gboolean             
> *flipped_x,
> +                                                   gboolean             
> *flipped_y);
> 
> Can't we just just move these to declarations to the .c file?

Sure.



> @@ +71,3 @@
> +
> +void gdk_attach_params_move_window                (const GdkAttachParams
> *params,
> +                                                   GdkWindow            
> *window);
> 
> gdk_attach_params_move_window_default ? (to make it obvious that this is
> just one of many possible ways of moving the window, i.e. won't be called
> from Mir or Wayland like backends).

Fixed.



> ::: gdk/gdkwindow.c
> @@ +11562,3 @@
> +void
> +gdk_window_move_using_params (GdkWindow             *window,
> +                              const GdkAttachParams *params)
> 
> Since 'params' needs to be stored by the backend until the callback is
> invoked, should we maybe add some promise about validity of 'params'? Like,
> can it safely be assumed to be valid until the next
> "gdk_window_move_using_params()" call or until the destruction of GdkWindow?

I think this is really the responsibility of the backend to retain a copy of params if it thinks it's going to need it to invoke the callback at some future time. Once Wayland and Mir support position feedback, we can add gdk_attach_params_copy() so they can do this properly.



> ::: gdk/gdkattachparams.h
> @@ +76,3 @@
> +GDK_AVAILABLE_IN_3_20
> +void              gdk_attach_params_set_attach_margin     (GdkAttachParams 
> *params,
> +                                                           const GdkBorder 
> *margin);
> 
> Whats the reason for adding a margin, instead of just letting gtk+ add that
> margin to the attach_rect?

We don't always use the margin in some cases and I would prefer let the backend also decide how to handle the margin.

Also, for example, in the default implementation, we're only using the margin in those cases that the attachment rectangle and window don't overlap in that direction (i.e. they appear side-by-side). If they do overlap at all, then we're ignoring the margin because it doesn't really make sense to apply it there.



> @@ +80,3 @@
> +GDK_AVAILABLE_IN_3_20
> +void              gdk_attach_params_set_window_padding    (GdkAttachParams 
> *params,
> +                                                           const GdkBorder 
> *padding);
> 
> The window padding isn't really padding on the window, but on the main
> content of the window, i.e. the shadow. (I only see this being the shaddow
> at least).
> 
> Since this information is already available to the backend, should we maybe
> drop this to avoid duplication?

How can the backend know this? I thought the shadow widths came from GTK+.



> ::: gdk/gdkattachparams.c
> @@ +107,3 @@
> + * @params: a #GdkAttachParams
> + * @rectangle: (nullable): the attachment rectangle
> + * @parent: (nullable): the #GdkWindow that @rectangle is relative to
> 
> Why are these (nullable)? Does this mean the backend should look at
> transient_for if there is no parent here? What is the rectangle if NULL was
> passed? What is the default rectangle if this function was not called at
> all, or is that not valid?

This is not easy to change because gtk_menu_popup() and its variants allow the user to pop up the menu at the current pointer position, independent of any parent window. So that's a case where the parent might be NULL. Anyways, I would rather there be no pre-conceptions by the backend to assume that the attach_rect and attach_parent will always be available. The backend also needs to be robust enough to handle the case that gdk_attach_params_set_attach_rect() is not called.



> Tested gtk3-demo using X11 some. One thing I noticed, which I believe is
> different from before, is that menus now flip both horizontally and
> vertically, while I believe only horizontal menus flipped vertically, and
> only vertical menus flipped horizontally.

Thanks, fixed this for all the affected widgets. The widgets should more-or-less flip/slide the way they were before.



> > > ::: gtk/gtkmenu.c
> > > @@ +1906,3 @@
> > > +  if (widget)
> > > +    {
> > > +      gtk_widget_get_allocation (widget, &allocation);
> > > 
> 
> ...snip...
> 
> > When trying, I can't seem to make the allocation offsets to have any
> > consistency. Sometimes they are relative to the inner window ((0, 0) is just
> > under the title bar, example: the menu bar in gtk3-demo-application) and
> > sometimes they are relative to outside of the shadow region (example: the
> > [v] button next to the open icon in gtk3-demo-application).
> > 
> > mclasen, what would be a reliable way to get an allocation in toplevel
> > window coordinate space? I.e. a space including the title bar and shadow if
> > there are any.
> 
> Replying to myself here again. After some more IRC, this method seems to
> work:
> 
>       GtkAllocation allocation;
>       GtkWidget *toplevel;
> 
>       gtk_widget_get_allocation (widget, &allocation);
>       toplevel = gtk_widget_get_toplevel (widget);
> 
>       gtk_widget_translate_coordinates (widget, toplevel,
>                                         0, 0,
>                                         &allocation.x, &allocation.y);
> 
>       gdk_attach_params_set_attach_rect (params, &allocation,
>                                          gtk_widget_get_window (widget));

Using the default implementation, the menus seem to be positioned properly under Weston. Are the positioning issues happening with the wip/attach-params branch or is it a patch you're currently working on?

(In reply to William Hua from comment #230)
> (In reply to Jonas Ådahl from comment #209)
> > Review of attachment 319774 [details] [review] [review] [review]:
> >
> 

..snip..

> 
> > ::: gdk/gdkwindow.c
> > @@ +11562,3 @@
> > +void
> > +gdk_window_move_using_params (GdkWindow             *window,
> > +                              const GdkAttachParams *params)
> > 
> > Since 'params' needs to be stored by the backend until the callback is
> > invoked, should we maybe add some promise about validity of 'params'? Like,
> > can it safely be assumed to be valid until the next
> > "gdk_window_move_using_params()" call or until the destruction of GdkWindow?
> 
> I think this is really the responsibility of the backend to retain a copy of
> params if it thinks it's going to need it to invoke the callback at some
> future time. Once Wayland and Mir support position feedback, we can add
> gdk_attach_params_copy() so they can do this properly.

Hmm. We'd need a "copy_user_data" vfunc to copy the callback user data, or unset it from the original params behind the callers back.

> 
> 
> 
> > ::: gdk/gdkattachparams.h
> > @@ +76,3 @@
> > +GDK_AVAILABLE_IN_3_20
> > +void              gdk_attach_params_set_attach_margin     (GdkAttachParams 
> > *params,
> > +                                                           const GdkBorder 
> > *margin);
> > 
> > Whats the reason for adding a margin, instead of just letting gtk+ add that
> > margin to the attach_rect?
> 
> We don't always use the margin in some cases and I would prefer let the
> backend also decide how to handle the margin.
> 
> Also, for example, in the default implementation, we're only using the
> margin in those cases that the attachment rectangle and window don't overlap
> in that direction (i.e. they appear side-by-side). If they do overlap at
> all, then we're ignoring the margin because it doesn't really make sense to
> apply it there.

Not sure I follow what you mean here. How exactly does the behavior differ if the attach_rect+margin is the same as attach_rect with 0-width margin?

> 
> 
> 
> > @@ +80,3 @@
> > +GDK_AVAILABLE_IN_3_20
> > +void              gdk_attach_params_set_window_padding    (GdkAttachParams 
> > *params,
> > +                                                           const GdkBorder 
> > *padding);
> > 
> > The window padding isn't really padding on the window, but on the main
> > content of the window, i.e. the shadow. (I only see this being the shaddow
> > at least).
> > 
> > Since this information is already available to the backend, should we maybe
> > drop this to avoid duplication?
> 
> How can the backend know this? I thought the shadow widths came from GTK+.
> 

https://git.gnome.org/browse/gtk+/tree/gdk/gdkwindowimpl.h#n290

> 
> 
> > ::: gdk/gdkattachparams.c
> > @@ +107,3 @@
> > + * @params: a #GdkAttachParams
> > + * @rectangle: (nullable): the attachment rectangle
> > + * @parent: (nullable): the #GdkWindow that @rectangle is relative to
> > 
> > Why are these (nullable)? Does this mean the backend should look at
> > transient_for if there is no parent here? What is the rectangle if NULL was
> > passed? What is the default rectangle if this function was not called at
> > all, or is that not valid?
> 
> This is not easy to change because gtk_menu_popup() and its variants allow
> the user to pop up the menu at the current pointer position, independent of
> any parent window. So that's a case where the parent might be NULL. Anyways,
> I would rather there be no pre-conceptions by the backend to assume that the
> attach_rect and attach_parent will always be available. The backend also
> needs to be robust enough to handle the case that
> gdk_attach_params_set_attach_rect() is not called.
> 

That gtk+ allows this is IMO a bug in the API, and if we design new API, we should avoid letting it be possible. Otherwise the backend needs to start guessing the intention, looking in its pockets for possibly reasonable parents.

> 
> > Tested gtk3-demo using X11 some. One thing I noticed, which I believe is
> > different from before, is that menus now flip both horizontally and
> > vertically, while I believe only horizontal menus flipped vertically, and
> > only vertical menus flipped horizontally.
> 
> Thanks, fixed this for all the affected widgets. The widgets should
> more-or-less flip/slide the way they were before.
> 
> 
> 
> > > > ::: gtk/gtkmenu.c
> > > > @@ +1906,3 @@
> > > > +  if (widget)
> > > > +    {
> > > > +      gtk_widget_get_allocation (widget, &allocation);
> > > > 
> > 
> > ...snip...
> > 
> > > When trying, I can't seem to make the allocation offsets to have any
> > > consistency. Sometimes they are relative to the inner window ((0, 0) is just
> > > under the title bar, example: the menu bar in gtk3-demo-application) and
> > > sometimes they are relative to outside of the shadow region (example: the
> > > [v] button next to the open icon in gtk3-demo-application).
> > > 
> > > mclasen, what would be a reliable way to get an allocation in toplevel
> > > window coordinate space? I.e. a space including the title bar and shadow if
> > > there are any.
> > 
> > Replying to myself here again. After some more IRC, this method seems to
> > work:
> > 
> >       GtkAllocation allocation;
> >       GtkWidget *toplevel;
> > 
> >       gtk_widget_get_allocation (widget, &allocation);
> >       toplevel = gtk_widget_get_toplevel (widget);
> > 
> >       gtk_widget_translate_coordinates (widget, toplevel,
> >                                         0, 0,
> >                                         &allocation.x, &allocation.y);
> > 
> >       gdk_attach_params_set_attach_rect (params, &allocation,
> >                                          gtk_widget_get_window (widget));
> 
> Using the default implementation, the menus seem to be positioned properly
> under Weston. Are the positioning issues happening with the
> wip/attach-params branch or is it a patch you're currently working on?

I have a wip branches locally here which adds a move_using_params implementation to the Wayland backend. It consists of a mutter branch, a wayland-protocols branch, and a gtk+ branch. If you want, I can push them somewhere.

Review of attachment 319981 [details] [review]:

::: gdk/gdkattachparams.c
@@ +1,1 @@
+#include "config.h"

Should have a Copyright header above #include "config.h" (and in the other new files as well) right?

@@ +37,3 @@
+ * In addition, you can also specify additional "flip" parameters that hint to
+ * the backend that the anchors can be mirrored if not enough space is
+ * available horizontally or vertically on-screen.

This documentation blurb makes it sound like the default is "don't flip", but the default seems to be "flip".

Just pushed a set of branches making it possible to test the current work-in-progress Wayland implementation.

It consists of 4 branches: wayland-protocols, clutter, mutter, gtk+. The clutter branch is needed only because the mutter branch is based on the relative pointer and pointer constraint branch, and it is non-trivial to rebase it on top of just master.

The branches are:
https://github.com/jadahl/wayland-protocols/tree/wip/xdg-positioner
https://github.com/jadahl/clutter/tree/wip/relative-pointer
https://github.com/jadahl/mutter/tree/wip/xdg-positioner
https://github.com/jadahl/gtk/tree/wip/attach-params

It might be possible to disable the native backend in mutter during ./configure and skip the clutter branch but I have not tested that.

To test without running mutter from a VT, one can run (from the mutter/ dir) "./src/mutter --nested" and a nested mutter Wayland compositor will pop up.

(In reply to Jonas Ådahl from comment #233)
> Just pushed a set of branches making it possible to test the current
> work-in-progress Wayland implementation.
> 
> It consists of 4 branches: wayland-protocols, clutter, mutter, gtk+. The
> clutter branch is needed only because the mutter branch is based on the
> relative pointer and pointer constraint branch, and it is non-trivial to
> rebase it on top of just master.
> 
> The branches are:
> https://github.com/jadahl/wayland-protocols/tree/wip/xdg-positioner
> https://github.com/jadahl/clutter/tree/wip/relative-pointer
> https://github.com/jadahl/mutter/tree/wip/xdg-positioner
> https://github.com/jadahl/gtk/tree/wip/attach-params
> 
> It might be possible to disable the native backend in mutter during
> ./configure and skip the clutter branch but I have not tested that.

I just force-pushed the mutter branch. You don't need the clutter branch to test if you disable the native backend with "./configure --disable-native-backend".

> 
> To test without running mutter from a VT, one can run (from the mutter/ dir)
> "./src/mutter --nested" and a nested mutter Wayland compositor will pop up.

Sorry, that'd only work if its running from a Wayland session I think. To make it more fail safe, it needs to be "./src/mutter --wayland --nested".

Created attachment 320343 [details] [review]
gdkborder: remove unneeded definition

Created attachment 320344 [details] [review]
gdkwindow: store shadow sizes

Created attachment 320345 [details] [review]
gdkattachparams: add GdkAttachParams

Created attachment 320346 [details] [review]
gdkwindow: add gdk_window_move_using_params ()

Created attachment 320347 [details] [review]
gdkwindowimpl: implement move_using_params ()

Created attachment 320348 [details] [review]
gdkwindow-mir: implement move_using_params ()

Created attachment 320349 [details] [review]
gtkmenu: add gtk_menu_popup_with_params ()

Created attachment 320350 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_params ()

Created attachment 320351 [details] [review]
gtkmenubutton: use gtk_menu_popup_with_params ()

Created attachment 320352 [details] [review]
gtktoolbar: use gtk_menu_popup_with_params ()

Created attachment 320353 [details] [review]
gtklabel: use gtk_menu_popup_with_params ()

Created attachment 320354 [details] [review]
gtkentry: use gtk_menu_popup_with_params ()

Created attachment 320355 [details] [review]
gtktextview: use gtk_menu_popup_with_params ()

Created attachment 320356 [details] [review]
gtklinkbutton: use gtk_menu_popup_with_params ()

Created attachment 320357 [details] [review]
gtknotebook: use gtk_menu_popup_with_params ()

Created attachment 320358 [details] [review]
gtkwindow: use gtk_menu_popup_with_params ()

Created attachment 320359 [details] [review]
gtkrecentchooserdefault: use gtk_menu_popup_with_params ()

Created attachment 320360 [details] [review]
gtkplacesview: use gtk_menu_popup_with_params ()

Created attachment 320361 [details] [review]
gtkappchooserwidget: use gtk_menu_popup_with_params ()

Created attachment 320362 [details] [review]
gtkmountoperation: use gtk_menu_popup_with_params ()

Created attachment 320363 [details] [review]
gtkcolorsel: use gtk_menu_popup_with_params ()

Created attachment 320364 [details] [review]
gtkcombobox: use gtk_menu_popup_with_params ()

I decided to switch away from using gbooleans for the flip_x and flip_y and use an enum GdkAttachHints instead because you mentioned the possibility of adding fixed_x and fixed_y flags in the future for popovers.

I also created a new enum GdkAttachAnchor for the anchors instead, which makes it a bit easier to extract the horizontal and vertical components separately, as well as giving us some space for new additions in the future.



> Hmm. We'd need a "copy_user_data" vfunc to copy the callback user data, or
> unset it from the original params behind the callers back.

Good point, didn't think about that initially. I've changed it to a GObject (GInitiallyUnowned) so that we can use ref-counting semantics instead of copying.



> Not sure I follow what you mean here. How exactly does the behavior differ
> if the attach_rect+margin is the same as attach_rect with 0-width margin?

Ok, ended up removing margin and just adding it to the attach_rect instead. For the two cases where we were using the margin, together v. separate made no real difference.



> > How can the backend know this? I thought the shadow widths came from GTK+.
> > 
> 
> https://git.gnome.org/browse/gtk+/tree/gdk/gdkwindowimpl.h#n290

Ok, I removed the window_padding that was intended for the shadow and added gdk_window_get_shadow_widths (), which the default implementation uses. So that means there is nothing currently using GdkBorder any more, so I also have a patch for reverting GdkBorder, which I hope is ok since we're still unstable.



> > This is not easy to change because gtk_menu_popup() and its variants allow
> > the user to pop up the menu at the current pointer position, independent of
> > any parent window. So that's a case where the parent might be NULL. Anyways,
> > I would rather there be no pre-conceptions by the backend to assume that the
> > attach_rect and attach_parent will always be available. The backend also
> > needs to be robust enough to handle the case that
> > gdk_attach_params_set_attach_rect() is not called.
> > 
> 
> That gtk+ allows this is IMO a bug in the API, and if we design new API, we
> should avoid letting it be possible. Otherwise the backend needs to start
> guessing the intention, looking in its pockets for possibly reasonable
> parents.

Ok, these are explicitly (not nullable) now. I sifted through and looked at all the potential places they could've been NULL and handled them as best I could, so we should be good.



> Should have a Copyright header above #include "config.h" (and in the other
> new files as well) right?

D'oh! Thanks, fixed... :)



> @@ +37,3 @@
> + * In addition, you can also specify additional "flip" parameters that hint
> to
> + * the backend that the anchors can be mirrored if not enough space is
> + * available horizontally or vertically on-screen.
> 
> This documentation blurb makes it sound like the default is "don't flip",
> but the default seems to be "flip".

Fixed.



*snip*

> 
> I just force-pushed the mutter branch. You don't need the clutter branch to
> test if you disable the native backend with "./configure
> --disable-native-backend".
> 
> > 
> > To test without running mutter from a VT, one can run (from the mutter/ dir)
> > "./src/mutter --nested" and a nested mutter Wayland compositor will pop up.
> 
> Sorry, that'd only work if its running from a Wayland session I think. To
> make it more fail safe, it needs to be "./src/mutter --wayland --nested".

Can you please re-test it again based on the current branch? I fixed a minor bug with the positioning, so maybe it fixed this too.

Also, I tried to run this, but ran into trouble. I'll ping you out of band for help, but I'm a bit busy with some other things, so I'm not sure if I'll have a lot of time to look at it...

> Ok, I removed the window_padding that was intended for the shadow and added
> gdk_window_get_shadow_widths (), which the default implementation uses. So
> that means there is nothing currently using GdkBorder any more, so I also
> have a patch for reverting GdkBorder, which I hope is ok since we're still
> unstable.

Err... correction. I just stored the shadow widths in the GdkWindow private struct and grab them directly. No new gdk_window_get_shadow_widths () API or anything like that.

(In reply to William Hua from comment #257)

> 
> > > How can the backend know this? I thought the shadow widths came from GTK+.
> > > 
> > 
> > https://git.gnome.org/browse/gtk+/tree/gdk/gdkwindowimpl.h#n290
> 
> Ok, I removed the window_padding that was intended for the shadow and added
> gdk_window_get_shadow_widths (), which the default implementation uses. So
> that means there is nothing currently using GdkBorder any more, so I also
> have a patch for reverting GdkBorder, which I hope is ok since we're still
> unstable.

Yes, thats still ok. Feel free to push that revert.

Jonas, William, if you two agree on the api here, please merge this as you see fit (that is, assuming the fallback implementation works in all the backends that don't have their own implementation).

Created attachment 320385 [details] [review]
gdkattachparams: add GdkAttachParams

Review of attachment 320385 [details] [review]:

::: gdk/gdkattachparams.h
@@ +75,3 @@
+  GDK_ATTACH_BOTTOM_RIGHT       = GDK_ATTACH_BOTTOM | GDK_ATTACH_RIGHT,
+  GDK_ATTACH_LEFT_RIGHT         = GDK_ATTACH_LEFT | GDK_ATTACH_RIGHT,
+  GDK_ATTACH_TOP_BOTTOM         = GDK_ATTACH_TOP | GDK_ATTACH_BOTTOM

I don't understand what LEFT_RIGHT and TOP_BOTTOM anchors mean. I understand that it can be used for matching "either top or bottom", but it doesn't make sense as a valid anchor. As such, I think those should be dropped. If anything, we could add "gdk_attach_anchor_is_*()" helper functions, but at least keep the enum values limited to only ones that make sense to specify.

@@ +80,3 @@
+/**
+ * GdkAttachHints:
+ * @GDK_ATTACH_NO_HINTS: the backend should assume nothing

I think something must be assumed for "NO_HINTS" as in that the method for not overflowing off-screen should simply be to move the window on-screen, as described in the overview documention (SECTION gdkattachparams). I guess we could say that "NO_HINTS" means the default assumptions as described in the overview section should be made.

@@ +100,3 @@
+ * GdkAttachCallback:
+ * @window: the #GdkWindow that was moved
+ * @params: (transfer none) (nullable): the #GdkAttachParams that was used

Why is this nullable? I assume the backend will keep a reference to the GdkAttachParams while invoking this callback, so it will always have something to pass here.

@@ +108,3 @@
+ *            on-screen, relative to the originally intended position
+ * @flipped_x: %TRUE if the backend flipped @window horizontally
+ * @flipped_y: %TRUE if the backend flipped @window vertically

It should be fine to just let the backend calculate offset_x/y and flipped_x/y right? It should be possible to do so by just knowing the original attach_rect, anchors and the final x/y the window was positioned at.

Created attachment 320404 [details] [review]
gdkattachparams: add GdkAttachParams

Created attachment 320405 [details] [review]
gdkwindow-mir: implement move_using_params ()

Created attachment 320406 [details] [review]
gtkmenu: add gtk_menu_popup_with_params ()

Created attachment 320407 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_params ()

> I don't understand what LEFT_RIGHT and TOP_BOTTOM anchors mean. I understand
> that it can be used for matching "either top or bottom", but it doesn't make
> sense as a valid anchor. As such, I think those should be dropped. If
> anything, we could add "gdk_attach_anchor_is_*()" helper functions, but at
> least keep the enum values limited to only ones that make sense to specify.

I intended them as bit masks, but you're right, it's probably only adding confusion. Removed.



> @@ +80,3 @@
> +/**
> + * GdkAttachHints:
> + * @GDK_ATTACH_NO_HINTS: the backend should assume nothing
> 
> I think something must be assumed for "NO_HINTS" as in that the method for
> not overflowing off-screen should simply be to move the window on-screen, as
> described in the overview documention (SECTION gdkattachparams). I guess we
> could say that "NO_HINTS" means the default assumptions as described in the
> overview section should be made.

Fixed.



> @@ +100,3 @@
> + * GdkAttachCallback:
> + * @window: the #GdkWindow that was moved
> + * @params: (transfer none) (nullable): the #GdkAttachParams that was used
> 
> Why is this nullable? I assume the backend will keep a reference to the
> GdkAttachParams while invoking this callback, so it will always have
> something to pass here.

Fixed.



> @@ +108,3 @@
> + *            on-screen, relative to the originally intended position
> + * @flipped_x: %TRUE if the backend flipped @window horizontally
> + * @flipped_y: %TRUE if the backend flipped @window vertically
> 
> It should be fine to just let the backend calculate offset_x/y and
> flipped_x/y right? It should be possible to do so by just knowing the
> original attach_rect, anchors and the final x/y the window was positioned at.

Yes, it can be done that way.

I'll test the branches again on Mac OS and Windows to confirm everything is still working.

Review of attachment 320404 [details] [review]:

I'm mostly happy with the API. The hard part I'm having a hard time understand is what exactly the rect represents, or rather what we want it to represent.

::: gdk/gdkattachparams.c
@@ +136,3 @@
+ * Sets the attachment rectangle the window needs to be aligned relative to.
+ * The @rectangle should be in @parent's coordinate space, which means it
+ * should account for the sizes of any client-side window decorations.

We discussed this already, and I'm still not sure what to do here. Currently the implementation does not do this, but is just relative to the "internal" GdkWindow (i.e. what is inside the window decorations, if any).

This seems to work fine in the default move_using_params implementation because "gdk_window_get_origin()" will fetch the window coordinates in "root window coordinates". The "root window coordinates" is retrieved by translating "window->abs_x" and "window->abs_y" (how they are set I have yet to figure out). In wayland this translation is done by traversing the "transient-for" tree, adding the various offsets.

It'd be good with some input from someone who has good understanding about how GdkWindow's work related to CSD vs SSD, and whether it makes sense to rely on some clever calculations to translate any coordinates to "display server" GdkWindow (i.e. including csd, shadow etc), or if we should move to something that is described above, i.e. GTK+ needs to pass a coordinate which is relative to the "display server" GdkWindow, i.e. including csd and shadow if any.

Also, is there any place to read about how GdkWindow's are structured when CSD is used? Is one "body" GdkWindow placed on top of a "container" GdkWindow or something? How does a GdkWindow manage to figure out its position when there CSD is used?

@@ +516,3 @@
+      rect_x = 0;
+      rect_y = 0;
+    }

Guess we can skip the false-branch and just always do the true-branch now that having a parent is a requirement?

@@ +714,3 @@
+
+  if (!params || !params->has_attach_rect)
+    return;

Should we make this g_return_if_fail (params && params->has_attach_rect && params->rect_parent); ?

Created attachment 320446 [details] [review]
gdkattachparams: add GdkAttachParams

Created attachment 320447 [details] [review]
gdkwindow-mir: implement move_using_params ()

Created attachment 320448 [details] [review]
gtkmenu: add gtk_menu_popup_with_params ()

Created attachment 320449 [details] [review]
gtkmenuitem: use gtk_menu_popup_with_params ()

Review of attachment 320404 [details] [review]:

::: gdk/gdkattachparams.c
@@ +516,3 @@
+      rect_x = 0;
+      rect_y = 0;
+    }

Although we require it, the caller could still forget to call gdk_attach_params_set_attach_rect (). So I think it's safer to check for it still.

@@ +714,3 @@
+
+  if (!params || !params->has_attach_rect)
+    return;

Fixed.

> ::: gdk/gdkattachparams.c
> @@ +136,3 @@
> + * Sets the attachment rectangle the window needs to be aligned relative to.
> + * The @rectangle should be in @parent's coordinate space, which means it
> + * should account for the sizes of any client-side window decorations.
> 
> We discussed this already, and I'm still not sure what to do here. Currently
> the implementation does not do this, but is just relative to the "internal"
> GdkWindow (i.e. what is inside the window decorations, if any).

So the internal GdkWindow is non-native then? For the Wayland backend, can we try something simple like calculating the rectangle relative to first native GdkWindow we encounter up the hierarchy?

(In reply to William Hua from comment #273)
> > ::: gdk/gdkattachparams.c
> > @@ +136,3 @@
> > + * Sets the attachment rectangle the window needs to be aligned relative to.
> > + * The @rectangle should be in @parent's coordinate space, which means it
> > + * should account for the sizes of any client-side window decorations.
> > 
> > We discussed this already, and I'm still not sure what to do here. Currently
> > the implementation does not do this, but is just relative to the "internal"
> > GdkWindow (i.e. what is inside the window decorations, if any).
> 
> So the internal GdkWindow is non-native then? For the Wayland backend, can
> we try something simple like calculating the rectangle relative to first
> native GdkWindow we encounter up the hierarchy?

Discussed thinsg related to this some on #gtk+. I wonder, wouldn't it be easiest to make it a hard requirement to have the GdkWindow to be the toplevel window (i.e. AFAIU gtk_widget_get_window (gtk_widget_get_toplevel (attach_widget)) or gdk_window_get_effective_toplevel (gtk_widget_get_window (attach_widget)) ).

We'd then make it hard requirement for the rectangle to be in the toplevel window coordinate space. This more or less means what is now in the description, i.e. the rectangle is including window decorations, shadow etc, just that the GdkWindow is also the correct one.

Everywhere we now do

gtk_widget_get_allocation (attach_widget, &rectangle);
gdk_attach_params_set_attach_rect (params, &rectagle,
                                   gtk_widget_get_window (attach_widget);

we have to do

GtkWidget *toplevel;

toplevel = gtk_widget_get_toplevel (attach_widget);
gtk_widget_get_allocation (attach_widget, &rectangle);
gtk_widget_translate_coordinates (attach_widget, toplevel,
                                  &rectangle.x, &rectangle.y);
gdk_attach_params_set_attach_rectangle (params, &rectangle,
                                        gtk_widget_get_window (toplevel));



When there is no attach_widget (i.e. right click menu), we'll have to fetch the toplevel GdkWindow from the GdkWindow of the event. I.e.

parent = gdk_window_get_effective_toplevel (parent);

before translating any event coordinates.

I tested this change in gtkmenu.c and it seems to work fine on both X11 and Wayland, and I think it will make it much easier to make this in Mir and Wayland, since we won't have to translate GdkWindow coordinates from one GdkWindow's coordinate space to another. Doing such translation in the backend doesn't seem possible without having to fake a lot of root window legacy stuff, which doesn't seem sustainable.

Review of attachment 320448 [details] [review]:

::: gtk/gtkmenu.c
@@ +1567,3 @@
+  if (params)
+    {
+      g_object_ref_sink (params);

Could we limit reference counting to just setting parameters of something outliving this function? Just seems risky without any benefit to keep references that will only ever be alive during the duration of this function.

@@ +1910,3 @@
+ * @type_hint: window type hint to use for the popup menu's top-level window
+ * @params: (transfer floating) (nullable): a description of how to position
+ *          @menu

Could we some how make it a requirement to either have a attach_widget or just a parent widget? I'd really like to get rid of the guess work involving figuring out on top of what a popup menu should be placed.

If we're still required to have guess work implemnted here, at least I think those should end up in g_warning ("failed because of this and that\n"); return; instead of continuing bothering the backend.

Introducing new API that doesn't mean guess work would however be much nicer IMHO.

@@ +1967,3 @@
+
+  if (seat)
+    g_object_ref (seat);

I think the reference/unreferencing (including all the g_set_object and g_clear_object of the local variables) are more confusing than anything. It'd be easier if we never referenced/unreferenced anything in this function except if the reason is that we reference with the intention of keeping the reference.

@@ +2010,3 @@
+              if (parent)
+                {
+                  gdk_window_get_origin (parent, &x, &y);

If we do as I suggested in the other comment about gdk_attach_params_set_attech_rect, and regarding references, this would be:

parent = gdk_window_get_effective_toplevel (parent);
gdk_window_get_origin (parent, &x, &y);

@@ +2026,3 @@
+      gtk_widget_realize (attach_widget);
+      gtk_widget_get_allocation (attach_widget, &rectangle);
+      gdk_attach_params_set_attach_rect (params, &rectangle, gtk_widget_get_window (attach_widget));

These three lines would, if we only deal with toplevels, as discussed elsewhere:

      GtkWidget *toplevel;

      gtk_widget_realize (attach_widget);
      gtk_widget_get_allocation (attach_widget, &rectangle);

      toplevel = gtk_widget_get_toplevel (attach_widget);
      gtk_widget_translate_coordinates (attach_widget, toplevel,
                                        0, 0,
                                        &rectangle.x, &rectangle.y);

      gdk_attach_params_set_attach_rect (params, &rectangle,
                                         gtk_widget_get_window (toplevel));

(In reply to Jonas Ådahl from comment #275)
> Review of attachment 320448 [details] [review] [review]:
> 
> ::: gtk/gtkmenu.c
> @@ +1567,3 @@
> +  if (params)
> +    {
> +      g_object_ref_sink (params);
> 
> Could we limit reference counting to just setting parameters of something
> outliving this function? Just seems risky without any benefit to keep
> references that will only ever be alive during the duration of this function.
> 
> @@ +1910,3 @@
> + * @type_hint: window type hint to use for the popup menu's top-level window
> + * @params: (transfer floating) (nullable): a description of how to position
> + *          @menu
> 
> Could we some how make it a requirement to either have a attach_widget or
> just a parent widget? I'd really like to get rid of the guess work involving
> figuring out on top of what a popup menu should be placed.
> 
> If we're still required to have guess work implemnted here, at least I think
> those should end up in g_warning ("failed because of this and that\n");
> return; instead of continuing bothering the backend.
> 
> Introducing new API that doesn't mean guess work would however be much nicer
> IMHO.
> 

Could we maybe split up gtk_menu_popup_with_params() into 2 or 3 different functions?

One helper like function passing a attach_widget and a params (both not nullable).

One helper like function passing a parent used for context menu style menus.

One helper with no attach widget and a non-null params, which would g_return_if_fail (params->rectparent != NULL).

We'd then force the callee to always pass sane input (always a parent widget, i.e. no need to guess the parent window).

 * Why do I need to pass attach params to the menu/GDK window all the time? Why aren't those normal properties that I setup when reating the menu/GDK window?

 * In particular, why does the GdkAttachParams object exist? It doesn't seem necessary to me.

 * I found functions with 14 arguments in this patchset. I don't think functions with 14 arguments are in any way understandable.

 * Who's responsible for updating the window position of a window popped up with params? Like when I pop up a combobox inside a scrolled window and then scroll, who makes sure the popup gets moved?

 * How do these anchor points interact with GdkGravity and GdkGeometry and all that mess? Could we just say that attached windows cannot be moved?

(In reply to Benjamin Otte (Company) from comment #277)
>  * Why do I need to pass attach params to the menu/GDK window all the time?
> Why aren't those normal properties that I setup when reating the menu/GDK
> window?

As with gtk_menu_popup(), the same menu might want to be positioned differently every time it pops up. As mentioned in a comment earlier one also might want to move a popup.

> 
>  * In particular, why does the GdkAttachParams object exist? It doesn't seem
> necessary to me.

GTK needs to provide GDK with a description of how the menu/popup should be positioned on the screen. In some backends like X11 GDK will use this information together with screen size and global window position to position menu accordingly. In other, such as Mir and Wayland, GDK will more or less pass this information on to the compositor.

The alternative would be to just through all of these functions on GdkWindow, but to me isolating them to an separate object seems more reasonable to me. The GdkWindow API is large enough as it is.

> 
>  * I found functions with 14 arguments in this patchset. I don't think
> functions with 14 arguments are in any way understandable.

I'm assuming you are talking about some implementation detail. Actually commenting on the patch would be much better than having others guess what you mean.

> 
>  * Who's responsible for updating the window position of a window popped up
> with params? Like when I pop up a combobox inside a scrolled window and then
> scroll, who makes sure the popup gets moved?

Are you talking about when GTK+ changes the position of its widgets? If so it should be GTK+ who moves the menu. If there are ways scrolling could work, please describe, because I'm not aware of them.

> 
>  * How do these anchor points interact with GdkGravity and GdkGeometry and
> all that mess? Could we just say that attached windows cannot be moved?

You mean interactive move or something? Because just above you described a use case where you wanted them to move. If you meant interactive move, I don't think that should be possible on such popups no.

> >  * In particular, why does the GdkAttachParams object exist? It doesn't seem
> > necessary to me.
> 
> GTK needs to provide GDK with a description of how the menu/popup should be
> positioned on the screen. In some backends like X11 GDK will use this
> information together with screen size and global window position to position
> menu accordingly. In other, such as Mir and Wayland, GDK will more or less
> pass this information on to the compositor.
> 
> The alternative would be to just through all of these functions on
> GdkWindow, but to me isolating them to an separate object seems more
> reasonable to me. The GdkWindow API is large enough as it is.

I've stubbed out an alternative API without the object just to see what it looks like. Maybe it's an acceptable compromise?:

void gtk_menu_popup_at_rect (GtkMenu            *menu,
                             GdkWindow          *rect_window,
                             const GdkRectangle *rect,
                             GdkGravity          rect_anchor,
                             const GdkEvent     *trigger_event);

void gtk_menu_popup_at_widget (GtkMenu        *menu,
                               GtkWidget      *widget,
                               GdkGravity      widget_anchor,
                               const GdkEvent *trigger_event);

void gtk_menu_popup_at_pointer (GtkMenu        *menu,
                                const GdkEvent *trigger_event);

void gdk_window_move_to_rect (GdkWindow           *window,    
                              GdkWindow           *rect_window,     
                              const GdkRectangle  *rect,
                              GdkGravity           rect_anchor,      
                              GdkGravity           window_anchor,    
                              GdkAnchorHints       anchor_hints,     
                              gint                 dx,               
                              gint                 dy,               
                              GAsyncReadyCallback  callback,
                              gpointer             user_data);       

void gdk_window_move_to_rect_finish (GdkWindow    *window,
                                     GAsyncResult *result,
                                     gint         *x,    
                                     gint         *y,    
                                     gint         *dx,   
                                     gint         *dy,   
                                     gboolean     *flipped_x,
                                     gboolean     *flipped_y);

gdk_window_move_to_rect () and gdk_window_move_to_rect_finish () are still big, but most people are going to be looking at the gtk_menu_popup_at_* () functions. Also, the menu anchor, menu offset, and anchor hints would be properties on the GtkMenu itself.

How does this look as an alternative? My only worry is that we lose the main benefit of having a separate GdkAttachParams, which is the ability to add new parameters in the future if we need them.



> >  * I found functions with 14 arguments in this patchset. I don't think
> > functions with 14 arguments are in any way understandable.
> 
> I'm assuming you are talking about some implementation detail. Actually
> commenting on the patch would be much better than having others guess what
> you mean.

Yeah. I think Benjamin's referring to gdk_attach_params_choose_position (). I'll try to cut that down, but it is private. I think we kind of have to accept that we're already passing around a lot of info... gtk_menu_popup_for_device () is already kind of hefty with 9 arguments, and that's part of *public* API.



> >  * Who's responsible for updating the window position of a window popped up
> > with params? Like when I pop up a combobox inside a scrolled window and then
> > scroll, who makes sure the popup gets moved?
> 
> Are you talking about when GTK+ changes the position of its widgets? If so
> it should be GTK+ who moves the menu. If there are ways scrolling could
> work, please describe, because I'm not aware of them.

Yeah... I'm also wondering how would this be possible. The menu should be grabbing the pointer, so you wouldn't be able to get any mouse scrolling events on the scrolled window containing the combo box.

Even if you could do this, we would never be able to move the menu in response to one of its ancestor windows moving, that sounds fairly crazy...

(In reply to William Hua from comment #279)
> > >  * In particular, why does the GdkAttachParams object exist? It doesn't seem
> > > necessary to me.
> > 
> > GTK needs to provide GDK with a description of how the menu/popup should be
> > positioned on the screen. In some backends like X11 GDK will use this
> > information together with screen size and global window position to position
> > menu accordingly. In other, such as Mir and Wayland, GDK will more or less
> > pass this information on to the compositor.
> > 
> > The alternative would be to just through all of these functions on
> > GdkWindow, but to me isolating them to an separate object seems more
> > reasonable to me. The GdkWindow API is large enough as it is.
> 
> I've stubbed out an alternative API without the object just to see what it
> looks like. Maybe it's an acceptable compromise?:
> 
> void gtk_menu_popup_at_rect (GtkMenu            *menu,
>                              GdkWindow          *rect_window,
>                              const GdkRectangle *rect,
>                              GdkGravity          rect_anchor,
>                              const GdkEvent     *trigger_event);
> 
> void gtk_menu_popup_at_widget (GtkMenu        *menu,
>                                GtkWidget      *widget,
>                                GdkGravity      widget_anchor,
>                                const GdkEvent *trigger_event);
> 
> void gtk_menu_popup_at_pointer (GtkMenu        *menu,
>                                 const GdkEvent *trigger_event);
> 
> void gdk_window_move_to_rect (GdkWindow           *window,    
>                               GdkWindow           *rect_window,     
>                               const GdkRectangle  *rect,
>                               GdkGravity           rect_anchor,      
>                               GdkGravity           window_anchor,    
>                               GdkAnchorHints       anchor_hints,     
>                               gint                 dx,               
>                               gint                 dy,               
>                               GAsyncReadyCallback  callback,
>                               gpointer             user_data);       
> 
> void gdk_window_move_to_rect_finish (GdkWindow    *window,
>                                      GAsyncResult *result,
>                                      gint         *x,    
>                                      gint         *y,    
>                                      gint         *dx,   
>                                      gint         *dy,   
>                                      gboolean     *flipped_x,
>                                      gboolean     *flipped_y);
> 
> gdk_window_move_to_rect () and gdk_window_move_to_rect_finish () are still
> big, but most people are going to be looking at the gtk_menu_popup_at_* ()
> functions. Also, the menu anchor, menu offset, and anchor hints would be
> properties on the GtkMenu itself.
> 
> How does this look as an alternative? My only worry is that we lose the main
> benefit of having a separate GdkAttachParams, which is the ability to add
> new parameters in the future if we need them.
> 

I don't like making GdkAttachParams huge gdk_window_ functions. They are huge and it kills any extensibility that doesn't involve deprecating old functions while introducing new larger ones.

Hiding GdkAttachParams from GtkMenu users is somewhat similar what I proposed a couple of comments above, i.e. split up gtk_menu_popup_with_params() similarly to what you wrote above.

> 
> 
> > >  * I found functions with 14 arguments in this patchset. I don't think
> > > functions with 14 arguments are in any way understandable.
> > 
> > I'm assuming you are talking about some implementation detail. Actually
> > commenting on the patch would be much better than having others guess what
> > you mean.
> 
> Yeah. I think Benjamin's referring to gdk_attach_params_choose_position ().
> I'll try to cut that down, but it is private. I think we kind of have to
> accept that we're already passing around a lot of info...
> gtk_menu_popup_for_device () is already kind of hefty with 9 arguments, and
> that's part of *public* API.
> 
> 
> 
> > >  * Who's responsible for updating the window position of a window popped up
> > > with params? Like when I pop up a combobox inside a scrolled window and then
> > > scroll, who makes sure the popup gets moved?
> > 
> > Are you talking about when GTK+ changes the position of its widgets? If so
> > it should be GTK+ who moves the menu. If there are ways scrolling could
> > work, please describe, because I'm not aware of them.
> 
> Yeah... I'm also wondering how would this be possible. The menu should be
> grabbing the pointer, so you wouldn't be able to get any mouse scrolling
> events on the scrolled window containing the combo box.

Not all menus (or, well popups that'll be placed using GdkAttachParams) grabs the pointer, and the client could also potentially scroll some other way that doesn't involve user interaction.

> 
> Even if you could do this, we would never be able to move the menu in
> response to one of its ancestor windows moving, that sounds fairly crazy...

Yea, at least if it would to mean responding to server side outside-of-monitor re-positioning. It'd be involve complex synhcronization mechanism (notify server of new popup attach params, receive new popup position, update parent content, update child content (atomically)) for very rare use cases.

It'd probably be best to explicitly not promise anything about synchronousy regarding moving and if it is really needed add that later.

(In reply to Jonas Ådahl from comment #278)
> As with gtk_menu_popup(), the same menu might want to be positioned
> differently every time it pops up. As mentioned in a comment earlier one
> also might want to move a popup.
> 
All of these things would work fine, you'd just gtk_menu_set_attach_params()/gdk_window_set_attach_params() the new params and the menu would reposition itself. You wouldn't even need to pop it down first.

But more importantly: This seems to be a rare case. In most cases the patchset uses the same attach params every time. So setting them once when creating the widget - ideally in the ui file - seems way better to me.

> The alternative would be to just through all of these functions on
> GdkWindow, but to me isolating them to an separate object seems more
> reasonable to me. The GdkWindow API is large enough as it is.
> 
Yeah, this is what I'm asking about in essence. GDK/GTK has historically not used objects that are property bags and this would be an exception. So I'm mostly wondering why this exception is necessary.

> I'm assuming you are talking about some implementation detail. Actually
> commenting on the patch would be much better than having others guess what
> you mean.
>
The whole patchset has the problem that it uses functions with way too many arguments. gdk_attach_params_choose_position() is just the worst example. It could be made to take 4 arguments: params, screen_rect, attach_rect and return the position_rect.

And this gets really bad because it spills into the public API: gtk_menu_popup_with_params() has 9 arguments. I bet nobody could name them off the top of their head in the right order.

This is btw another reason why I want to make attach params a property that you set up in advance. If you do that, you save an argument to the popup function at no cost.
 
> >  * Who's responsible for updating the window position of a window popped up
> > with params? Like when I pop up a combobox inside a scrolled window and then
> > scroll, who makes sure the popup gets moved?
> 
> Are you talking about when GTK+ changes the position of its widgets? If so
> it should be GTK+ who moves the menu. If there are ways scrolling could
> work, please describe, because I'm not aware of them.
> 
I was thinking mostly about not menus but regular subwindows here, but I guess the problem also applies to menus:
If a combobox inside a scrolled window is being scrolled, all that happens is that the scrollable calls gdk_window_scroll() on a non-native window (the viewport's bin window I think). Neither the combobox not the native windows get any information about this afaik so they can't reposition themselves.

And the patchset ignores all non-native windows inside GDK so it wouldn't update the attach params.

> You mean interactive move or something? Because just above you described a
> use case where you wanted them to move. If you meant interactive move, I
> don't think that should be possible on such popups no.
>
Code - like the one in gtkwindow.c - that tries to gdk_window_move() its widget in various situations might need to be aware that it's dealing with a self-positioning window so it doesn't try to gdk_window_move() it in the first place.

(In reply to William Hua from comment #279)
> void gdk_window_move_to_rect (GdkWindow           *window,    
>                               GdkWindow           *rect_window,     
>                               const GdkRectangle  *rect,
>                               GdkGravity           rect_anchor,      
>                               GdkGravity           window_anchor,    
>                               GdkAnchorHints       anchor_hints,     
>                               gint                 dx,               
>                               gint                 dy,               
>                               GAsyncReadyCallback  callback,
>                               gpointer             user_data);       
> 
I'd expect rect_anchor, window_anchor, anchor_hints and rect_window to be properties on the GdkWindow that were setup in advance. That would leave just the rect, dx/dy and the async callback.

> void gdk_window_move_to_rect_finish (GdkWindow    *window,
>                                      GAsyncResult *result,
>                                      gint         *x,    
>                                      gint         *y,    
>                                      gint         *dx,   
>                                      gint         *dy,   
>                                      gboolean     *flipped_x,
>                                      gboolean     *flipped_y);
> 
On a similar note to making GdkAttachParams a custom object: Should these be some kind of object - say GdkAttachInfo - instead of 6 numbers?
Also, should the window store them for later retrieval instead of them just being passed in a callback?

> How does this look as an alternative? My only worry is that we lose the main
> benefit of having a separate GdkAttachParams, which is the ability to add
> new parameters in the future if we need them.
>

 
> Yeah... I'm also wondering how would this be possible. The menu should be
> grabbing the pointer, so you wouldn't be able to get any mouse scrolling
> events on the scrolled window containing the combo box.
> 
The easiest way to trigger things like these for testing is with CSS animations in the GTK inspector. Try this:

@keyframes woohoo {
  50% { padding: 5px; }
}

window > * {
  animation: woohoo 5s linear infinite;
}

The real use case I'm wondering about here usually comes from updates to the UI while a menu or popup is open.
Say you have some menu open on some row in your inbox and new email arrives. This might cause the row to move while the menu is open.

> > The alternative would be to just through all of these functions on
> > GdkWindow, but to me isolating them to an separate object seems more
> > reasonable to me. The GdkWindow API is large enough as it is.
> > 
> Yeah, this is what I'm asking about in essence. GDK/GTK has historically not
> used objects that are property bags and this would be an exception. So I'm
> mostly wondering why this exception is necessary.

I can understand putting something like the menu anchor point or menu offset as a property of the menu, which makes a lot of sense, but something about setting the rect, rect_window, rect_anchor as stateful public properties feels weird to me. The gtk_menu_popup_with_params () function (or gtk_menu_popup_at_* for the stub above) is really just a one-shot stateless (re-)positioning of the menu.

Passing a property bag helps us a bit too in case we need to provide extra parameters, for example, if we plan to re-use this API for popovers.



> The whole patchset has the problem that it uses functions with way too many
> arguments. gdk_attach_params_choose_position() is just the worst example. It
> could be made to take 4 arguments: params, screen_rect, attach_rect and
> return the position_rect.
> 
> And this gets really bad because it spills into the public API:
> gtk_menu_popup_with_params() has 9 arguments. I bet nobody could name them
> off the top of their head in the right order.

FWIW, gtk_menu_popup_for_device () also has 9 args, so I think that's just the nature of the problem at hand ;)

And also, I'm thinking that instead of passing a GdkDevice, button and event time, we should really be passing in a triggering GdkEvent instead, which saves us two args, bringing us down to 7.



> > >  * Who's responsible for updating the window position of a window popped up
> > > with params? Like when I pop up a combobox inside a scrolled window and then
> > > scroll, who makes sure the popup gets moved?
> > 
> > Are you talking about when GTK+ changes the position of its widgets? If so
> > it should be GTK+ who moves the menu. If there are ways scrolling could
> > work, please describe, because I'm not aware of them.
> > 
> I was thinking mostly about not menus but regular subwindows here, but I
> guess the problem also applies to menus:
> If a combobox inside a scrolled window is being scrolled, all that happens
> is that the scrollable calls gdk_window_scroll() on a non-native window (the
> viewport's bin window I think). Neither the combobox not the native windows
> get any information about this afaik so they can't reposition themselves.
> 
> And the patchset ignores all non-native windows inside GDK so it wouldn't
> update the attach params.

I might be misunderstanding here, but basically the position is re-computed every time the menu is popped up.

If you mean re-computing the position of the window whenever an ancestor window moves, I think we have to give up on this dynamic motion... it rarely happens and sounds extremely difficult to implement.



> > You mean interactive move or something? Because just above you described a
> > use case where you wanted them to move. If you meant interactive move, I
> > don't think that should be possible on such popups no.
> >
> Code - like the one in gtkwindow.c - that tries to gdk_window_move() its
> widget in various situations might need to be aware that it's dealing with a
> self-positioning window so it doesn't try to gdk_window_move() it in the
> first place.

This is true, but from working on the branch, I've found that the menu's toplevel doesn't really move after mapping. I'm sure we can find some implementation-specific ways to fix it if we encounter such behaviour.



(In reply to Benjamin Otte (Company) from comment #282)
> (In reply to William Hua from comment #279)
> > void gdk_window_move_to_rect (GdkWindow           *window,    
> >                               GdkWindow           *rect_window,     
> >                               const GdkRectangle  *rect,
> >                               GdkGravity           rect_anchor,      
> >                               GdkGravity           window_anchor,    
> >                               GdkAnchorHints       anchor_hints,     
> >                               gint                 dx,               
> >                               gint                 dy,               
> >                               GAsyncReadyCallback  callback,
> >                               gpointer             user_data);       
> > 
> I'd expect rect_anchor, window_anchor, anchor_hints and rect_window to be
> properties on the GdkWindow that were setup in advance. That would leave
> just the rect, dx/dy and the async callback.

I'm thinking window_anchor, anchor_hints, dx, dy could definitely be properties on the window. The rect* and callback parameters seem weird to 'property-ify' for the reason above.



> > void gdk_window_move_to_rect_finish (GdkWindow    *window,
> >                                      GAsyncResult *result,
> >                                      gint         *x,    
> >                                      gint         *y,    
> >                                      gint         *dx,   
> >                                      gint         *dy,   
> >                                      gboolean     *flipped_x,
> >                                      gboolean     *flipped_y);
> > 
> On a similar note to making GdkAttachParams a custom object: Should these be
> some kind of object - say GdkAttachInfo - instead of 6 numbers?
> Also, should the window store them for later retrieval instead of them just
> being passed in a callback?

Sounds like another property bag ;)



> > Yeah... I'm also wondering how would this be possible. The menu should be
> > grabbing the pointer, so you wouldn't be able to get any mouse scrolling
> > events on the scrolled window containing the combo box.
> > 
> The easiest way to trigger things like these for testing is with CSS
> animations in the GTK inspector. Try this:
> 
> @keyframes woohoo {
>   50% { padding: 5px; }
> }
> 
> window > * {
>   animation: woohoo 5s linear infinite;
> }
> 
> The real use case I'm wondering about here usually comes from updates to the
> UI while a menu or popup is open.
> Say you have some menu open on some row in your inbox and new email arrives.
> This might cause the row to move while the menu is open.

Yeah, I think we need to give up on this idea. If I right-click on something to pop up a context-sensitive menu, and that thing moves, I'm not expecting the menu to move with it generally.

> > > >  * Who's responsible for updating the window position of a window popped up
> > > > with params? Like when I pop up a combobox inside a scrolled window and then
> > > > scroll, who makes sure the popup gets moved?
> > > 
> > > Are you talking about when GTK+ changes the position of its widgets? If so
> > > it should be GTK+ who moves the menu. If there are ways scrolling could
> > > work, please describe, because I'm not aware of them.
> > > 
> > I was thinking mostly about not menus but regular subwindows here, but I
> > guess the problem also applies to menus:
> > If a combobox inside a scrolled window is being scrolled, all that happens
> > is that the scrollable calls gdk_window_scroll() on a non-native window (the
> > viewport's bin window I think). Neither the combobox not the native windows
> > get any information about this afaik so they can't reposition themselves.
> > 
> > And the patchset ignores all non-native windows inside GDK so it wouldn't
> > update the attach params.
> 
> I might be misunderstanding here, but basically the position is re-computed
> every time the menu is popped up.
> 
> If you mean re-computing the position of the window whenever an ancestor
> window moves, I think we have to give up on this dynamic motion... it rarely
> happens and sounds extremely difficult to implement.

And... I think I just understood what you meant.

I haven't tested for that particular situation, but I have a small patch that transforms the rect to the coordinate space of the closest native ancestor window. It may or may not work, but it seems like it's not too hard to fix anyways.

(In reply to William Hua from comment #283)
> Passing a property bag helps us a bit too in case we need to provide extra
> parameters, for example, if we plan to re-use this API for popovers.
>
We are kind of already using this API for popovers as it's part of GdkWindow. So I expected it to target for toplevels, menus, popovers, popups and tooltips.
 
> FWIW, gtk_menu_popup_for_device () also has 9 args, so I think that's just
> the nature of the problem at hand ;)
> 
I don't think so. Most people pass lots of NULLs to gtk_menu_popup_for_device() which is a clear sign that either the callers don't know what they're doing or that the API is shit. I'm very strongly leaning towards the API being shit.

> If you mean re-computing the position of the window whenever an ancestor
> window moves, I think we have to give up on this dynamic motion... it rarely
> happens and sounds extremely difficult to implement.
>
GTK is moving towards being dynamic more and more. Introducing APIs that won't deal with dynamic content in 2016 seems very backwards.
 
> I'm thinking window_anchor, anchor_hints, dx, dy could definitely be
> properties on the window. The rect* and callback parameters seem weird to
> 'property-ify' for the reason above.
>
I agree on the callback parameter (though I'd replace the callback with a signal).

I initially agreed on the rect. But I don't do after thinking about it. The rect is responsible for where the window is positioned so it is relevant information and should be queryable at least until the window is hidden. Which very much means it should be a property.

> Sounds like another property bag ;)
> 
Yes.
Or a bunch of read-only properties on the window, depending on what approach you take. ;)

> Yeah, I think we need to give up on this idea. If I right-click on something
> to pop up a context-sensitive menu, and that thing moves, I'm not expecting
> the menu to move with it generally.

On the other hand, if you click a menu in the menubar and the menubar moves, you'd expect the menu to move with it...

> > If you mean re-computing the position of the window whenever an ancestor
> > window moves, I think we have to give up on this dynamic motion... it rarely
> > happens and sounds extremely difficult to implement.
> >
> GTK is moving towards being dynamic more and more. Introducing APIs that
> won't deal with dynamic content in 2016 seems very backwards.

I can't comment on how hard it would be for Wayland/Mir backends. But for X11, wouldn't we need some heavy machinery to watch for position changes of every window up the hierarchy?



> > I'm thinking window_anchor, anchor_hints, dx, dy could definitely be
> > properties on the window. The rect* and callback parameters seem weird to
> > 'property-ify' for the reason above.
> >
> I agree on the callback parameter (though I'd replace the callback with a
> signal).
> 
> I initially agreed on the rect. But I don't do after thinking about it. The
> rect is responsible for where the window is positioned so it is relevant
> information and should be queryable at least until the window is hidden.
> Which very much means it should be a property.

Ok, that rationale is pretty sensible. Just wondering how Jonas feels about the idea. If pretty much everything's a property on the menu/window, we don't need GdkAttachParams any more since we can always add new properties on the menu/window later.

Any reason for switching to a signal on the GdkWindow? I only felt a callback was better because it felt like an action to move the window, followed by waiting for the results (final position) of that motion.



> > Sounds like another property bag ;)
> > 
> Yes.
> Or a bunch of read-only properties on the window, depending on what approach
> you take. ;)

Do you think there might be a problem here since we'd be setting properties on it async? /me is worried



> > Yeah, I think we need to give up on this idea. If I right-click on something
> > to pop up a context-sensitive menu, and that thing moves, I'm not expecting
> > the menu to move with it generally.
> 
> On the other hand, if you click a menu in the menubar and the menubar moves,
> you'd expect the menu to move with it...

Yes, that's a good point, I didn't think of it... but we still have to figure out *how* we can technically do it. I don't have any good ideas besides watching for configure events on all the windows in the hierarchy.



Anyways, I guess this is what the API would look like if we moved everything into properties on the menu/window instead:

void gtk_menu_popup_at_rect (GtkMenu            *menu,
                             const GdkEvent     *trigger_event);

void gtk_menu_popup_at_widget (GtkMenu        *menu,
                               GtkWidget      *widget,
                               const GdkEvent *trigger_event);

void gtk_menu_popup_at_pointer (GtkMenu        *menu,
                                const GdkEvent *trigger_event);

void gdk_window_move_to_rect (GdkWindow           *window,
                              GAsyncReadyCallback  callback,
                              gpointer             user_data);

void gdk_window_move_to_rect_finish (GdkWindow    *window,
                                     GAsyncResult *result);
                                     gint         *x,
                                     gint         *y,
                                     gint         *dx,
                                     gint         *dy,
                                     gboolean     *flipped_x,
                                     gboolean     *flipped_y);

GtkMenu:rect-window
GtkMenu:rect
GtkMenu:rect-anchor
GtkMenu:menu-anchor
GtkMenu:anchor-hints
GtkMenu:flip-if-rtl
GtkMenu:window-type-hint
GtkMenu:dx
GtkMenu:dy

GdkWindow:rect-window
GdkWindow:rect
GdkWindow:rect-anchor
GdkWindow:window-anchor
GdkWindow:anchor-hints
GdkWindow:dx
GdkWindow:dy

> void gtk_menu_popup_at_rect (GtkMenu            *menu,
>                              const GdkEvent     *trigger_event);
> 
> void gtk_menu_popup_at_widget (GtkMenu        *menu,
>                                GtkWidget      *widget,
>                                const GdkEvent *trigger_event);
> 
> void gtk_menu_popup_at_pointer (GtkMenu        *menu,
>                                 const GdkEvent *trigger_event);

I'm staring at this and thinking we probably wouldn't need three variants of gtk_menu_popup_at_*. Anyways, I don't want to make any decisions here without Jonas' input.

(In reply to Benjamin Otte (Company) from comment #282)
> (In reply to William Hua from comment #279)
> > void gdk_window_move_to_rect (GdkWindow           *window,    
> >                               GdkWindow           *rect_window,     
> >                               const GdkRectangle  *rect,
> >                               GdkGravity           rect_anchor,      
> >                               GdkGravity           window_anchor,    
> >                               GdkAnchorHints       anchor_hints,     
> >                               gint                 dx,               
> >                               gint                 dy,               
> >                               GAsyncReadyCallback  callback,
> >                               gpointer             user_data);       
> > 
> I'd expect rect_anchor, window_anchor, anchor_hints and rect_window to be
> properties on the GdkWindow that were setup in advance. That would leave
> just the rect, dx/dy and the async callback.

How is adding more API to GdkWindow better? I don't see the benefit. The benefit of doing it via a GdkAttachParams is that the different interconnected parameters are more easily discovered and won't be mixed up with gdk_window_move(x,y) which is incompatible with using params.

Just because adding everything to huge objects just because that's how it has been done traditionally doesn't seem like a good reason.

> 
> > void gdk_window_move_to_rect_finish (GdkWindow    *window,
> >                                      GAsyncResult *result,
> >                                      gint         *x,    
> >                                      gint         *y,    
> >                                      gint         *dx,   
> >                                      gint         *dy,   
> >                                      gboolean     *flipped_x,
> >                                      gboolean     *flipped_y);
> > 
> On a similar note to making GdkAttachParams a custom object: Should these be
> some kind of object - say GdkAttachInfo - instead of 6 numbers?
> Also, should the window store them for later retrieval instead of them just
> being passed in a callback?

GdkAttachInfo seems like a good idea to me.

> 
> > How does this look as an alternative? My only worry is that we lose the main
> > benefit of having a separate GdkAttachParams, which is the ability to add
> > new parameters in the future if we need them.
> >
> 


(In reply to Benjamin Otte (Company) from comment #285)
> (In reply to William Hua from comment #283)
> > Passing a property bag helps us a bit too in case we need to provide extra
> > parameters, for example, if we plan to re-use this API for popovers.
> >
> We are kind of already using this API for popovers as it's part of
> GdkWindow. So I expected it to target for toplevels, menus, popovers, popups
> and tooltips.

GdkAttachParams should not be used to position toplevels. Only windows that are subwindows to some other window, such as menus, popovers, popups and tooltips. Toplevels will need a completely different strategy for screen-global positioning, as explicit global positioning will not be supported some backends.

>  
> > FWIW, gtk_menu_popup_for_device () also has 9 args, so I think that's just
> > the nature of the problem at hand ;)
> > 
> I don't think so. Most people pass lots of NULLs to
> gtk_menu_popup_for_device() which is a clear sign that either the callers
> don't know what they're doing or that the API is shit. I'm very strongly
> leaning towards the API being shit.
> 
> > If you mean re-computing the position of the window whenever an ancestor
> > window moves, I think we have to give up on this dynamic motion... it rarely
> > happens and sounds extremely difficult to implement.
> >
> GTK is moving towards being dynamic more and more. Introducing APIs that
> won't deal with dynamic content in 2016 seems very backwards.

I agree that it should be considered. What I don't fully grasp is whether moving a non-native GdkWindow can affect the content on native GdkWindow's buffer, how to deal with that, especially since it'd be hard to know whether a simple "move_resize" is an actual move, or a shift caused by some non-native window being moved.

Is there any reason to still move around GdkWindow's to update the content on the native GdkWindow buffer?

For when GTK+ updates the content, it should be as easy to set a new attach params or update the current attach params on the window. That would, however, ignore all kind of parent-child synchronization (i.e. the parent content and the child position and content will not be synchronized). That will eventually need to be fixed, but I'm not sure it should be in scope for an initial version.

(In reply to William Hua from comment #286)
> > > If you mean re-computing the position of the window whenever an ancestor
> > > window moves, I think we have to give up on this dynamic motion... it rarely
> > > happens and sounds extremely difficult to implement.
> > >
> > GTK is moving towards being dynamic more and more. Introducing APIs that
> > won't deal with dynamic content in 2016 seems very backwards.
> 
> I can't comment on how hard it would be for Wayland/Mir backends. But for
> X11, wouldn't we need some heavy machinery to watch for position changes of
> every window up the hierarchy?
> 

Well, there are three states that needs to be updated atomically.

 1) The parent window's content
 2) The child window's position
 3) The child window's content

1) and 3) may depend on 2), which in that case involves a roundtrip to the display server, which could cause lag. To make things not lag, we'd have to special-case offsetting the child window so that the future position is predictable (i.e. if child windows wouldn't move within the screen if overflowed). If lag is acceptible GTK+ would need to wait for the roundtrip to update its content. For both cases we'd need to add inter-surface synchronized buffer updating which is not available for surfaces like menus right now, but could be added.

> 
> 
> > > I'm thinking window_anchor, anchor_hints, dx, dy could definitely be
> > > properties on the window. The rect* and callback parameters seem weird to
> > > 'property-ify' for the reason above.
> > >
> > I agree on the callback parameter (though I'd replace the callback with a
> > signal).
> > 
> > I initially agreed on the rect. But I don't do after thinking about it. The
> > rect is responsible for where the window is positioned so it is relevant
> > information and should be queryable at least until the window is hidden.
> > Which very much means it should be a property.
> 
> Ok, that rationale is pretty sensible. Just wondering how Jonas feels about
> the idea. If pretty much everything's a property on the menu/window, we
> don't need GdkAttachParams any more since we can always add new properties
> on the menu/window later.

I don't see the reason for shoving everything into GdkWindow except "it's the way its been done". I do see benefit of using a separate GdkAttachParams (while trying to limit its exposure in GTK API). It also feels the code in GDK will be harder to follow if the same isolation is not implemented internally in every backend.

If everything is shoved into GdkWindow, then I see no point of having a GdkAttachParams though.

> 
> Any reason for switching to a signal on the GdkWindow? I only felt a
> callback was better because it felt like an action to move the window,
> followed by waiting for the results (final position) of that motion.

I guess less memory to manage by GDK (the callback and associated data). I don't care whether a signal is used or a callback.

> 
> 
> 
> > > Sounds like another property bag ;)
> > > 
> > Yes.
> > Or a bunch of read-only properties on the window, depending on what approach
> > you take. ;)
> 
> Do you think there might be a problem here since we'd be setting properties
> on it async? /me is worried
> 
> 
> 

(In reply to William Hua from comment #287)
> > void gtk_menu_popup_at_rect (GtkMenu            *menu,
> >                              const GdkEvent     *trigger_event);

As I assume "trigger_event" may be NULL, this will need a parent widget/window of some kind to avoid having the backend guestimate it.

> > 
> > void gtk_menu_popup_at_widget (GtkMenu        *menu,
> >                                GtkWidget      *widget,
> >                                const GdkEvent *trigger_event);
> > 
> > void gtk_menu_popup_at_pointer (GtkMenu        *menu,
> >                                 const GdkEvent *trigger_event);
> 
> I'm staring at this and thinking we probably wouldn't need three variants of
> gtk_menu_popup_at_*. Anyways, I don't want to make any decisions here
> without Jonas' input.

I'd prefer having per-use-case gtk_menu_popup_* functions that doesn't make it possible to ever have the backend guess things. If not these three, what is it that you are suggesting?

*snip*

Just want to address a couple of small things quickly (going to skip the dynamic real-time positioning because I have no solid ideas for it):



> (In reply to William Hua from comment #287)
> > > void gtk_menu_popup_at_rect (GtkMenu            *menu,
> > >                              const GdkEvent     *trigger_event);
> 
> As I assume "trigger_event" may be NULL, this will need a parent
> widget/window of some kind to avoid having the backend guestimate it.

I think we should make trigger_event a non-nullable parameter. I can't think of a good reason to pop up a menu without any sort of user impetus, menus seem to only pop up on button click, or key press (Shift+F10 or the menu key), or pointer motion (when the user hovers over a menu item that pops open its submenu).

Have I missed something? Is there another reason an application would want to pop up a menu that is not in response to some user interaction?



> > > void gtk_menu_popup_at_widget (GtkMenu        *menu,
> > >                                GtkWidget      *widget,
> > >                                const GdkEvent *trigger_event);
> > > 
> > > void gtk_menu_popup_at_pointer (GtkMenu        *menu,
> > >                                 const GdkEvent *trigger_event);
> > 
> > I'm staring at this and thinking we probably wouldn't need three variants of
> > gtk_menu_popup_at_*. Anyways, I don't want to make any decisions here
> > without Jonas' input.
> 
> I'd prefer having per-use-case gtk_menu_popup_* functions that doesn't make
> it possible to ever have the backend guess things. If not these three, what
> is it that you are suggesting?

Yeah, you're right, I've changed my mind on this. I made that remark because I thought that we could use just one function and infer the caller's intention just from what parameters they set, but that's a bad idea.

+1 for per-use-case popup functions.



(In reply to Jonas Ådahl from comment #288)
> (In reply to Benjamin Otte (Company) from comment #282)
> > (In reply to William Hua from comment #279)
> > > void gdk_window_move_to_rect (GdkWindow           *window,    
> > >                               GdkWindow           *rect_window,     
> > >                               const GdkRectangle  *rect,
> > >                               GdkGravity           rect_anchor,      
> > >                               GdkGravity           window_anchor,    
> > >                               GdkAnchorHints       anchor_hints,     
> > >                               gint                 dx,               
> > >                               gint                 dy,               
> > >                               GAsyncReadyCallback  callback,
> > >                               gpointer             user_data);       
> > > 
> > I'd expect rect_anchor, window_anchor, anchor_hints and rect_window to be
> > properties on the GdkWindow that were setup in advance. That would leave
> > just the rect, dx/dy and the async callback.
> 
> How is adding more API to GdkWindow better? I don't see the benefit. The
> benefit of doing it via a GdkAttachParams is that the different
> interconnected parameters are more easily discovered and won't be mixed up
> with gdk_window_move(x,y) which is incompatible with using params.
> 
> Just because adding everything to huge objects just because that's how it
> has been done traditionally doesn't seem like a good reason.

*snip*

> I don't see the reason for shoving everything into GdkWindow except "it's
> the way its been done". I do see benefit of using a separate GdkAttachParams
> (while trying to limit its exposure in GTK API). It also feels the code in
> GDK will be harder to follow if the same isolation is not implemented
> internally in every backend.
> 
> If everything is shoved into GdkWindow, then I see no point of having a
> GdkAttachParams though.

Yeah, these seem to be pretty compelling arguments for having GdkAttachParams. We want to isolate the usage of the parameters just to our specialized move function.

(In reply to William Hua from comment #289)
> *snip*
> 
> Just want to address a couple of small things quickly (going to skip the
> dynamic real-time positioning because I have no solid ideas for it):
> 
> 
> 
> > (In reply to William Hua from comment #287)
> > > > void gtk_menu_popup_at_rect (GtkMenu            *menu,
> > > >                              const GdkEvent     *trigger_event);
> > 
> > As I assume "trigger_event" may be NULL, this will need a parent
> > widget/window of some kind to avoid having the backend guestimate it.
> 
> I think we should make trigger_event a non-nullable parameter. I can't think
> of a good reason to pop up a menu without any sort of user impetus, menus
> seem to only pop up on button click, or key press (Shift+F10 or the menu
> key), or pointer motion (when the user hovers over a menu item that pops
> open its submenu).
> 
> Have I missed something? Is there another reason an application would want
> to pop up a menu that is not in response to some user interaction?
> 

Yea I suppose that makes sense. A tooltip which may be mapped after a timeout wouldn't have a GdkEvent to pass, but a tooltip won't use this API anyway I assume so maybe that is fine.

>I think we should make trigger_event a non-nullable parameter. I can't think of 
> a good reason to pop up a menu without any sort of user impetus, menus seem to 
> only pop up on button click, or key press (Shift+F10 or the menu key), or 
> pointer motion (when the user hovers over a menu item that pops open its 
> submenu).

Not true. E.g. in gnome-terminal, when we get an event which leads us to show the context menu, we defer showing the menu, and instead request to get the clipboard targets. Only when the async(!) response to that request comes in, we build and show the context menu.

It's also not true for accessibility or generally code that was triggered via IPC. And it's not true for code that works through signal handlers that don't pass the event - including prominent ones like GtkButton::clicked but also code paths that have call chains that go through external libraries like GStreamer or WebKit.

But then, neither opening a window nor popping up a menu has conceptually anything to do with GDK events, so it shouldn't be mandatory to have one.

In fact, aren't events just necessary because of grabs and the new Wayland APIs were supposed to save use from the need of taking grabs? Because if that was the case, we should make sure to not expose that requirement at all in public APIs.

(In reply to Benjamin Otte (Company) from comment #292)
> It's also not true for accessibility or generally code that was triggered
> via IPC. And it's not true for code that works through signal handlers that
> don't pass the event - including prominent ones like GtkButton::clicked but
> also code paths that have call chains that go through external libraries
> like GStreamer or WebKit.
> 
> But then, neither opening a window nor popping up a menu has conceptually
> anything to do with GDK events, so it shouldn't be mandatory to have one.
> 
> In fact, aren't events just necessary because of grabs and the new Wayland
> APIs were supposed to save use from the need of taking grabs? Because if
> that was the case, we should make sure to not expose that requirement at all
> in public APIs.

There are two issues with not passing something like an event:

1. The backend will have to guess what Wayland input event to use when creating the popup. In xdg_shell, mapping a popup requires a serial from such an input event; if we don't have an event, we'll have to take whatever event that is most recent on a given seat. This is probably not a problem, since the server will grab all devices anyway, and the application can deal with the other events and unmap the popup accordingly.

2. The backend will have to guess what seat the grab should happen on, which is more problematic since it breaks multi seat. This might not be a problem however, if the rely can on and require that gdk_seat_grab() on a given popup window can only be done for one seat per window. The API allows for any number of seats to grab the same window however, and that is not possible to implement given how popup grabs are designed to function on Wayland. In other words, it'll work only if we limit the grab API to not allow more than one grab per window.

If we don't care about actually mapping in response to a particular event, and require that a menu window can only be grabbed on a single seat, we can drop anything related to GdkEvent from the gtk_menu_popup_ API, and rely only on the single grabbed seat to know whether the popup should grab input or not, and whatever input event happened last on that seat for a serial.

FWIW, for example GtkButton::clicked is broken in multi-seat configurations as it carries no information about what caused the click. Someone, somewhere, will have to pick an arbitrary seat, or we define "GtkButton::clicked" to only function on some particular seat. "GtkButton::clicked" should probably be deprecated because of this.

As a side note, the current design of xdg_shell also makes it impossible to popup grabbing menus without actual user interaction passed by the compositor. In other words, popping up a menu from IPC without there being user interaction such as pointer click, key press or touch on the given surface since last time it received focus, will not work. But that issue is unrelated to this bug.

Why do we need a grab? This should be easily implementable without the need for grabs, especially if you have control over the protocol, no?

(In reply to Benjamin Otte (Company) from comment #294)
> Why do we need a grab? This should be easily implementable without the need
> for grabs, especially if you have control over the protocol, no?

The compositor needs to be notified if it should grab the popup or not so that it can dismiss it when clicking outside any of the clients window. We cannot rely on the client to deal with this, because clients should not be able to block dismissing of popups. 

For a non-grabbing popup, clicking outside should not affect anything. It should just result in pointer button events and window raising on wherever was clicked.

But the compositor just needs a flag on the window for that and not a full grab.

And then instead of telling the window "You're no longer the active Window" it says "You've been dismissed" when someone does an outside-click.

(In reply to Benjamin Otte (Company) from comment #296)
> But the compositor just needs a flag on the window for that and not a full
> grab.
> 
> And then instead of telling the window "You're no longer the active Window"
> it says "You've been dismissed" when someone does an outside-click.

It should only be dismissed if the pointer/touch of the same seat that opened the popup touches/clicks outside.

If you have two seats. Seat A opens a menu and intends to select something. The other seat B opens another menu, then clicks outside to dismiss it. Only the menu opened by seat B should be dismissed.

The grab really isn't much more than a per-seat "flag" that does what you say, i.e. clicking outside dismisses it and the "click" doesn't go to the other clients window.

I am not sure that is a relevant or even desired use case.

In particular I am not sure that GTK could or would want to handle the case where a menu bar is supposed to open 2 menus - one for each seat.
I'm also not sure who's gonna ensure that everything is fine if seat 1 opens a submenu and then seat 2 does something that causes the owner of seat 1's menu to get hidden.
And do we have an idea what happens if 2 seats open the same menu? Do they get to share it or would we pop up one menu for each seat?

I think menus should be application-modal popups. For all devices.

(In reply to Benjamin Otte (Company) from comment #298)
> I am not sure that is a relevant or even desired use case.
> 
> In particular I am not sure that GTK could or would want to handle the case
> where a menu bar is supposed to open 2 menus - one for each seat.
> I'm also not sure who's gonna ensure that everything is fine if seat 1 opens
> a submenu and then seat 2 does something that causes the owner of seat 1's
> menu to get hidden.
> And do we have an idea what happens if 2 seats open the same menu? Do they
> get to share it or would we pop up one menu for each seat?
> 
> I think menus should be application-modal popups. For all devices.

If GTK+ don't want to support per seat popup grabs, then that'd be up to GTK+, and I can't really say whether it should be in scope or not. That doesn't change the fact that the Wayland popup protocol should still be per seat, and we need to pass both a seat and a serial there.

As for the GtkMenu API, if popup grabs are "per-application", and we don't want to grab/pass either a seat or event, either gtkmenu.c or the backend will need to estimate what seat should be used. If we gdk_seat_grab() and only allow one seat to be grabbed at a time, that'd work, but it doesn't look like the API is limited to that.

Carlos, how did you intend the GdkSeat API to work with multiple seats?

The discussion on irc indicated that we need to defer this to a face-to-face discussion at the hackfest.

(In reply to Jonas Ådahl from comment #299)
> (In reply to Benjamin Otte (Company) from comment #298)
> > I am not sure that is a relevant or even desired use case.
> > 
> > In particular I am not sure that GTK could or would want to handle the case
> > where a menu bar is supposed to open 2 menus - one for each seat.
> > I'm also not sure who's gonna ensure that everything is fine if seat 1 opens
> > a submenu and then seat 2 does something that causes the owner of seat 1's
> > menu to get hidden.
> > And do we have an idea what happens if 2 seats open the same menu? Do they
> > get to share it or would we pop up one menu for each seat?
> > 
> > I think menus should be application-modal popups. For all devices.
> 
> If GTK+ don't want to support per seat popup grabs, then that'd be up to
> GTK+, and I can't really say whether it should be in scope or not. That
> doesn't change the fact that the Wayland popup protocol should still be per
> seat, and we need to pass both a seat and a serial there.
> 
> As for the GtkMenu API, if popup grabs are "per-application", and we don't
> want to grab/pass either a seat or event, either gtkmenu.c or the backend
> will need to estimate what seat should be used. If we gdk_seat_grab() and
> only allow one seat to be grabbed at a time, that'd work, but it doesn't
> look like the API is limited to that.
> 
> Carlos, how did you intend the GdkSeat API to work with multiple seats?

Sorry, I missed this question. Currently seats grabs use internally GDK_OWNERSHIP_NONE, which means that devices unaffected by the grab (i.e. those in other seats) are still free to interact, and possibly trigger other grabs. This might strike as too liberal though.

TBH, I think some questions about how wayland is going to handle multi-seat need replying first:
- What behavior makes sense wrt the popup window and the non-grabbing seats? are non grabbing seats allowed to interact with it? create further popups with the one for the other seat as a parent?
- At a more generic "grabbing" level, how is wayland going to handle contradicting requests? eg. requests to resize a window from the same edge, an app closing an app or rendering dnd content invalid while another seat is dnd'ing, etc.

At a conceptual level in the application, multi-seat also brings some dangers we can't resolve at the toolkit level only, eg. actions from some seats rendering actions from other seat(s) invalid. And there's other things where we're just lacking in GTK+, we eg. have a single keyboard focus.

IMHO, the simplest and most effective approach is locking onto the seat performing the grab, and disallow other seats as long as the grab holds. multi-seat cooperative apps need to be carefully designed anyway, we might expose GdkGrabOwnership again for those though as a per-seat toggle.

Anyhow, if the reply to these questions come as fast as they came for MPX in X11, I'll see you in some 8 years :).

One possible thing to consider, since it seems that hangups about the GDK side of the API are a big part of what's blocking this:

We could consider introducing a GDK_PRIVATE() mechanism similar to GLIB_PRIVATE() in order to expose these APIs for use in Gtk only.  Then we wouldn't have to commit to any particular public API and we could change them as we see fit.

(In reply to Allison Ryan Lortie (desrt) from comment #302)

> We could consider introducing a GDK_PRIVATE() mechanism similar to
> GLIB_PRIVATE() in order to expose these APIs for use in Gtk only.  Then we
> wouldn't have to commit to any particular public API and we could change
> them as we see fit.

We already have it: https://git.gnome.org/browse/gtk+/tree/gdk/gdk-private.h

Created attachment 330872 [details] [review]
[PATCH 1/7] gdkwindow: store shadow sizes

Created attachment 330873 [details] [review]
[PATCH 2/7] gdkwindow: add gdk_window_move_to_rect ()

Created attachment 330874 [details] [review]
[PATCH 3/7] gtkmenu: add gtk_menu_popup_at_* ()

Created attachment 330875 [details] [review]
[PATCH 4/7] gtkmenu: position menu using gdk_window_move_to_rect ()

Created attachment 330876 [details] [review]
[PATCH 5/7] add demo for testing gtk_menu_popup_at_* ()

Created attachment 330877 [details] [review]
[PATCH 6/7] port to new gtk_menu_popup_at_* () functions

Created attachment 330878 [details] [review]
[PATCH 7/7] mir: implement gdk_window_move_to_rect ()

These patches introduce the following public API:

void gtk_menu_popup_at_rect (GtkMenu            *menu,
                             GdkWindow          *transient_for,
                             const GdkRectangle *rect,
                             GdkGravity          rect_anchor,
                             GdkGravity          menu_anchor,
                             const GdkEvent     *trigger_event)

void gtk_menu_popup_at_widget (GtkMenu        *menu,
                               GtkWidget      *widget,
                               GdkGravity      widget_anchor,
                               GdkGravity      menu_anchor,
                               const GdkEvent *trigger_event)

void gtk_menu_popup_at_pointer (GtkMenu        *menu,
                                const GdkEvent *trigger_event)

void GtkMenu::popped-up (GtkMenu            *menu,
                         const GdkRectangle *flipped_rect,
                         const GdkRectangle *slid_rect,
                         gboolean            flipped_x,
                         gboolean            flipped_y,
                         gpointer            user_data)

GdkAnchorHints    GtkMenu:anchor-hints
gint              GtkMenu:rect-anchor-dx
gint              GtkMenu:rect-anchor-dy
GdkWindowTypeHint GtkMenu:menu-type-hint

typedef enum 
{
  GDK_ANCHOR_FLIP_X   = 1 << 0,
  GDK_ANCHOR_FLIP_Y   = 1 << 1,
  GDK_ANCHOR_SLIDE_X  = 1 << 2,
  GDK_ANCHOR_SLIDE_Y  = 1 << 3,
  GDK_ANCHOR_RESIZE_X = 1 << 4,
  GDK_ANCHOR_RESIZE_Y = 1 << 5,
  GDK_ANCHOR_FLIP     = GDK_ANCHOR_FLIP_X | GDK_ANCHOR_FLIP_Y,
  GDK_ANCHOR_SLIDE    = GDK_ANCHOR_SLIDE_X | GDK_ANCHOR_SLIDE_Y,
  GDK_ANCHOR_RESIZE   = GDK_ANCHOR_RESIZE_X | GDK_ANCHOR_RESIZE_Y
} GdkAnchorHints;



GdkWindowImpl backend API:

void (* move_to_rect) (GdkWindow          *window,
                       GdkWindow          *transient_for,
                       const GdkRectangle *rect,
                       GdkGravity          rect_anchor,
                       GdkGravity          window_anchor,
                       GdkAnchorHints      anchor_hints,
                       gint                rect_anchor_dx,
                       gint                rect_anchor_dy)



Private API:

void gdk_window_move_to_rect (GdkWindow          *window,
                              GdkWindow          *transient_for,
                              const GdkRectangle *rect,
                              GdkGravity          rect_anchor,
                              GdkGravity          window_anchor,
                              GdkAnchorHints      anchor_hints,
                              gint                rect_anchor_dx,
                              gint                rect_anchor_dy)

void GdkWindow::moved-to-rect (GdkWindow          *window,
                               const GdkRectangle *flipped_rect,
                               const GdkRectangle *slid_rect,
                               gboolean            flipped_x,
                               gboolean            flipped_y,
                               gpointer            user_data)

*** Bug 752730 has been marked as a duplicate of this bug. ***

Review of attachment 330873 [details] [review]:

Looks pretty nice to me. I mostly have naming and documentation comments.

::: gdk/gdkwindow.c
@@ +482,3 @@
+   * GdkWindow::moved-to-rect:
+   * @window: the #GdkWindow that moved
+   * @flipped_rect: the position of @window after any possible flipping

I wonder if we actually need this one. Don't seem to be called. But I suppose it doesn't hurt.

Side question: can we add more parameters to the signal later?

@@ +483,3 @@
+   * @window: the #GdkWindow that moved
+   * @flipped_rect: the position of @window after any possible flipping
+   * @slid_rect: the position of @window after any possible sliding

bikeshed mode: should we maybe call this final rect or something? It might have slid, or flipped, or not moved at all really.

@@ +6177,3 @@
 
+/**
+ * gdk_window_move_to_rect:

bikeshed mode: I wonder we can have a name that states that it is relative to something, like gdk_window_(place|move)_relative_to. It also doesn't really move the window to a "rect" (or, rather, gdk_window_move rather moves the window to a "rect", but this does much more).

@@ +6185,3 @@
+ * @anchor_hints: positioning hints to use when limited on space
+ * @rect_anchor_dx: horizontal offset to shift @rect's anchor point
+ * @rect_anchor_dy: vertical offset to shift @rect's anchor point

nit: When we discussed this, the purpose of the dx/dy was to offset the window (so that the selected combox element would be placed on top of the anchor rect), not the anchor rect. But in the end, I guess it doesn't matter, since the effect is the same.

@@ +6192,3 @@
+ * @window_anchor determine anchor points on @rect and @window to pin together.
+ * @rect's anchor point can optionally be offset by @rect_anchor_dx and
+ * @rect_anchor_dy.

Should probably clarify what for example gravity "north" means for the x value. Is it the same as center for x?

@@ +6203,3 @@
+void
+gdk_window_move_to_rect (GdkWindow          *window,
+                         GdkWindow          *transient_for,

How does this transient_of relate to the one of gdk_window_set_transient_for()? Does calling this function implicitly mean the same thing as having called gdk_window_set_transient_for()? I think it makes sense to have it as an argument here, to ensure that the call always have a parent/transient_for.

@@ +6216,3 @@
+  g_return_if_fail (GDK_IS_WINDOW (window));
+  g_return_if_fail (GDK_IS_WINDOW (transient_for));
+  g_return_if_fail (rect);

I think we should add a

g_return_if_fail (gdk_window_get_display (window) == gdk_window_get_display (transient_for));

to ensure we don't mix displays. (related to the other note in the impl part).

::: gdk/gdkwindow.h
@@ +263,3 @@
+ * the window would fall off-screen if placed in its ideal position. The
+ * default implementation first attempts to flip the anchors if allowed, then
+ * attempts to slide the window on-screen if allowed.

I think we should not state what the default implementation would do, but what result can be expected. We don't want different backend making different choices.

We should still say what order things are done, and while at it, also say when resizing is attempted.

::: gdk/gdkwindowimpl.c
@@ +55,3 @@
+
+  return gdk_display_get_default ();
+}

When is this needed? I don't think it makes any sense to support having a different GdkDisplay for the child and parent. It won't work in Mir (I assume) and Wayland. I realize this is the fallback implementation, but I think it is reasonable to require that both the moved window and the transient-for window both come from the same GdkDisplay.

@@ +119,3 @@
+
+static gint
+get_anchor_y (GdkGravity anchor)

nit: While being a clever implementation, the naming of this is a bit confusing as well. It doesn't get an y coordinate, it gets some kind of number that combined with another number to create a factor (1 + value)/2 or (1 - value)/2. Not sure what to call that though. Mathematically it's used as a "term" in the formula, so maybe that?

@@ +143,3 @@
+
+static gint
+choose_position (gint      bounds_x,

nit: This function maybe flips right? Should it be called that maybe? maybe_flip_position (...)

@@ +152,3 @@
+                 gint      rect_anchor_dx,
+                 gboolean  flip,
+                 gboolean *flipped)

nit: You call these variables _x and _width, but you then pass y and height, which is a bit confusing. Should x and width be changed to pos and size or something?

@@ +169,3 @@
+
+  *flipped = FALSE;
+  return rect_x + (1 + rect_anchor) * rect_width / 2 + rect_anchor_dx - (1 + window_anchor) * window_width / 2;

nit: You could just save the first x and return it here, instead of recalculating it.

Review of attachment 330872 [details] [review]:

Looks good to me.

(In reply to Jonas Ådahl from comment #313)

> ::: gdk/gdkwindow.c
> @@ +482,3 @@
> +   * GdkWindow::moved-to-rect:
> +   * @window: the #GdkWindow that moved
> +   * @flipped_rect: the position of @window after any possible flipping
> 
> I wonder if we actually need this one. Don't seem to be called. But I
> suppose it doesn't hurt.
> 
> Side question: can we add more parameters to the signal later?

Not without breaking ABI: signals are just functions, so the same API compatibility rules apply, with the additional restriction that adding a return value to a signal that didn't have one is an ABI break, unlike in plain C functions.

Review of attachment 330874 [details] [review]:

Looks fairly good to me with some minor comments, but I don't see the point of splitting this and the next patch. This one just adds an API that is completely ignored.

::: docs/reference/gtk/Makefile.am
@@ +390,3 @@
+	$(srcdir)/images/popup-anchors.png				\
+	$(srcdir)/images/popup-flip.png					\
+	$(srcdir)/images/popup-slide.png				\

Very nice.

::: gtk/gtkmenu.c
@@ +571,3 @@
+   * @menu: the #GtkMenu that popped up
+   * @flipped_rect: the position of @menu after any possible flipping
+   * @slid_rect: the position of @menu after any possible sliding

The comments about naming these (added to the gdk patch) applies here as well.

@@ +759,3 @@
+   * For example, %GDK_ANCHOR_FLIP_Y will replace %GDK_GRAVITY_NORTH_WEST with
+   * %GDK_GRAVITY_SOUTH_WEST and vice versa if the menu extends beyond the
+   * bottom edge of the monitor.

Well, it'll attempt to, and fall back if it failed.

@@ +2093,3 @@
+
+      if (!display)
+        display = gdk_display_get_default ();

If this happens we will get surprises when we are using multiple displays. I think we should just error (return NULL I suppose) if we can't get at least a device from the correct display.

@@ +2109,3 @@
+ * @rect_anchor: the point on @rect to align with @menu's anchor point
+ * @menu_anchor: the point on @menu to align with @rect's anchor point
+ * @trigger_event: the #GdkEvent that initiated this request

Should we also say (here and the other new functions) that if NULL is passed, we pretend its the latest event that happened to come? Or should we just make it (not nullable)?

I suppose we can't make it nullable if we want voice activatable menus.

Review of attachment 330875 [details] [review]:

Looks mostly good to me.

::: gtk/gtkmenu.c
@@ +4833,3 @@
+    default:
+      g_warning ("unknown GdkGravity: %d", anchor);
+      return anchor;

If you remove the default and add a g_warning () after the switch we'll get a compiler warning would the enum ever change (however unlikely).

@@ +4912,3 @@
+
+      g_signal_handlers_disconnect_by_func (toplevel, moved_to_rect_cb, menu);
+      g_signal_connect (toplevel, "moved-to-rect", G_CALLBACK (moved_to_rect_cb), menu);

Should we also disconnect on object destruction? (or just use g_signal_connect_object?)

@@ +4924,3 @@
+
+      return;
+    }

I think it'd be nice if we could instead split up gtk_menu_position into

gtk_menu_position_legacy (...) that does what gtk_menu_position did before this patch.

gtk_menu_psition (...)
{
  if (use_legacy_positioning (..))
    {
      gtk_menu_position_legacy (...);
      return;
    }

  ... the stuff that ends up calling gdk_window_move_to_rect ...
}

Eventually we should add a g_warning () if we ever end up in the legacy path, since that path is non-portable.

Review of attachment 330876 [details] [review]:

Does this contain any way to test resize, like a very tall menu? Couldn't find it. Also it prints a bunch of these warnings when I run it:

(lt-testpopupat:29524): Gtk-WARNING **: gtk_menu_attach_to_widget(): menu already attached to GtkMenuButton

(In reply to Emmanuele Bassi (:ebassi) from comment #315)
> (In reply to Jonas Ådahl from comment #313)
> 
> > ::: gdk/gdkwindow.c
> > @@ +482,3 @@
> > +   * GdkWindow::moved-to-rect:
> > +   * @window: the #GdkWindow that moved
> > +   * @flipped_rect: the position of @window after any possible flipping
> > 
> > I wonder if we actually need this one. Don't seem to be called. But I
> > suppose it doesn't hurt.
> > 
> > Side question: can we add more parameters to the signal later?
> 
> Not without breaking ABI: signals are just functions, so the same API
> compatibility rules apply, with the additional restriction that adding a
> return value to a signal that didn't have one is an ABI break, unlike in
> plain C functions.

Ah right, because the user_data is added as the last argument to the invocation. Otherwise it could work, couldn't it?

Review of attachment 330877 [details] [review]:

Looks mostly fine to me. Only some minor comments.

::: demos/gtk-demo/main.c
@@ +967,3 @@
 scrollbar_popup (GtkWidget *scrollbar, GtkWidget *menu)
 {
+  gtk_menu_popup_at_pointer (GTK_MENU (menu), NULL);

As a continuation of a comment on another patch, should we make it clear at the call site what event is triggering, if any? Here, would it be gtk_get_current_event ().

::: gtk/deprecated/gtkcolorsel.c
@@ +1467,3 @@
 
+  if (trigger_event && gdk_event_triggers_context_menu (trigger_event))
+    gtk_menu_popup_at_pointer (GTK_MENU (menu), trigger_event);

Hmm. Did we do this before?

::: gtk/gtkcombobox.c
@@ +2167,3 @@
+                    "anchor-hints", GDK_ANCHOR_FLIP_Y |
+                                    GDK_ANCHOR_SLIDE |
+                                    GDK_ANCHOR_RESIZE,

nit: Wrapping the enum entries inside () would make indenters happy (here and once more below)

::: gtk/gtkmenu.c
@@ +2183,3 @@
                            activate_time);
 
+  g_clear_pointer (&current_event, gdk_event_free);

Looks like the changes in this file leaked over from some other patch.

Review of attachment 330878 [details] [review]:

::: gdk/mir/gdkmirwindowimpl.c
@@ +714,3 @@
+  impl->has_rect = TRUE;
+
+  ensure_no_surface (window);

I don't see anywhere where you call "moved_to_rect". Should a backend not do that, even though the result may be wrong? Should we maybe add that to the documentation that a backend may not support the feedback, or should the backend always pretend to know what happen?

Created attachment 331424 [details] [review]
[PATCH 1/7] gdkwindow: store shadow sizes

Created attachment 331425 [details] [review]
[PATCH 2/7] gdkwindow: add gdk_window_move_to_rect ()

Created attachment 331426 [details] [review]
[PATCH 3/7] gtkmenu: add gtk_menu_popup_at_* ()

Created attachment 331427 [details] [review]
[PATCH 4/7] gtkmenu: position menu using gdk_window_move_to_rect ()

Created attachment 331428 [details] [review]
[PATCH 5/7] add demo for testing gtk_menu_popup_at_* ()

Created attachment 331429 [details] [review]
[PATCH 6/7] port to new gtk_menu_popup_at_* () functions

Created attachment 331430 [details] [review]
[PATCH 7/7] mir: implement gdk_window_move_to_rect ()

(In reply to Jonas Ådahl from comment #313)
> Review of attachment 330873 [details] [review] [review]:
>
> Looks pretty nice to me. I mostly have naming and documentation comments.
>
> ::: gdk/gdkwindow.c
> @@ +482,3 @@
> +   * GdkWindow::moved-to-rect:
> +   * @window: the #GdkWindow that moved
> +   * @flipped_rect: the position of @window after any possible flipping
>
> I wonder if we actually need this one. Don't seem to be called. But I
> suppose it doesn't hurt.

Yep, the signal is used in a later patch for combo box offsetting.



> Side question: can we add more parameters to the signal later?
>
> @@ +483,3 @@
> +   * @window: the #GdkWindow that moved
> +   * @flipped_rect: the position of @window after any possible flipping
> +   * @slid_rect: the position of @window after any possible sliding
>
> bikeshed mode: should we maybe call this final rect or something? It might
> have slid, or flipped, or not moved at all really.

Sure.



> @@ +6177,3 @@
>
> +/**
> + * gdk_window_move_to_rect:
>
> bikeshed mode: I wonder we can have a name that states that it is relative
> to something, like gdk_window_(place|move)_relative_to. It also doesn't
> really move the window to a "rect" (or, rather, gdk_window_move rather moves
> the window to a "rect", but this does much more).

gdk_window_move_relative_to_rect ()? gdk_window_anchor_to_rect ()? Not sure what's optimal, but I don't mind since it's private API. I would like to avoid a name that's too long though.

I originally chose gdk_window_move_to_rect () since this is kind of a higher-level wrapper around gdk_window_move ().



> @@ +6185,3 @@
> + * @anchor_hints: positioning hints to use when limited on space
> + * @rect_anchor_dx: horizontal offset to shift @rect's anchor point
> + * @rect_anchor_dy: vertical offset to shift @rect's anchor point
>
> nit: When we discussed this, the purpose of the dx/dy was to offset the
> window (so that the selected combox element would be placed on top of the
> anchor rect), not the anchor rect. But in the end, I guess it doesn't
> matter, since the effect is the same.

It's still the case, but I guess it isn't very clear here that moving the rect anchor is equivalent to moving the window/menu. I'll amend the docs to make that clearer.



> @@ +6192,3 @@
> + * @window_anchor determine anchor points on @rect and @window to pin
> together.
> + * @rect's anchor point can optionally be offset by @rect_anchor_dx and
> + * @rect_anchor_dy.
>
> Should probably clarify what for example gravity "north" means for the x
> value. Is it the same as center for x?

I think the GdkGravity docs are descriptive enough. It says "the reference point is in the middle of the top edge."



> @@ +6203,3 @@
> +void
> +gdk_window_move_to_rect (GdkWindow          *window,
> +                         GdkWindow          *transient_for,
>
> How does this transient_of relate to the one of
> gdk_window_set_transient_for()? Does calling this function implicitly mean
> the same thing as having called gdk_window_set_transient_for()? I think it
> makes sense to have it as an argument here, to ensure that the call always
> have a parent/transient_for.

Sure, I guess "transient_for" isn't a good name in general. It just happens to be that the rect window and transient_for are the same in the popup menu case. I'll replace it with just "rect_window" instead.



> @@ +6216,3 @@
> +  g_return_if_fail (GDK_IS_WINDOW (window));
> +  g_return_if_fail (GDK_IS_WINDOW (transient_for));
> +  g_return_if_fail (rect);
>
> I think we should add a
>
> g_return_if_fail (gdk_window_get_display (window) == gdk_window_get_display
> (transient_for));
>
> to ensure we don't mix displays. (related to the other note in the impl
> part).

No problem. It's not possible for a window to have no display, is it?



> ::: gdk/gdkwindow.h
> @@ +263,3 @@
> + * the window would fall off-screen if placed in its ideal position. The
> + * default implementation first attempts to flip the anchors if allowed,
> then
> + * attempts to slide the window on-screen if allowed.
>
> I think we should not state what the default implementation would do, but
> what result can be expected. We don't want different backend making
> different choices.
>
> We should still say what order things are done, and while at it, also say
> when resizing is attempted.

Right, this is a bit tricky since it's hard to describe the result without describing the process. But yes, it's up to the backend how it wants to implement this. We can probably state something deliberately vague like "in general, flipping takes precedence over sliding, which takes precedence over resizing."



> ::: gdk/gdkwindowimpl.c
> @@ +55,3 @@
> +
> +  return gdk_display_get_default ();
> +}
>
> When is this needed? I don't think it makes any sense to support having a
> different GdkDisplay for the child and parent. It won't work in Mir (I
> assume) and Wayland. I realize this is the fallback implementation, but I
> think it is reasonable to require that both the moved window and the
> transient-for window both come from the same GdkDisplay.

I'll add a warning there for that case.



> @@ +119,3 @@
> +
> +static gint
> +get_anchor_y (GdkGravity anchor)
>
> nit: While being a clever implementation, the naming of this is a bit
> confusing as well. It doesn't get an y coordinate, it gets some kind of
> number that combined with another number to create a factor (1 + value)/2 or
> (1 - value)/2. Not sure what to call that though. Mathematically it's used
> as a "term" in the formula, so maybe that?

Sure, let's go with "sign" since it's -1, 0, or +1?



> @@ +143,3 @@
> +
> +static gint
> +choose_position (gint      bounds_x,
>
> nit: This function maybe flips right? Should it be called that maybe?
> maybe_flip_position (...)

Sounds good.



> @@ +152,3 @@
> +                 gint      rect_anchor_dx,
> +                 gboolean  flip,
> +                 gboolean *flipped)
>
> nit: You call these variables _x and _width, but you then pass y and height,
> which is a bit confusing. Should x and width be changed to pos and size or
> something?

Sounds good.



> @@ +169,3 @@
> +
> +  *flipped = FALSE;
> +  return rect_x + (1 + rect_anchor) * rect_width / 2 + rect_anchor_dx - (1
> + window_anchor) * window_width / 2;
>
> nit: You could just save the first x and return it here, instead of
> recalculating it.

Right, thanks!



(In reply to Jonas Ådahl from comment #316)
> Review of attachment 330874 [details] [review] [review]:
>
> @@ +759,3 @@
> +   * For example, %GDK_ANCHOR_FLIP_Y will replace %GDK_GRAVITY_NORTH_WEST
> with
> +   * %GDK_GRAVITY_SOUTH_WEST and vice versa if the menu extends beyond the
> +   * bottom edge of the monitor.
>
> Well, it'll attempt to, and fall back if it failed.

Sure.



> @@ +2093,3 @@
> +
> +      if (!display)
> +        display = gdk_display_get_default ();
>
> If this happens we will get surprises when we are using multiple displays. I
> think we should just error (return NULL I suppose) if we can't get at least
> a device from the correct display.

It seems like an extremely rare case, can we just emit a warning when this happens and try our best with the default display?



> @@ +2109,3 @@
> + * @rect_anchor: the point on @rect to align with @menu's anchor point
> + * @menu_anchor: the point on @menu to align with @rect's anchor point
> + * @trigger_event: the #GdkEvent that initiated this request
>
> Should we also say (here and the other new functions) that if NULL is
> passed, we pretend its the latest event that happened to come? Or should we
> just make it (not nullable)?
>
> I suppose we can't make it nullable if we want voice activatable menus.

I think passing in NULL for the trigger_event is fine, we just end up using gtk_get_current_event () in that case. I added some documentation for this.



(In reply to Jonas Ådahl from comment #317)
> Review of attachment 330875 [details] [review] [review]:
>
> Looks mostly good to me.
>
> ::: gtk/gtkmenu.c
> @@ +4833,3 @@
> +    default:
> +      g_warning ("unknown GdkGravity: %d", anchor);
> +      return anchor;
>
> If you remove the default and add a g_warning () after the switch we'll get
> a compiler warning would the enum ever change (however unlikely).

Good point, done.



> @@ +4912,3 @@
> +
> +      g_signal_handlers_disconnect_by_func (toplevel, moved_to_rect_cb,
> menu);
> +      g_signal_connect (toplevel, "moved-to-rect", G_CALLBACK
> (moved_to_rect_cb), menu);
>
> Should we also disconnect on object destruction? (or just use
> g_signal_connect_object?)

Good catch, adding a disconnect on destroy.



> @@ +4924,3 @@
> +
> +      return;
> +    }
>
> I think it'd be nice if we could instead split up gtk_menu_position into
>
> gtk_menu_position_legacy (...) that does what gtk_menu_position did before
> this patch.
>
> gtk_menu_psition (...)
> {
>   if (use_legacy_positioning (..))
>     {
>       gtk_menu_position_legacy (...);
>       return;
>     }
>
>   ... the stuff that ends up calling gdk_window_move_to_rect ...
> }
>
> Eventually we should add a g_warning () if we ever end up in the legacy
> path, since that path is non-portable.

Sure.



(In reply to Jonas Ådahl from comment #318)
> Review of attachment 330876 [details] [review] [review]:
>
> Does this contain any way to test resize, like a very tall menu? Couldn't
> find it. Also it prints a bunch of these warnings when I run it:
>
> (lt-testpopupat:29524): Gtk-WARNING **: gtk_menu_attach_to_widget(): menu
> already attached to GtkMenuButton

Added, fixed.



(In reply to Jonas Ådahl from comment #320)
> Review of attachment 330877 [details] [review] [review]:
>
> Looks mostly fine to me. Only some minor comments.
>
> ::: demos/gtk-demo/main.c
> @@ +967,3 @@
>  scrollbar_popup (GtkWidget *scrollbar, GtkWidget *menu)
>  {
> +  gtk_menu_popup_at_pointer (GTK_MENU (menu), NULL);
>
> As a continuation of a comment on another patch, should we make it clear at
> the call site what event is triggering, if any? Here, would it be
> gtk_get_current_event ().

Added docs on the API side to state that NULL means gtk_get_current_event (). At the call site, it's hard to assert the type of the event because in general I guess these functions could be called from anywhere.



> ::: gtk/deprecated/gtkcolorsel.c
> @@ +1467,3 @@
>
> +  if (trigger_event && gdk_event_triggers_context_menu (trigger_event))
> +    gtk_menu_popup_at_pointer (GTK_MENU (menu), trigger_event);
>
> Hmm. Did we do this before?

We didn't do this before, but now we're also using GDK_KEY_PRESS events (menu key) as trigger events where NULL was used before. So now it's no longer sufficient to just check if the trigger_event is NULL/non-NULL.



> ::: gtk/gtkcombobox.c
> @@ +2167,3 @@
> +                    "anchor-hints", GDK_ANCHOR_FLIP_Y |
> +                                    GDK_ANCHOR_SLIDE |
> +                                    GDK_ANCHOR_RESIZE,
>
> nit: Wrapping the enum entries inside () would make indenters happy (here
> and once more below)

Good idea.



> ::: gtk/gtkmenu.c
> @@ +2183,3 @@
>                             activate_time);
>
> +  g_clear_pointer (&current_event, gdk_event_free);
>
> Looks like the changes in this file leaked over from some other patch.

Thanks, fixed.



(In reply to Jonas Ådahl from comment #321)
> Review of attachment 330878 [details] [review] [review]:
>
> ::: gdk/mir/gdkmirwindowimpl.c
> @@ +714,3 @@
> +  impl->has_rect = TRUE;
> +
> +  ensure_no_surface (window);
>
> I don't see anywhere where you call "moved_to_rect". Should a backend not do
> that, even though the result may be wrong? Should we maybe add that to the
> documentation that a backend may not support the feedback, or should the
> backend always pretend to know what happen?

I think it's better not to emit it at all than to emit it with invalid information. I'll document the fact that backends aren't guaranteed to support it.

(In reply to William Hua from comment #329)
> (In reply to Jonas Ådahl from comment #313)
> > ::: gdk/gdkwindow.c
> > @@ +482,3 @@
> > +   * GdkWindow::moved-to-rect:
> > +   * @window: the #GdkWindow that moved
> > +   * @flipped_rect: the position of @window after any possible flipping
> >
> > I wonder if we actually need this one. Don't seem to be called. But I
> > suppose it doesn't hurt.
> 
> Yep, the signal is used in a later patch for combo box offsetting.
> 

I was more referring to "flipped_rect". I went through the handlers but couldn't find any user.

> 
> > @@ +6177,3 @@
> >
> > +/**
> > + * gdk_window_move_to_rect:
> >
> > bikeshed mode: I wonder we can have a name that states that it is relative
> > to something, like gdk_window_(place|move)_relative_to. It also doesn't
> > really move the window to a "rect" (or, rather, gdk_window_move rather moves
> > the window to a "rect", but this does much more).
> 
> gdk_window_move_relative_to_rect ()? gdk_window_anchor_to_rect ()? Not sure
> what's optimal, but I don't mind since it's private API. I would like to
> avoid a name that's too long though.
> 
> I originally chose gdk_window_move_to_rect () since this is kind of a
> higher-level wrapper around gdk_window_move ().
> 

I don't really have a better name at hand. Since its private, we can bikeshed about it next time.

> 
> > @@ +6192,3 @@
> > + * @window_anchor determine anchor points on @rect and @window to pin
> > together.
> > + * @rect's anchor point can optionally be offset by @rect_anchor_dx and
> > + * @rect_anchor_dy.
> >
> > Should probably clarify what for example gravity "north" means for the x
> > value. Is it the same as center for x?
> 
> I think the GdkGravity docs are descriptive enough. It says "the reference
> point is in the middle of the top edge."

Ah. True. Nm, just me not reading the docs properly.

> 
> 
> 
> > @@ +6203,3 @@
> > +void
> > +gdk_window_move_to_rect (GdkWindow          *window,
> > +                         GdkWindow          *transient_for,
> >
> > How does this transient_of relate to the one of
> > gdk_window_set_transient_for()? Does calling this function implicitly mean
> > the same thing as having called gdk_window_set_transient_for()? I think it
> > makes sense to have it as an argument here, to ensure that the call always
> > have a parent/transient_for.
> 
> Sure, I guess "transient_for" isn't a good name in general. It just happens
> to be that the rect window and transient_for are the same in the popup menu
> case. I'll replace it with just "rect_window" instead.
> 

But can't we make it the same? I.e. gdk_window_move_to_rect implicitly sets transient_for, or we require that transient_for is set and g_return_if_fail if it isn't? In the backend I need to make an awkward decision whether to use the parent here and ignore the "real" transient_for, or ignore the parent here, and use the "real" transient_for. It'd be good if there were only one option.

> 
> 
> > @@ +6216,3 @@
> > +  g_return_if_fail (GDK_IS_WINDOW (window));
> > +  g_return_if_fail (GDK_IS_WINDOW (transient_for));
> > +  g_return_if_fail (rect);
> >
> > I think we should add a
> >
> > g_return_if_fail (gdk_window_get_display (window) == gdk_window_get_display
> > (transient_for));
> >
> > to ensure we don't mix displays. (related to the other note in the impl
> > part).
> 
> No problem. It's not possible for a window to have no display, is it?
> 

I assume that its not possible but I have not verified.

> 
> 
> > ::: gdk/gdkwindow.h
> > @@ +263,3 @@
> > + * the window would fall off-screen if placed in its ideal position. The
> > + * default implementation first attempts to flip the anchors if allowed,
> > then
> > + * attempts to slide the window on-screen if allowed.
> >
> > I think we should not state what the default implementation would do, but
> > what result can be expected. We don't want different backend making
> > different choices.
> >
> > We should still say what order things are done, and while at it, also say
> > when resizing is attempted.
> 
> Right, this is a bit tricky since it's hard to describe the result without
> describing the process. But yes, it's up to the backend how it wants to
> implement this. We can probably state something deliberately vague like "in
> general, flipping takes precedence over sliding, which takes precedence over
> resizing."
> 

Yea, but we at least try to describe what the caller can expect in return, even though we may make it a bit vague to allow wiggle room in the backend.

> 
> > @@ +119,3 @@
> > +
> > +static gint
> > +get_anchor_y (GdkGravity anchor)
> >
> > nit: While being a clever implementation, the naming of this is a bit
> > confusing as well. It doesn't get an y coordinate, it gets some kind of
> > number that combined with another number to create a factor (1 + value)/2 or
> > (1 - value)/2. Not sure what to call that though. Mathematically it's used
> > as a "term" in the formula, so maybe that?
> 
> Sure, let's go with "sign" since it's -1, 0, or +1?
> 

"sign" is fine I guess. Is 0 a sign though? +/- :P Otherwise maybe direction? 
 
> 
> > @@ +2093,3 @@
> > +
> > +      if (!display)
> > +        display = gdk_display_get_default ();
> >
> > If this happens we will get surprises when we are using multiple displays. I
> > think we should just error (return NULL I suppose) if we can't get at least
> > a device from the correct display.
> 
> It seems like an extremely rare case, can we just emit a warning when this
> happens and try our best with the default display?
> 

I suppose so. We'd also probably get crashes later on on multi display clients, but at least we have a warning that points us to where things went wrong.


> 
> 
> (In reply to Jonas Ådahl from comment #321)
> > Review of attachment 330878 [details] [review] [review] [review]:
> >
> > ::: gdk/mir/gdkmirwindowimpl.c
> > @@ +714,3 @@
> > +  impl->has_rect = TRUE;
> > +
> > +  ensure_no_surface (window);
> >
> > I don't see anywhere where you call "moved_to_rect". Should a backend not do
> > that, even though the result may be wrong? Should we maybe add that to the
> > documentation that a backend may not support the feedback, or should the
> > backend always pretend to know what happen?
> 
> I think it's better not to emit it at all than to emit it with invalid
> information. I'll document the fact that backends aren't guaranteed to
> support it.

How would the frontend know what to expect then? If it would want to wait until being positioned until drawing itself, for example if it would draw itself differently depending on the final position, how would it know whether to wait or not? Could we maybe instead make it possible to pass NULL rects as result, to communicate that it was positioned, but without knowing the final position?

Review of attachment 331425 [details] [review]:

Looks good to me. Just a nit regarding declaring signal stability. The question whether to include "flipped_rect" also still stands (if we can change this signal any way we want in the future without caring about stability we can remove it now if its not used and add it later if needed).

::: gdk/gdkwindow.c
@@ +499,3 @@
+   * flipping, but before any possible sliding. @final_rect is @flipped_rect,
+   * but possibly translated in the case that flipping is still ineffective in
+   * keeping @window on-screen.

Sorry, forgot about this in the last round:

Should we add

Stability: unstable

for now?

Review of attachment 331426 [details] [review]:

Looks good. Only thing to discuss is how an application should know whether it will get the correct position or not.

::: gtk/gtkmenu.c
@@ +571,3 @@
+   * @menu: the #GtkMenu that popped up
+   * @flipped_rect: (not nullable): the position of @menu after any possible
+   *                flipping

I see now that flipped_rect is passed along here (though without users so far) but if we are assuming this signal is going to be stable and there might eventually be users, please ignore my comment about removing it from the private GDK API.

@@ +579,3 @@
+   * using gtk_menu_popup_at_rect (), gtk_menu_popup_at_widget (), or
+   * gtk_menu_popup_at_pointer (). This signal is only emitted if the backend
+   * supports it.

Just repeating what was commented on elsewhere. How would an application know whether to wait or not?

Review of attachment 331427 [details] [review]:

Looks fine to me. Only one style nit.

::: gtk/gtkmenu.c
@@ +5129,3 @@
+    }
+
+  if (rect_window)

nit: Feel free to ignore, but you can avoid the indented block if you'd do:

if (!rect_window)
  {
    gtk_menu_position_legacy (menu, set_scroll_offset);
    return;
  }

/* Realize so ...

Review of attachment 331428 [details] [review]:

A useful demo IMO.

Review of attachment 331429 [details] [review]:

Looks good to me.

Review of attachment 331430 [details] [review]:

Looks fine to me. Marking as 'reviewed' until we have discussed the emitting of the signal or not.

(In reply to Jonas Ådahl from comment #330)
> (In reply to William Hua from comment #329)
> > (In reply to Jonas Ådahl from comment #313)
> > > ::: gdk/gdkwindow.c
> > > @@ +482,3 @@
> > > +   * GdkWindow::moved-to-rect:
> > > +   * @window: the #GdkWindow that moved
> > > +   * @flipped_rect: the position of @window after any possible flipping
> > >
> > > I wonder if we actually need this one. Don't seem to be called. But I
> > > suppose it doesn't hurt.
> > 
> > Yep, the signal is used in a later patch for combo box offsetting.
> > 
> 
> I was more referring to "flipped_rect". I went through the handlers but
> couldn't find any user.

Ah, ok, I thought you meant the signal itself. flipped_rect is used in gtk_menu_update_scroll_offset () to determine how to offset the combo box menu when it gets pushed back on-screen.

(In reply to William Hua from comment #337)
> (In reply to Jonas Ådahl from comment #330)
> > (In reply to William Hua from comment #329)
> > > (In reply to Jonas Ådahl from comment #313)
> > > > ::: gdk/gdkwindow.c
> > > > @@ +482,3 @@
> > > > +   * GdkWindow::moved-to-rect:
> > > > +   * @window: the #GdkWindow that moved
> > > > +   * @flipped_rect: the position of @window after any possible flipping
> > > >
> > > > I wonder if we actually need this one. Don't seem to be called. But I
> > > > suppose it doesn't hurt.
> > > 
> > > Yep, the signal is used in a later patch for combo box offsetting.
> > > 
> > 
> > I was more referring to "flipped_rect". I went through the handlers but
> > couldn't find any user.
> 
> Ah, ok, I thought you meant the signal itself. flipped_rect is used in
> gtk_menu_update_scroll_offset () to determine how to offset the combo box
> menu when it gets pushed back on-screen.

Ah, right. Ignore the comments about flipped_rect then.

Created attachment 331520 [details] [review]
[PATCH 2/7] gdkwindow: add gdk_window_move_to_rect ()

Created attachment 331521 [details] [review]
[PATCH 3/7] gtkmenu: add gtk_menu_popup_at_* ()

Created attachment 331522 [details] [review]
[PATCH 4/7] gtkmenu: position menu using gdk_window_move_to_rect ()

Created attachment 331523 [details] [review]
[PATCH 7/7] mir: implement gdk_window_move_to_rect ()

(In reply to Jonas Ådahl from comment #330)
> (In reply to William Hua from comment #329)
> > (In reply to Jonas Ådahl from comment #313)
> > > @@ +6203,3 @@
> > > +void
> > > +gdk_window_move_to_rect (GdkWindow          *window,
> > > +                         GdkWindow          *transient_for,
> > >
> > > How does this transient_of relate to the one of
> > > gdk_window_set_transient_for()? Does calling this function implicitly mean
> > > the same thing as having called gdk_window_set_transient_for()? I think it
> > > makes sense to have it as an argument here, to ensure that the call always
> > > have a parent/transient_for.
> > 
> > Sure, I guess "transient_for" isn't a good name in general. It just happens
> > to be that the rect window and transient_for are the same in the popup menu
> > case. I'll replace it with just "rect_window" instead.
> > 
> 
> But can't we make it the same? I.e. gdk_window_move_to_rect implicitly sets
> transient_for, or we require that transient_for is set and g_return_if_fail
> if it isn't? In the backend I need to make an awkward decision whether to
> use the parent here and ignore the "real" transient_for, or ignore the
> parent here, and use the "real" transient_for. It'd be good if there were
> only one option.

(IMHO) I feel like calling gdk_window_set_transient_for () implicitly is a weird thing to do since gdk_window_move_to_rect () should be able to work with non-transient windows too, in theory. So this is why I'd like to have the parameter called "rect_window", which would only be used for positioning.



> > > ::: gdk/gdkwindow.h
> > > @@ +263,3 @@
> > > + * the window would fall off-screen if placed in its ideal position. The
> > > + * default implementation first attempts to flip the anchors if allowed,
> > > then
> > > + * attempts to slide the window on-screen if allowed.
> > >
> > > I think we should not state what the default implementation would do, but
> > > what result can be expected. We don't want different backend making
> > > different choices.
> > >
> > > We should still say what order things are done, and while at it, also say
> > > when resizing is attempted.
> > 
> > Right, this is a bit tricky since it's hard to describe the result without
> > describing the process. But yes, it's up to the backend how it wants to
> > implement this. We can probably state something deliberately vague like "in
> > general, flipping takes precedence over sliding, which takes precedence over
> > resizing."
> > 
> 
> Yea, but we at least try to describe what the caller can expect in return,
> even though we may make it a bit vague to allow wiggle room in the backend.

So this is what it was amended to:

 265  * For example, %GDK_ANCHOR_FLIP_X will replace %GDK_GRAVITY_NORTH_WEST with
 266  * %GDK_GRAVITY_NORTH_EAST and vice versa if the window extends beyond the left
 267  * or right edges of the monitor.
 268  *
 269  * If %GDK_ANCHOR_SLIDE_X is set, the window can be shifted horizontally to fit
 270  * on-screen. If %GDK_ANCHOR_RESIZE_X is set, the window can be shrunken
 271  * horizontally to fit.
 272  *
 273  * In general, when multiple flags are set, flipping should take precedence over
 274  * sliding, which should take precedence over resizing.

How does it sound? It describes the order and briefly touches on the flags' purposes. Is there something you'd like to specify more clearly?



> > (In reply to Jonas Ådahl from comment #321)
> > > Review of attachment 330878 [details] [review] [review] [review] [review]:
> > >
> > > ::: gdk/mir/gdkmirwindowimpl.c
> > > @@ +714,3 @@
> > > +  impl->has_rect = TRUE;
> > > +
> > > +  ensure_no_surface (window);
> > >
> > > I don't see anywhere where you call "moved_to_rect". Should a backend not do
> > > that, even though the result may be wrong? Should we maybe add that to the
> > > documentation that a backend may not support the feedback, or should the
> > > backend always pretend to know what happen?
> > 
> > I think it's better not to emit it at all than to emit it with invalid
> > information. I'll document the fact that backends aren't guaranteed to
> > support it.
> 
> How would the frontend know what to expect then? If it would want to wait
> until being positioned until drawing itself, for example if it would draw
> itself differently depending on the final position, how would it know
> whether to wait or not? Could we maybe instead make it possible to pass NULL
> rects as result, to communicate that it was positioned, but without knowing
> the final position?

Sure, we could definitely do that, this sounds like a good compromise for backends that can't support it.



(In reply to Jonas Ådahl from comment #331)
> Review of attachment 331425 [details] [review] [review]:
> 
> Looks good to me. Just a nit regarding declaring signal stability. The
> question whether to include "flipped_rect" also still stands (if we can
> change this signal any way we want in the future without caring about
> stability we can remove it now if its not used and add it later if needed).
> 
> ::: gdk/gdkwindow.c
> @@ +499,3 @@
> +   * flipping, but before any possible sliding. @final_rect is
> @flipped_rect,
> +   * but possibly translated in the case that flipping is still ineffective
> in
> +   * keeping @window on-screen.
> 
> Sorry, forgot about this in the last round:
> 
> Should we add
> 
> Stability: unstable
> 
> for now?

Thanks, I used "Stability: Private" here and in a couple other places too if that's fine with you. What about the rest of the API? Should that be flagged as Unstable as well, or is it good as is?



(In reply to Jonas Ådahl from comment #332)
> Review of attachment 331426 [details] [review] [review]:
> @@ +579,3 @@
> +   * using gtk_menu_popup_at_rect (), gtk_menu_popup_at_widget (), or
> +   * gtk_menu_popup_at_pointer (). This signal is only emitted if the
> backend
> +   * supports it.
> 
> Just repeating what was commented on elsewhere. How would an application
> know whether to wait or not?

Yep, agreed. Let's emit the signal with NULLs to indicate not enough information.



(In reply to Jonas Ådahl from comment #333)
> Review of attachment 331427 [details] [review] [review]:
> 
> Looks fine to me. Only one style nit.
> 
> ::: gtk/gtkmenu.c
> @@ +5129,3 @@
> +    }
> +
> +  if (rect_window)
> 
> nit: Feel free to ignore, but you can avoid the indented block if you'd do:
> 
> if (!rect_window)
>   {
>     gtk_menu_position_legacy (menu, set_scroll_offset);
>     return;
>   }
> 
> /* Realize so ...

Sounds good.

(In reply to William Hua from comment #343)
> (In reply to Jonas Ådahl from comment #330)
> > (In reply to William Hua from comment #329)
> > > (In reply to Jonas Ådahl from comment #313)
> > > > @@ +6203,3 @@
> > > > +void
> > > > +gdk_window_move_to_rect (GdkWindow          *window,
> > > > +                         GdkWindow          *transient_for,
> > > >
> > > > How does this transient_of relate to the one of
> > > > gdk_window_set_transient_for()? Does calling this function implicitly mean
> > > > the same thing as having called gdk_window_set_transient_for()? I think it
> > > > makes sense to have it as an argument here, to ensure that the call always
> > > > have a parent/transient_for.
> > > 
> > > Sure, I guess "transient_for" isn't a good name in general. It just happens
> > > to be that the rect window and transient_for are the same in the popup menu
> > > case. I'll replace it with just "rect_window" instead.
> > > 
> > 
> > But can't we make it the same? I.e. gdk_window_move_to_rect implicitly sets
> > transient_for, or we require that transient_for is set and g_return_if_fail
> > if it isn't? In the backend I need to make an awkward decision whether to
> > use the parent here and ignore the "real" transient_for, or ignore the
> > parent here, and use the "real" transient_for. It'd be good if there were
> > only one option.
> 
> (IMHO) I feel like calling gdk_window_set_transient_for () implicitly is a
> weird thing to do since gdk_window_move_to_rect () should be able to work
> with non-transient windows too, in theory. So this is why I'd like to have
> the parameter called "rect_window", which would only be used for positioning.
> 

How would it work with non-transient windows? Are there any other types of parent-child relationships we have (except the GdkWindow tree thing we are getting rid of)?

Created attachment 331648 [details] [review]
[PATCH 2/8] gdkwindow: store transient_for window

Created attachment 331649 [details] [review]
[PATCH 3/8] gdkwindow: add gdk_window_move_to_rect ()

Created attachment 331650 [details] [review]
[PATCH 4/8] gtkmenu: add gtk_menu_popup_at_* ()

Created attachment 331651 [details] [review]
[PATCH 5/8] gtkmenu: position menu using gdk_window_move_to_rect ()

Created attachment 331652 [details] [review]
[PATCH 8/8] mir: implement gdk_window_move_to_rect ()

Review of attachment 331649 [details] [review]:

(Only took a very quick look, will look for real on monday)

::: gdk/gdkwindow.c
@@ +6221,3 @@
+
+  g_return_if_fail (GDK_IS_WINDOW (window));
+  g_return_if_fail (rect);

Should we g_return_if_fail (window->transient_for);?

Created attachment 331653 [details] [review]
[PATCH 3/8] gdkwindow: add gdk_window_move_to_rect ()

I've flagged all the public API as Unstable for now. We can change it easily in a later commit, as well as deprecate the old API.



(In reply to Jonas Ådahl from comment #344)
> How would it work with non-transient windows? Are there any other types of
> parent-child relationships we have (except the GdkWindow tree thing we are
> getting rid of)?

So after some discussion, there doesn't seem to be any compelling reason to allow positioning relative to anything other than the transient_for window.

So for the GDK private API, we won't be using a rect_window parameter. Backends will have to look at the transient_for, figure out what's the closest native ancestor, and transform between those two coordinate spaces when looking at the rect parameter. The Mir patch does this, and the default implementation does it implicitly via gdk_window_get_root_coords ().

To allow those backends to retrieve the current transient_for window, we store it in the GdkWindow internal struct.

The GTK+ menu API is unchanged, but the implementation now also sets the menu's toplevel's GDK window's transient_for. Doing this at the GTK+ level seems ok to me.

Review of attachment 331648 [details] [review]:

Looks fine to me.