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 592435 - --geometry not working
--geometry not working
Status: RESOLVED FIXED
Product: gnome-terminal
Classification: Core
Component: general
2.27.x
Other Linux
: Normal normal
: ---
Assigned To: GNOME Terminal Maintainers
GNOME Terminal Maintainers
fixed-2-28
: 593078 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2009-08-20 09:46 UTC by Yanko Kaneti
Modified: 2010-10-10 00:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Make geometry parsing more robust (5.04 KB, patch)
2009-08-31 21:15 UTC, Owen Taylor
none Details | Review
Make geometry parsing more robust (5.04 KB, patch)
2010-10-08 20:29 UTC, Owen Taylor
committed Details | Review

Description Yanko Kaneti 2009-08-20 09:46:12 UTC
Since upgrading to the fedora gnome-terminal-2.27.91-1.fc12.x86_64 package, the --geometry option not longer works.
Bisected to this
http://git.gnome.org/cgit/gnome-terminal/commit/?id=b2e682189b32dda2b0eca105fe1ee057e5e8c544
Avoid changing the size during realize

cc Owen
Comment 1 Christian Persch 2009-08-25 20:56:16 UTC
*** Bug 593078 has been marked as a duplicate of this bug. ***
Comment 2 Owen Taylor 2009-08-25 21:08:04 UTC
Just a quick note to the gnome-terminal maintainers - investigating and fixing this is not a priority for me, so it would probably be good if someone else looked into this.

(About the only thing that would inspire me to look at this would be a threat to revert my patch and go back to the previous whacky way of doing things. I will be grumpy if that threat is made.)
Comment 3 Christian Persch 2009-08-26 12:22:20 UTC
Reverting b2e682189b32dda2b0eca105fe1ee057e5e8c544 does indeed fix the problem.

"The observed effect is that depending on timing, the terminal will be reset to 80x24 when switching to a compositing window manager."

So this commit makes the rather esoteric case of switching window managers work but breaks --geometry. I don't think that's a good trade; therefore I'm indeed considering reverting this commit for now.
Comment 4 Owen Taylor 2009-08-26 16:06:34 UTC
Setting the size in a realize callback (what the code was doing before) is a really bad thing to do from a GTK+ perspective. It's completely undefined and unsupported.

My hope would be that other options would be present other than "revert" and "not revert" - like debugging what's going on....
Comment 5 Roland Dreier 2009-08-31 05:20:39 UTC
This bug is rather annoying to me -- it means I have to manually resize all my terminal windows rather than being able to specify the size from the launcher.

Anyway, I tried to debug this but got lost before I got all the way to the bottom of it.  Some findings: the old code called terminal_window_set_size() before gtk_window_parse_geometry(), and the new code after Owen's patch calls it after.  What seems to be happening is that with the old code, gtk_window_parse_geometry() gets a chance to set the default geometry after terminal_window_set_size() calculates its default based on the default terminal screen, while in the new code, the geometry gets parsed but that default size gets overwritten.

Simply removing the call to terminal_window_set_size() from where Owen's patch moved it does make --geometry start working again.

Owen, I do think your attitude is a little off here -- your patch introduced a regression that looks worse than the bug it fixed (and in any case trading a new bug for an old one is really never a good trade), and you say fixing this regression is not a priority for you.  I don't think it's reasonable to expect someone else to debug the problem for you, rather than simply reverting your broken patch.
Comment 6 Owen Taylor 2009-08-31 21:11:34 UTC
Christian checked a fix into HEAD a few days ago and merged it into the gnome-2-28 branch today.

http://git.gnome.org/cgit/gnome-terminal/commit/?h=gnome-2-28&id=9f8b6a3001b5c62e95367131ae9c7bc38a26c751

Looking at the code, the basic problem with geometry is that gtk_window_parse_geometry() is essentially only queueing up the new size to be applied later. If terminal_window_set_size() gets called before the window is actually shown, then that queued up size will get overwritten based on the current size of the active terminal.

Christian's fix looks reasonable to me and should work - with the code flow in gnome-terminal, terminal_window_set_size() will have been called earlier, and since nothing will have changed since then, it doesn't need to be called again.

But it's a little fragile against future modifications - if ordering was reworked, then things could conceivably break. I'll attach a cleanup patch against the HEAD branch that:

 - Backs out Christian's change
 - Does things a slightly more robust way and adds comments explain why
Comment 7 Owen Taylor 2009-08-31 21:15:16 UTC
Created attachment 142166 [details] [review]
Make geometry parsing more robust

Instead of counting on terminal_window_set_size() not being called
between gtk_window_parse_geometry(), find out the grid size that
gtk_window_parse_geometry() determined and set that on the active
terminal. Then if terminal_window_set_size() is called, it will get
the correct new size instead of the old one.

terminal_window_parse_geometry() is added to encapsulate this logic.

This replaces the fix in commit 549e45a
Comment 8 Owen Taylor 2009-08-31 21:16:27 UTC
Christian checked a fix into HEAD a few days ago and merged it into the gnome-2-28 branch today.

http://git.gnome.org/cgit/gnome-terminal/commit/?h=gnome-2-28&id=9f8b6a3001b5c62e95367131ae9c7bc38a26c751

Looking at the code, the basic problem with geometry is that gtk_window_parse_geometry() is essentially only queueing up the new size to be applied later. If terminal_window_set_size() gets called before the window is actually shown, then that queued up size will get overwritten based on the current size of the active terminal.

Christian's fix looks reasonable to me and should work - with the code flow in gnome-terminal, terminal_window_set_size() will have been called earlier, and since nothing will have changed since then, it doesn't need to be called again.

But it's a little fragile against future modifications - if ordering was reworked, then things could conceivably break. I've attach a cleanup patch against the HEAD branch that:

 - Backs out Christian's change
 - Does things a slightly more robust way and adds comments explain why

It seems to work in my testing. Probably doesn't need to be merged into the 2-28 branch.
Comment 9 Martin Olsson 2009-09-04 09:29:15 UTC
Thanks for making the effort to fix this properly. If this bug is now fixed please mark the bug as resolved so that downstream packagers etc get notified etc.
Comment 10 Martin Olsson 2009-09-11 16:40:21 UTC
Christian Persch, is this bug fixed or not? Was the fix part of the RC tarballs?
Comment 11 Christian Persch 2009-09-21 13:34:05 UTC
This seems fixed on 2-28 and trunk. I'm leaving the bug open while I consider the remaining patch (attachment 142166 [details] [review]).
Comment 12 Owen Taylor 2010-10-08 20:29:30 UTC
Created attachment 171967 [details] [review]
Make geometry parsing more robust

Rebased to HEAD
Comment 13 Christian Persch 2010-10-08 21:08:24 UTC
Comment on attachment 171967 [details] [review]
Make geometry parsing more robust

Looks good to commit, just one nit:

@@ -2280,24 +2280,20 @@ terminal_window_show (GtkWidget *widget)
 
   gtk_widget_get_allocation (widget, &widget_allocation);
 
-#if 0
   TerminalWindowPrivate *priv = window->priv;

Need to move the |priv| declaration up (for -Wdeclaration-after-statement).
Comment 14 Owen Taylor 2010-10-10 00:33:58 UTC
Pushed with the fix you pointed out and another small fix that showed up
compiling against GTK+ 3 (patch was using widget->allocation.width
rather than widget->allocation_width in the debug printf)

Attachment 171967 [details] pushed as c304285 - Make geometry parsing more robust