GNOME Bugzilla – Bug 787710
testapps report incorrect size
Last modified: 2017-09-29 12:44:59 UTC
In both test apps, the size reported by the window manager does not match the actual size. E.g. "./src/testvte" comes up with 78x24 (the actual value, also as reported by "stty size"), however, upon dragging the window's edge, my Unity7 reports 80x24. (Found this while working on https://github.com/gnunn1/tilix/issues/1122. Not sure yet if the two issues are related, but I'd guess they are.)
Same in GNOME Shell.
Summary of the "main" issue in the other bug: In Tilix, "top" falls apart, every second line is empty, and hence the upper half (status bar etc.) get scrolled out. As if it printed lines that are a bit longer than the terminal. Manually examining the size doesn't explain this though; the actual width, the one reported by "stty size" and the typescript of "top" match each other. So I'm puzzled here. There's also a scenario where the window is made wider, this is confirmed by rewrap-on-resize kicking in, yet "stty size" reports the same size before and after. Totally weird. How come we see bugs in the two test apps as well as Tilix, but not in the other plenty of frontends? Christian, I'm afraid this one will be a quite tough one, requiring deep Gtk+ knowledge around geometry handling. Any help is appreciated, I'm hardly familiar with this area.
According to downstream investigation, the real Tilix bug ("top" falling apart, VTE not having a coherent idea of its size??) is related to GtkScrolledWindow / overlay scrollbar. This was said to be unsupported in bug 760718. See also bug 766569. (I'm just linking these bugs, I have no idea what the actual problem is.) That being said, the two test apps reporting invalid geometry is still an issue (they don't use GtkScrolledWindow, do they?). I still have no clue if these two are related or not.
The main Tilix bug turned out to be mostly irrelevant.
Bug 783247 might be relevant.
Yes, need to bring over the g-t changes from bug 760944, probably.
You mean https://git.gnome.org/browse/gnome-terminal/commit/?id=88bd325, right? Doesn't look trivial :(
Should be fixed on master with the new test application (which is the only one that matters going forward; the old C test app was removed, and the vala test app is noinst and only there to check the vala bindings work.)
./src/app/vte-2.91, right? It starts with 80x24 but does not enable geometry hints. There's no cmdline switch to enable it, there's one to disable (something that's disabled to begin with), causing the initial window to be unusably tiny.
There's the big "if" above gtk_window_set_geometry_hints. The 7 conditions, one per line, each line corresponds to a new occasion this "if" is reached, are: 0 1 1 1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 1 0 0 0 0 0 0 1 0 0 0 0 0 0 So the first time char_{width,height} and chrome_{width_height} are the ones that differ, they get cached after this "if", but the window is not yet mapped so the hints aren't set. Later the same method is called a few more times (not sure why), but the actual values no longer differ from the cached ones so gtk_window_set_geometry_hints() doesn't get called even when the window gets mapped. Removing all these stuff != cached_stuff conditions from the "if" and leaving only gtk_widget_get_realized() solves the problem for me. I guess gtk_window_set_geometry_hints() should be idempotent so it's okay, right? This is all in Ubuntu's Unity 7 as usual; this time on Artful beta.
So, with the change I outlined above: - Normally geom hints get enabled, and the size reported by the WM matches "stty size", so this is okay. - --no-geometry-hints still starts with a crazily small window - Padding larger than the default 1px (from css) causes the window to be smaller than 80x24, the pixel size is roughly the same as the 80x24 size would be with the default 1px padding. This bug is also present in g-t. I guess I should file a new bug, right?
The code is just the code from gnome-terminal re-formulated, the condition is in g-t (even more complicated since I omitted the padding check). I gave up trying to make sense of it :-) and I'm not sure that setting the geometry hints when they haven't changed doesn't mean extra, unncessary work, for the compositor (gnome-shell)... I tested this here with: $ ./vte-2.91 --geometry "123x45" # gives correctly sized window as per 'stty size' $ ./vte-2.91 --extra-margin 10 # still gives 80x24 window $ ./vte-2.91 --allow-window-ops # (which maybe should be enabled by default) then doing $ ../window 8 45 123 # to use the OSC command to resize, gives windows size 123x45 as expected And finally using Control+KP_Plus/Minus to change the font size keeps the window size as well, until the window reaches the screen size. This is all on gnome-shell from F25 (version 3.22.3) on X. No idea what's up with the CSS theming; I don't see how the code could be any different whether it's 1px all around or anything else... ?
Let's keep this all in one bug here.
(In reply to Christian Persch from comment #12) > and I'm not sure that setting the geometry hints when they haven't changed > doesn't mean extra, unncessary work, for the compositor (gnome-shell)... If it's just a one-time extra cost without side effects that happens to fix/workaround(?) Unity7 then I'd be up for it. > I tested this here with: > $ ./vte-2.91 --geometry "123x45" > # gives correctly sized window as per 'stty size' For me too. In Unity there's no geometry hints set at this point, resizing is per-pixel. In gnome-shell the geometry is correctly set. > $ ./vte-2.91 --extra-margin 10 > # still gives 80x24 window Works for me as expected in both WMs. The CSS padding is broken for me in both, though. So something is different between these two. Could you please try the padding from CSS? > $ ./vte-2.91 --allow-window-ops > # (which maybe should be enabled by default) then doing > $ ../window 8 45 123 > # to use the OSC command to resize, gives windows size 123x45 as expected For me too. > And finally using Control+KP_Plus/Minus to change the font size keeps the > window > size as well, until the window reaches the screen size. Changing the font size enables the geometry hints on Unity7. Also, --no-geometry-hints starts up with a tiny terminal in gnome-shell too, could you please try this one?
The --no-geometry-hints case should be fixed now. I tried to also fix the CSS padding case, but gave up :-( I'm tempted to just declare setting the padding to a non-default value as unsupported.
(In reply to Christian Persch from comment #15) > I'm tempted to > just declare setting the padding to a non-default value as unsupported. Don't; I'll help track it down.
A bit of personal context to that "don't": I don't like overlay scrollbars in terminals; at least not when they overlap actual content to the extent that characters aren't clearly readable individually. At this moment I don't have overlay scrollbar. I used to have (when Ubuntu shipped their own hack), and I worked it around not to be an overlay one by adding an extra right padding (so technically it was still overlaid, just over the padding). With the pending GtkScrolledWidget story I'm afraid there's a good chance for some different overlay scrollbar variant to come back one day and then I'll need this workaround again.
(In reply to Christian Persch from comment #15) > The --no-geometry-hints case should be fixed now. Yup, confirmed (unity7), thanks!
Let's also not forget about bug 756024 (geometry problems with padding: 0px) :-)
If the padding (as defined in vtegtk.cc gtk_css_provider_load_from_data() or the CSS file) matches the default_padding (vtegtk.hh) then the behavior is what's expected. The amount of the geometry mismatch is related to the difference between these two. I guess the style is set too late, later than the geometry is computed, and as such the geometry is still calculated from the default_padding rather than the CSS one. To be cont'd...
To clarify/correct the conclusion: Size reporting and all further resizing goes as expected, so the geometry hints are apparently set up correctly (whenever they are set up at all :-P). It's the initial window size in pixels that's apparently erroneously computed based on default_padding rather than the actual padding.
With your recent debug stuff added, here's the log with 9x18 font size and 2px padding: $ VTE_DEBUG=widget-size ./src/app/vte-2.91 VTE debug flags = 200000 [Terminal 0x55779d35b320] minimum_height=20, natural_height=434 for 80x24 cells (padding 1,1;1,1). [Terminal 0x55779d35b320] minimum_width=11, natural_width=722 for 80x24 cells (padding 1,1;1,1). Setting padding to (2,2,2,2) [Terminal 0x55779d35b320] minimum_height=22, natural_height=436 for 80x24 cells (padding 2,2;2,2). [Terminal 0x55779d35b320] minimum_width=13, natural_width=724 for 80x24 cells (padding 2,2;2,2). [Terminal 0x55779d35b320] Sizing window to 722x434 (79x23, padding 2,2;2,2). [...snip...] It's pretty obvious that "stuff" is being done with the hardwired default padding before reading the style. I've no clue yet what that "stuff" is :) Placing a widget_style_updated() at the end of VteTerminalPrivate's constructor fixes the problem. (I'm afraid it's ugly, probably not the best place to put it.) In the mean time, it seems to me that this makes it unnecessary to call it again at the end of widget_realize().
Well, that "stuff" is Gtk+ calling our callbacks. I'm not sure if there's another callback it calls earlier than the ones that return the geometry. I'm not even sure how to figure that out conveniently (without putting a debug statement in each). And even if we find one, I really cannot tell if Gtk+ guarantees to call our callbacks in that order in further releases as well. So... maybe the constructor is the best / safest place to initialize the style?
Created attachment 360568 [details] [review] Fix v1 Patch for the previously outlined approach.
Created attachment 360569 [details] [review] Fix v2 Or how about the constructed() method?
Let's go for v2. + // An attempt to fix bug 787710. Elaborate a bit more (at least don't say 'attempt' ;-) ), and also mention bug 727614 (which I guess is still fixed with this, right) ? + if (G_OBJECT_CLASS (vte_terminal_parent_class)->constructed) { + G_OBJECT_CLASS (vte_terminal_parent_class)->constructed (object); No need for the check, it's always true. With these nits fixed, please commit. Thanks!
Created attachment 360635 [details] [review] Fix v3 v2 was totally draft, the idea that there must be some constructed()-like method occurred to me at 0:40am-ish when I was already quite sleepy, and the patch was ready at 0:54 :-) I guess I was asleep by 1am. > also mention bug 727614 (which I guess is still fixed with this, right) ? I can no longer reproduce that bug (even if I remove that vte_terminal_style_updated() that was introduced there). Removing that but not adding the current one would introduce a new regression though: the window size keeps shrinking each time a new g-t tab is opened (at nondefault padding). This doesn't occur with the new fix. So I believe it's safe to say that we're at least as good as before. > No need for the check, it's always true. But I assume I still have to call the parent class's method, right :) I'd appreciate if you could pls take another quick look. In the mean time I'll test if it's okay to cherry-pick to 0-50 and maybe even 0-48 (can't recall if there were any geometry-related changes since then). Or shall we maybe wait with cherry-picking (maybe to 0.50.2?) to give it a bit more testing?
On a side note, bug 756024 (0px padding issue) isn't fixed by this patch.
Comment on attachment 360635 [details] [review] Fix v3 Yes, you do need to chain up. Looks fine, also for stable 0.50 and 0.48, if you want to. Thanks!
Submitted, closing. I've forked off the pending "no geometry initially" issue to bug 788334. Quick recap: This bug started as testapps reporting incorrect size, but this got obsoleted by a new test app replacing them. In the mean time another bug: incorrect initial size with non-default padding was discovered in comment 9, and this got fixed now.