After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 707194 - attached dialogs are shifted up for CSD windows
attached dialogs are shifted up for CSD windows
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
3.9.x
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2013-08-31 19:06 UTC by Sebastian Keller
Modified: 2014-12-29 04:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Screenshot of the problem (164.29 KB, image/png)
2013-08-31 19:06 UTC, Sebastian Keller
  Details
constraints: account for decorations when positioning modal dialogs (2.01 KB, patch)
2013-09-02 10:44 UTC, Giovanni Campagna
committed Details | Review
constraints, window: always account for CSD borders (9.67 KB, patch)
2013-09-02 10:44 UTC, Giovanni Campagna
none Details | Review
Use utility functions to convert between outer and client rectangles (29.65 KB, patch)
2013-11-18 15:36 UTC, Owen Taylor
committed Details | Review
MetaFrame: Cache borders (7.13 KB, patch)
2013-11-18 15:36 UTC, Owen Taylor
committed Details | Review
Stop passing around MetaFrameBorders (18.99 KB, patch)
2013-11-18 15:36 UTC, Owen Taylor
committed Details | Review

Description Sebastian Keller 2013-08-31 19:06:54 UTC
Created attachment 253713 [details]
Screenshot of the problem

Please see attached screenshot.
Comment 1 Giovanni Campagna 2013-09-02 10:44:43 UTC
Created attachment 253823 [details] [review]
constraints: account for decorations when positioning modal dialogs

What we want to achieve is that the dialog is visually centered
on the parent, including the decorations for both, and making sure
that CSD/frame_extents are respected.
Comment 2 Giovanni Campagna 2013-09-02 10:44:47 UTC
Created attachment 253824 [details] [review]
constraints, window: always account for CSD borders

Add a new helper, meta_window_calc_borders(), which can be used
on server or client-decorated windows to compute MetaFrameBorders,
and use it to provide the constraint subsystem with appropriate
data.
Comment 3 Richard Hughes 2013-10-11 08:34:19 UTC
If it's a datapoint, this breaks https://github.com/hughsie/gnome-shell-extension-screenshot-window-sizer/issues/1 too.
Comment 4 Owen Taylor 2013-11-15 16:58:48 UTC
Review of attachment 253824 [details] [review]:

