Created attachment 313356 [details] [review]
Proposed patch (mostly an RFC)

This patch is mostly for comments, it seems to work with the reproducer from the downstream bug, but there is no garantee that it will be 100% accurate (I am not actually sure we can be 100% accurate here, but that's still an improvement over the current status imho).

Created attachment 313358 [details] [review]
Proposed patch (Updated)

Updated patch to document the change to the give nsize and potential lack of accuracy.

Review of attachment 313358 [details] [review]:

::: gtk/gtkwindow.c
@@ +5214,3 @@
+ * size, but there is no garantee that the result will be totally
+ * accurate because the title bar and shadows are themes dependent and
+ * may not be known at the time the resize is issued.

And by 'actual window size' here we mean the 'non-titlebar, non-border, non-shadow portions of the window' ? Maybe that needs clarification.

Created attachment 313441 [details] [review]
Proposed patch (Updated)


Reworded the descriptions as per the review.

As a side note, I also tried to make the patch smarter by recalculating the sizes when the title bar is added, but that adds more complexity and doesn't work reliably so I think it's better to keep it simple as it is.

Review of attachment 313441 [details] [review]:

I think this is probably the best we can do, so I'm ok with it after moving the doc comment.
Does this solve a firefox problem ?

::: gtk/gtkwindow.c
@@ +5220,2 @@
  * Windows may not be resized smaller than 1 by 1 pixels.
  * 

I'd prefer if the doc comment stayed right above the function it documents. Move the aux. stuff above it.

Created attachment 313665 [details] [review]
Updated patch

(In reply to Matthias Clasen from comment #5)
> [...]
> I think this is probably the best we can do, so I'm ok with it after moving
> the doc comment.

Patch updated.

> Does this solve a firefox problem ?

According to https://bugzilla.redhat.com/show_bug.cgi?id=1269437#c20 it appears to help, yet I have not been able to confirm that myself in my own tests.

> I'd prefer if the doc comment stayed right above the function it documents.
> Move the aux. stuff above it.

Yeap, done in this updated version.

Review of attachment 313665 [details] [review]:

::: gtk/gtkwindow.c
@@ +5255,3 @@
+    *height = 1;
+}
+

Still looks to me like the aux function sits between gtk_window_resize and its docs...

Created attachment 313733 [details] [review]
Proposed patch (Updated)

(In reply to Matthias Clasen from comment #7)
> Still looks to me like the aux function sits between gtk_window_resize and
> its docs...

Oh, I see, sorry I misunderstood! Fixed now, hopefully :-)

Tangential question: Have you looked at doing the same kind of fixup for gtk_window_move ? It'll be more complicated if you try to handle window gravity...

Review of attachment 313733 [details] [review]:

looks good now

(In reply to Matthias Clasen from comment #9)
> Tangential question: Have you looked at doing the same kind of fixup for
> gtk_window_move ? It'll be more complicated if you try to handle window
> gravity...

Good point, no I haven't but I will now.

Created attachment 313859 [details] [review]
Another patch for gtk_window_move()/gtk_window_get_pos()

Tested with tests/testgtk gravity windows ("Window sizing" test, "Show gravity test windows") - Works with both move to current position and move to starting position, for all gravities.

We could merge these two patches (haven't done so because first one is already reviewed but it's the same issue after all).

Review of attachment 313859 [details] [review]:

No need to merge them. Looks good to me.

attachment 313733 [details] [review] pushed as 3450f53 GtkWindow: add up CSD size in gtk_window_resize()

attachment 313859 [details] [review] pushed as 305b34a GtkWindow: fix move/get position with CSD

This causes regression...

gnome-terminal: Sub menus in File, Edit are not correctly positioned any more.
gnome-panel: Same problem as above - all sub menus are positioned incorrectly.
nautilus: desktop window is positioned incorrectly.

Looks like all above has negative left and top offsets.

I think this might be problem why "tooltip: use an element name" and related theme changes was reverted.

(In reply to Alberts Muktupāvels from comment #15)
> This causes regression...
> 
> gnome-terminal: Sub menus in File, Edit are not correctly positioned any
> more.
> gnome-panel: Same problem as above - all sub menus are positioned
> incorrectly.
> nautilus: desktop window is positioned incorrectly.
> 
> Looks like all above has negative left and top offsets.
> 
> I think this might be problem why "tooltip: use an element name" and related
> theme changes was reverted.

Can you please elaborate on how to reproduce? I don't see any offset error in submenus - Is that under Wayland or X11, or other?

I am using nvidia proprietary driver, so X11.

I have full GNOME session build with jhbuild. GTK+ is current master, but other packages might not be fully up to date - max one week old.

Just login in GNOME Shell session, open terminal, and click on 'File'. Should I attach screenshot?

Ideally, a simple reproducer in C would be the best.

OK, I see what you mean with gnome-terminal...

Created attachment 314468 [details]
screenshot: nautilus desktop window

So from what I can see, it looks like these apps would presumably have a shadow but actually don't so the computation of the actual location takes the shadows into account while it should not, resulting in a negative offset...

Created attachment 314490 [details] [review]
Proposed patch

Can you check with this patch applied to gtk+?

Note that, while it should fix the issue, it also introduces a slight regression wrt the previous patches because trying to place a CSD window before it's mapped on screen will ignore the shadow, so it won't be placed at the expected location...

Offset problem is fixed with your proposed patch.

Well if there is other regressions than I would say that patches must be reverted until better fix is found, no?

(In reply to Alberts Muktupāvels from comment #24)
> Offset problem is fixed with your proposed patch.
> 
> Well if there is other regressions than I would say that patches must be
> reverted until better fix is found, no?

No, the regression I mentioned is with this new patch against the previous commit 305b34a to address the original issue, reverting these patches won't help that.

Created attachment 314509 [details] [review]
Proposed patch (Updated)

OK, let's start over. The issue was actually in gtk+ before any of these patches were pushed, but it was kept unnoticed till now.

The source of the problem is that get_shadow_width() does not return accurate values when there is no shadows apparently. Reason for this is a couple of logic mistakes in get_shadow_width() and gtk_window_should_use_csd().

Can you wipe out the previous patch and retry with this one instead?

This patch works too.

I reverted also these commits:
c4eb14eb01d5d17541fb320a9c464132e5fb6904
854c7d1f0fd98a6eaf283603adc05f1dec4f6e39
cab40f07434c7b4036660d08c974542a9868a09e

And tooltip positioning works.

Your last patch breaks CSD window shadows...

(In reply to Alberts Muktupāvels from comment #28)
> Your last patch breaks CSD window shadows...

In what sense? it works here afaiks...

(In reply to Olivier Fourdan from comment #29)
> In what sense? it works here afaiks...

I yes I see it...

GTK_CSD=1 ./tests/testgtk works

 whereas

gtk3-demo doesn't.

Created attachment 314612 [details] [review]
Proposed patch (Updated)

This updated patch seems to give consistent results, I tried with both X11 and Wayland with GTK_CSD set on X11 with regular and CSD windows, it works as expected as far as I can see (but admittedly there is a surprising number of different flags in GtkWindowPrivate which make things a tad confusing...)

Anyway, could you try this new patch?

There is same offset with nautilus desktop window.

(In reply to Alberts Muktupāvels from comment #32)
> There is same offset with nautilus desktop window.

Yes, Nautilus desktop window is not a popup window, so as far as gtk+ is concerned, it would use a shadow, that's the problem.

in nautilus-desktop-window.c, nautilus_desktop_window_constructed() does:

	gtk_window_move (GTK_WINDOW (window), 0, 0);
	gtk_window_set_decorated (GTK_WINDOW (window),
				  FALSE);

See https://git.gnome.org/browse/nautilus/tree/src/nautilus-desktop-window.c#n174

If, instead, we do:

	gtk_window_set_decorated (GTK_WINDOW (window),
				  FALSE);
	gtk_window_move (GTK_WINDOW (window), 0, 0);

Then it works, because gtk+ "knows" the window is not decorated.

We cannot fix that in gtk+ using the type hint (desktop) because Nautilus set the type hint after the move as well...

This need to be fixed in Nautilus, I'll send a patch accordingly.

(In reply to Olivier Fourdan from comment #34)
> This need to be fixed in Nautilus, I'll send a patch accordingly.

I filed bug 757471 for this.

Review of attachment 314612 [details] [review]:

ok

attachment 314612 [details] [review] pushed as commit a5b1cdd GtkWindow: Fix the shadow width logic

I have to reopen this again, unfortunately.

With the current git master code, the _gtk_window_request_csd calls in gtkmenu.c and gtktooltip.c are just ignored, and this is causing tooltips to have pointy corners, when they are supposed to be nice and rounded.

Yes, I think it's ignored because _gtk_window_request_csd() sets "csd_requested" while both menus and tooltips are popup windows that are ignored because of commit a5b1cdd

And we need commit a5b1cdd otherwise menu and tooltips are misplaced because of the negative offset induced by the shadow which is not drawn for popup windows.

I think I start to understand what is going on, even gtk_window_draw() uses the non-zero borders. But gtk_menu_draw() will draw differently.

So I think it's impossible for gtkwindow to get this right, it should therefore take client side decorations into account for positioning/resizing only for regular toplevel windows. It's actually a lot simpler and safer.

What about apps that draw their background themselves using gtk_widget_set_app_paintable(), should we ignore those as well?

(In reply to Olivier Fourdan from comment #40)
> [...]
> What about apps that draw their background themselves using
> gtk_widget_set_app_paintable(), should we ignore those as well?

Using "GTK_CSD=1 ./tests/animated-resizing" gives interesting results, but kinda show that an app that uses gtk_widget_set_app_paintable() could be drawing pretty much anywhere in its window so it's hard to conclude.

Created attachment 315184 [details] [review]
gtkwindow: css offset for toplevel only

    At the time gtk_window_move() or gtk_window_resize() get called, there
    is no way to predict if a popup window will actually draw its shadow, so
    applying an offset in this case may end up with a wrong size or
    positioning for such windows.
    
    Changing the logic in gtk_window_should_use_csd() as previously done to
    address that issue will cause some other breakage as popup windows may
    not draw a shadow but still need CSD.
    
    So best is to actually apply client side decorations offset for regular,
    top level windows only. This is actually a lot simpler and safer and
    less likely to cause additional breakage.

Attachment 315184 [details] pushed as f2b373a - gtkwindow: css offset for toplevel only

Sorry, I have to reopen this bug once again for gtk_window_set_default_size() and gtk_window_get_default_size() as well

Created attachment 315350 [details] [review]
gtkwindow: apply csd offset to set/get_default_size

An application may use gtk_window_get_size() to retrieve the current window size and later reuse that size with gtk_window_set_default_size().

gtk_window_set_default_size() and gtk_window_get_default_size() should also take client side decorations offset into account.

Related: bug 739174 (aspect ratio with CSD), bug 740922 (default_size with CSD)

(In reply to Christoph Reiter (lazka) from comment #46)
> Related: bug 739174 (aspect ratio with CSD), bug 740922 (default_size with
> CSD)

Yeah, but aspect ratio is geometry and I haven't touch those on purpose (for now).

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

Will this issue be fixed on the 3.18.x gtk release too?

Review of attachment 315350 [details] [review]:

as I said on irc, please commit this

Yeap, I was kinda waiting for an update from bug 758159 to see if this helps (as I could not reproduce) but if I guess it doesn't hurt to land it anyway, so:

attachment 315350 [details] [review] pushed as 370e346 gtkwindow: apply csd offset to set/get_default_size

Note: Leaving the bug opened for now, until the dust settles a bit.

The following example gives me three different window sizes with current trunk (compared to two with 3.18): https://paste.gnome.org/pvj6brkdz

(In reply to Christoph Reiter (lazka) from comment #52)
> The following example gives me three different window sizes with current
> trunk (compared to two with 3.18): https://paste.gnome.org/pvj6brkdz

This is to be expected, the resize functions will compensate for the gtkheaderbar only if it's known at the time the size is set.

If additional controls get added after the resize, this will cause a smaller window content and not a larger window.

Similarly, for the client-side decorations border size to be accounted for, the window need to be realized first.

And that's also true if the either the header bar or the shadows change their size after the resize occurred, those will not be compensated.

That's a lot of limitations, I know, but still an improvement as compared as before. And setting window size programatically should remain the exception rather than the rule, as window's size should be determined by its content.

(In reply to Olivier Fourdan from comment #53)
> (In reply to Christoph Reiter (lazka) from comment #52)
> > The following example gives me three different window sizes with current
> > trunk (compared to two with 3.18): https://paste.gnome.org/pvj6brkdz
> 
> This is to be expected, the resize functions will compensate for the
> gtkheaderbar only if it's known at the time the size is set.
> 
> If additional controls get added after the resize, this will cause a smaller
> window content and not a larger window.
> 
> Similarly, for the client-side decorations border size to be accounted for,
> the window need to be realized first.
> 
> And that's also true if the either the header bar or the shadows change
> their size after the resize occurred, those will not be compensated.

Could you turn this comment into a documentation patch explaining the behaviour of the API? That would be useful.

(In reply to Emmanuele Bassi (:ebassi) from comment #54)
> Could you turn this comment into a documentation patch explaining the
> behaviour of the API? That would be useful.

attachment 313733 [details] [review] had some explanation added but I agree it needs more.

Created attachment 315850 [details] [review]
gtkwindow: apply CSD adjustments to the default size when used instead of when setting it

Before the resulting window size would differ if the default size was set
before adding a headerbar vs after. Now the saved state is again the actual
requested size and it is adjusted at the time we request a window size

There is still a difference of the height for the content area because get_preferred_height() for the headerbar is returning something wrong... haven't looked at what's the problem there...

(In reply to Christoph Reiter (lazka) from comment #57)
> There is still a difference of the height for the content area because
> get_preferred_height() for the headerbar is returning something wrong...
> haven't looked at what's the problem there...

I don't think it's returning something wrong, it's returning the preferred height at the time it's called, but later buttons and label are added which may grow the header bar larger than initially computed.

At least it's hat happens in the case of Nautilus for example.

(In reply to Olivier Fourdan from comment #58)
> (In reply to Christoph Reiter (lazka) from comment #57)
> > There is still a difference of the height for the content area because
> > get_preferred_height() for the headerbar is returning something wrong...
> > haven't looked at what's the problem there...
> 
> I don't think it's returning something wrong, it's returning the preferred
> height at the time it's called, but later buttons and label are added which
> may grow the header bar larger than initially computed.

Right, it seems GtkHeaderBar needs to be realized for it's buttons to be really added and until that happens get_preferred_height() returns a wrong result.

A quick workaround might be to add a dummy image button on set_show_close_button() when the headerbar isn't realized..

Created attachment 315855 [details] [review]
gtkheaderbar: update the window buttons on ::hierarchy-changed instead of ::realize

The window button setup depends on properties of the toplevel window.
Instead of updating the setup on realize, do it when the toplevel
changes.

This makes sure that when a GtkHeaderBar is added to a window
all the widgets are present and get_preferred_height() will return
the height the widget will have when finally shown. This allows
the logic in gtkwindow to select the right window size so that
the content size will match the requested default size.

With these two patches the example from https://bugzilla.gnome.org/show_bug.cgi?id=756618#c52 works as expected in all four cases.

I tested with both attachment 315850 [details] [review] and attachment 315855 [details] [review] applied against the known regressions previously reported and found no problem with those.

Both patches look good to me and seem like an improvement overall (for example, Nautilus window does not shrink anymore upon restart with this, I guess thanks to attachment 315855 [details] [review] which helps getting a more accurate header bar height from the start).

I'll Matthias do a proper review of these patches before landing them if possible, but they have my vote.

Review of attachment 315855 [details] [review]:

Makes sense to me.

Review of attachment 315850 [details] [review]:

ok

Thanks, pushed to master.

Lets try again to close this

Reopening for a gtkfilechooser issue.

With the current state of patches, the file chooser would grow each time its closed and reopened.

Reason is gtkfilechooser saves its size to dconf in the unmap callback, which is called from the dispose callback and the heder bar is removed first, so the computed size end up wrong because of this.

Will post a couple of patches to fix ths.

Created attachment 316294 [details] [review]
gtkwindow: apply CSD in configure size request

Just like we did for the default size, that reduces the chances of having the headerbar missing when computing the client side decorations controls.

Created attachment 316295 [details] [review]
gtkwindow: remove headerbar after disposing parent

Widgets such as gtkfilechooser may be saving their size and position on the unmap callback, if the client-side decoration header bar is removed first, the reported size will be wrong.

Review of attachment 316294 [details] [review]:

::: gtk/gtkwindow.c
@@ +5105,2 @@
   if (!priv->decorated ||
+      !priv->client_decorated ||

Why is that needed?

Review of attachment 316294 [details] [review]:

::: gtk/gtkwindow.c
@@ +5105,2 @@
   if (!priv->decorated ||
+      !priv->client_decorated ||

Why is that needed?

Created attachment 316309 [details] [review]
gtkwindow: apply CSD in configure size request

(In reply to Christoph Reiter (lazka) from comment #71)
> Review of attachment 316294 [details] [review] [review]:
> 
> ::: gtk/gtkwindow.c
> @@ +5105,2 @@
>    if (!priv->decorated ||
> +      !priv->client_decorated ||
> 
> Why is that needed?

I agree it is not needed, it was meant as an optimization because this routine is called for ssd as well.

But I guess it's possible to have a header bar with ssd so it's safer to leave that change alone, updated patch attached.

Review of attachment 316295 [details] [review]:

hmm, a bit ugly, but ok.

Review of attachment 316309 [details] [review]:

This one looks dangerous to me - it brings in this size adjustment in a lot of fiddly code paths. Has it been tested thoroughly ?

(In reply to Matthias Clasen from comment #74)
> Review of attachment 316309 [details] [review] [review]:
> 
> This one looks dangerous to me - it brings in this size adjustment in a lot
> of fiddly code paths. Has it been tested thoroughly ?

resize_width/height is only set in gtk_window_resize(_to_geometry) and read in gtk_window_compute_configure_request_size so this only adds the adjustment to the gtk_window_resize_to_geometry path compared to master.

attachment 316295 [details] [review] pushed as commit 103d369 gtkwindow: remove headerbar after disposing parent

Review of attachment 316309 [details] [review]:

couldn't make this break anything in some testing

Review of attachment 316309 [details] [review]:

attachment 316309 [details] [review] pushed as commit e933233 gtkwindow: apply CSD in configure size request

Created attachment 316650 [details] [review]
gtkwindow: Document further resize with csd

(In reply to Olivier Fourdan from comment #55)
> (In reply to Emmanuele Bassi (:ebassi) from comment #54)
> > Could you turn this comment into a documentation patch explaining the
> > behaviour of the API? That would be useful.
> 
> attachment 313733 [details] [review] [review] had some explanation added but I agree
> it needs more.

This is an attempt at documenting this pitfall.

Note that with the recent commit e933233 and commit 308aec5, chances of getting the CSD sizes right are greatly increased.

Review of attachment 316650 [details] [review]:

sure, thanks

Review of attachment 316650 [details] [review]:

attachment 316650 [details] [review] pushed as commit de41389 gtkwindow: Document further resize with csd

To paraphrase Matthias, "Lets try again to close this".

Review of attachment 315855 [details] [review]:

This patch introduced regression. Now _gtk_header_bar_update_window_buttons is called before gtk_application_set_app_menu... So if gtk-shell-shows-app-menu == FALSE applications with appmenu does not show it (at least initially).

Created attachment 317944 [details] [review]
gtkheaderbar: update windows buttons also on realize

Review of attachment 317944 [details] [review]:

Please include the description on why this is needed in the commit message.

otherwise lgtm

Created attachment 317981 [details] [review]
gtkheaderbar: update window buttons also on realize

Review of attachment 317981 [details] [review]:

thanks

It introduced a weird regression in several apps, see bug #761978.

(In reply to Michael Catanzaro from comment #88)
> It introduced a weird regression in several apps, see bug #761978.

Most likely these apps try to save their size using a method that doesn't take decorations into account (e.g. gtk_widget_get_allocation) and restore it using another method which does (e.g. gtk_window_set_default_size), thus causing the window to grow.

See bug 759117 for an example of what I mean.