GNOME Bugzilla – Bug 707194
attached dialogs are shifted up for CSD windows
Last modified: 2014-12-29 04:49:09 UTC
Created attachment 253713 [details] Screenshot of the problem Please see attached screenshot.
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.
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.
If it's a datapoint, this breaks https://github.com/hughsie/gnome-shell-extension-screenshot-window-sizer/issues/1 too.
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.
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.
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.
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.
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.
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?
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.
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.
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.
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.
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.
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
I can't see any reason this was left open.