I think the idea of this patch - to clean things up so that there's an API on GtkWindow and we don't always have to check window->frame and now window->has_custom_frame_extents, but I have a hard time accepting the negative visible frame extents. It maintains one thing (the visible borders being the difference between the client's window border and the visible edge of the border), but breaks the idea that the visible borders is actually the thickness of title-bar / side borders. Basically, I think it's going to mostly work but make the code almost impossible to understand going forward.

(The only thing that I found that I'm pretty sure won't work is the use of visible borders for cascading in meta_window_cascade)

What seems to me to be the *right* way to go about things is unfortunately going to be a lot of work and a lot of changed lines. I think the right approach is to:

 * Rename window->rect and window->frame->rect to break all existing code that accesses them
 * Have rectangle members of window that have unambiguous meaning for all types of windows - decorated, undecorated, CSD, SSD

  window->toplevel_rect (the rectangle of the child of the root window)
  window->visible_rect (the rectangle that the user thinks of as being the bounds of the window)
  window->client_rect (the rectangle of window->xwindow)

 * Get rid of the usage of calculated borders for translating between the rects above
 * Make constraining in constraints.c work in terms of the visible rect and have minimal usage of border information (only for cascades or things like that)
 ? Make the normal meta_window_move / meta_window_move_resize() work in terms of the visible rect, and handle the semantics of X resize requests with separate API or by translating to the visible rect. (This will break extensions/gnome-shell so definitely can't be done for 3.10, even if the rest of the changes could be.)

I'm going to try the above and see how it works out and how far I get, and see if there's going to be a subset backportable for 3.10 - if not, we can evaluate this patch as a 3.10 stop-gap.
Comment 5 Owen Taylor 2013-11-15 16:58:50 UTC
Review of attachment 253824 [details] [review]:

I think the idea of this patch - to clean things up so that there's an API on GtkWindow and we don't always have to check window->frame and now window->has_custom_frame_extents, but I have a hard time accepting the negative visible frame extents. It maintains one thing (the visible borders being the difference between the client's window border and the visible edge of the border), but breaks the idea that the visible borders is actually the thickness of title-bar / side borders. Basically, I think it's going to mostly work but make the code almost impossible to understand going forward.

(The only thing that I found that I'm pretty sure won't work is the use of visible borders for cascading in meta_window_cascade)

What seems to me to be the *right* way to go about things is unfortunately going to be a lot of work and a lot of changed lines. I think the right approach is to:

 * Rename window->rect and window->frame->rect to break all existing code that accesses them
 * Have rectangle members of window that have unambiguous meaning for all types of windows - decorated, undecorated, CSD, SSD

  window->toplevel_rect (the rectangle of the child of the root window)
  window->visible_rect (the rectangle that the user thinks of as being the bounds of the window)
  window->client_rect (the rectangle of window->xwindow)

 * Get rid of the usage of calculated borders for translating between the rects above
 * Make constraining in constraints.c work in terms of the visible rect and have minimal usage of border information (only for cascades or things like that)
 ? Make the normal meta_window_move / meta_window_move_resize() work in terms of the visible rect, and handle the semantics of X resize requests with separate API or by translating to the visible rect. (This will break extensions/gnome-shell so definitely can't be done for 3.10, even if the rest of the changes could be.)

I'm going to try the above and see how it works out and how far I get, and see if there's going to be a subset backportable for 3.10 - if not, we can evaluate this patch as a 3.10 stop-gap.
Comment 6 Owen Taylor 2013-11-15 21:22:21 UTC
Sorry about the duplicated review. After working on this a bit, I decided that the above scheme is far too disruptive to the code base. Instead, it seemed to work better to add functions:

void meta_window_client_rect_to_outer_rect (MetaWindow    *window,
                                            MetaRectangle *outer_rect,
                                            MetaRectangle *client_rect);
void meta_window_outer_rect_to_client_rect (MetaWindow    *window,
                                            MetaRectangle *outer_rect,
                                            MetaRectangle *client_rect);

(These are essenetially the same as the extend_by_frame/unextend_by_frame in constraints.c, but with clearer names and fixed up to handle custom frame extents.)

Then it's possible to go through the code base and change all of the problematical translation that is using window->frame.rect and window->rect and the borders to use these functions and meta_frame_calc_borders() to use these functions and meta_window_get_outer_rect()

What this does defeat is the way that the code passes around MetaFrameBorders to avoid reculculating it, but I think we can make MetaWindow cache the borders to get the same efficiency while cleaning up the code.
Comment 7 Owen Taylor 2013-11-18 15:36:38 UTC
Created attachment 260137 [details] [review]
Use utility functions to convert between outer and client rectangles

There are extensive places in the code where we convert between the client
rectangle and the outer rectangle - instead of manually doing it use
new helper functions on MetaWindow and the existing meta_window_get_outer_rect().

This fixes a number of bugs where the computation was being done incorrectly,
most of these bugs are with the recently added custom frame extents, but
some relate to invisible borders or even simply to confusion between the
window and frame rectangle.

Switch the placement code to place the outer rectangle (edge of frame) rather
than the client window - this simplifies things considerably.
Comment 8 Owen Taylor 2013-11-18 15:36:43 UTC
Created attachment 260138 [details] [review]
MetaFrame: Cache borders

Cache the computed border size so we can fetch the border size at
any time without worrying that we'll be spending too much time in
the theme code (in some cases we might allocate a PangoFontDescription
or do other significant work.)

The main effort here is clearing the cache when various bits of window
state change that could potentially affect the computed borders.
Comment 9 Owen Taylor 2013-11-18 15:36:46 UTC
Created attachment 260139 [details] [review]
Stop passing around MetaFrameBorders

Instead of passing around MetaFrameBorders, compute it when we need it.
This also allows us to know that we are using MetaFrameBorders only for windows
with frames (where it is meaningful) and not for frameless windows, which
can have custom borders which we need to interpret differently.
Comment 10 Jasper St. Pierre (not reading bugmail) 2013-11-18 16:20:30 UTC
Review of attachment 260137 [details] [review]:

This is an excellent cleanup.

I'd like a comment somewhere that describes what outer_rect, client_rect, and input_rect are in the CSD and SSD cases.

::: src/core/place.c
@@ +309,3 @@
+
+  focus_window = window->display->focus_window;
+  g_assert (focus_window != NULL);

Couldn't this fail if we have the stage focused? e.g. a window pops open while playing around with the panel menus, constraints run, bam.

Was this broken beforehand as well?
Comment 11 Jasper St. Pierre (not reading bugmail) 2013-11-18 16:22:04 UTC
Review of attachment 260138 [details] [review]:

Why don't you cache the client / frame rect instead? That would also help things a bit for the CSD case with _GTK_VISIBLE_BORDERS.
Comment 12 Jasper St. Pierre (not reading bugmail) 2013-11-18 16:42:54 UTC
Review of attachment 260139 [details] [review]:

Could we move MetaFrameBorders out of common.h then?

::: src/core/constraints.c
@@ +1384,3 @@
+
+      bottom_amount = info->current.height + borders.visible.bottom;
+      vert_amount_onscreen = borders.visible.top;

Hm, in order to properly implement constrain_titlebar_visible, we need more information from GTK+ on how big the titlebar is.

I'm also not sure what bottom_amount is for -- it seems to be the height of the visible area under the titlebar. If info->current.height is specifically the client rect, this is going to be wrong.

::: src/core/place.c
@@ +109,3 @@
+      MetaFrameBorders borders;
+
+      meta_frame_calc_borders (window->frame, &borders);

Hm. I was wondering what we did here before, since I don't like using MetaFrameBorders, but we just pulled the MetaFrameGeometry directly:

https://git.gnome.org/browse/mutter/diff/src/core/place.c?id=e0fb83c6918b53e454ff3cc6e972fedb82be2e11

This also isn't correct in the CSD case, but I don't think GTK+ exposes detailed information on how big the CSD titlebar is anyway... the 15 should mask it in any case.
Comment 13 Owen Taylor 2013-11-18 17:14:56 UTC
Review of attachment 260137 [details] [review]:

::: src/core/place.c
@@ +309,3 @@
+
+  focus_window = window->display->focus_window;
+  g_assert (focus_window != NULL);

Both callers only call this function 
window->denied_focus_and_not_transient is set, which is carefully engineered to only be true when focus_window is set  - the mainloop is never run when window->denied_focus_and_not_transient is set - it's only briefly set during meta_window_show().

So there is no bug and no regression here, but I'd agree it would be cleaner to simply make this code robust and leave any assertions in the callers, since window->display->focus_window generically *can* be NULL.
Comment 14 Owen Taylor 2013-11-18 17:25:01 UTC
Review of attachment 260138 [details] [review]:

Caching the client/frame rect doesn't work because where this is probably most intensively used is in the constraint code, which is interested in hypotheticals "what would the frame rect be for this client rect".

We could save a client-frame delta in MetaWindow:

 * I don't think there's any real difference in efficiency - a few conditionals and arithmetic operations  should be lost in the noise, as opposed to the work of getting the frame sizes from the theme code that we avoid by introducing *some* caching.

 * It would require a few more invalidations - when the framed-status of the window changed, when the custom frame extent changes.

Basically to me it's half-a-dozen one way 6 the other way, and I'm not inclined to rewrite it, though it would have been a legitimate way to approach it.
Comment 15 Owen Taylor 2013-11-18 17:44:39 UTC
Review of attachment 260139 [details] [review]:

::: src/core/constraints.c
@@ +1384,3 @@
+
+      bottom_amount = info->current.height + borders.visible.bottom;
+      vert_amount_onscreen = borders.visible.top;

bottom_amount is the distance that the window can extend beyond the edge of the screen (or panel if there's a bottom panel). If there's no titlebar information, then we're computing it as 

 clamp(client_height/4, 10, 75)

Which works OK-ish, leaving hi-dpi aside. This patch makes it very obvious where we are counting on theme information for constraints and placement because those are the only remaining places calling meta_frame_calc_borders() outside of the frame and core window code.

I'd like GTK+ to export out information about the titlebar size for placement use and I'd also like to have some structure within Mutter so that we could pick reasonable sizes for arbitrary numbers in a resolution-independent way, but that seems like enhancement outside of the scope of this patch.

(Another thing I noticed that is broken when working on this patchset is cascading for CSD windows - see the usage of meta_window_get_position() in place.c - the code is assuming this below and to the right of decorations, but that is not the case for CSD windows. Try opening a bunch of nautilus windows and you'll see that cascading doesn't behave right.)

::: src/core/place.c
@@ +109,3 @@
+      MetaFrameBorders borders;
+
+      meta_frame_calc_borders (window->frame, &borders);

It's not even clear to me that using the titlebar size is even that meaningful here. Using 15 seems fine other than hi-dpi considerations.
Comment 16 Owen Taylor 2013-11-19 18:59:23 UTC
Attachment 253823 [details] pushed as c652a54 - constraints: account for decorations when positioning modal dialogs
Attachment 260138 [details] pushed as b4036e0 - MetaFrame: Cache borders
Attachment 260139 [details] pushed as fe8829f - Stop passing around MetaFrameBorders
Comment 17 Jasper St. Pierre (not reading bugmail) 2014-12-29 04:49:09 UTC
I can't see any reason this was left open.