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 730850 - Dropping a 2nd tab into a window makes it shrink
Dropping a 2nd tab into a window makes it shrink
Status: RESOLVED FIXED
Product: gnome-terminal
Classification: Core
Component: general
git master
Other Linux
: Normal minor
: ---
Assigned To: GNOME Terminal Maintainers
GNOME Terminal Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-05-27 19:12 UTC by Egmont Koblinger
Modified: 2014-05-29 12:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix (820 bytes, patch)
2014-05-28 13:06 UTC, Egmont Koblinger
needs-work Details | Review

Description Egmont Koblinger 2014-05-27 19:12:10 UTC
Have multiple gnome-terminal windows, one with a single tab (no tab bar present), one with more tabs.  Drag and drop one of the tabs into the window that has only one tab.

Notice that the window shrinks.  Expected: the terminal window should stay at the same grid size (i.e. enlarge its size in pixels to accomodate the new tab), exactly as when a new tab is opened with Ctrl+Shift+T.

Reproducible in gnome-shell and compiz/unity.
Comment 1 Christian Persch 2014-05-27 19:21:11 UTC
I guess the reverse is also true, if you drag off one tab from a window with exactly two, the window doesn't shrink?

Looks like missing terminal_window_update_size calls in the notebook's page added/removed handlers...
Comment 2 Egmont Koblinger 2014-05-27 20:32:10 UTC
> I guess the reverse is also true, if you drag off one tab from a window with
> exactly two, the window doesn't shrink?

Interestingly, not :)

> Looks like missing terminal_window_update_size calls in the notebook's page
> added/removed handlers...

Yup my guess is the same, we need to find some code that's already there when a tab is opened/closed, and copy it to when it's dragged.
Comment 3 Egmont Koblinger 2014-05-28 13:06:34 UTC
Created attachment 277388 [details] [review]
fix

There were at least two places where the window size was correctly set:
- when a tab was closed and only 1 remained
- when a tab was made active (this is necessary e.g. when font sizes differ)

but it was not done when a 2nd tab was added.

It means that dragging away a tab actually set the correct size twice (once because another tab had to be made active, and once because there was only 1 tab remaining). Opening another tab with Ctrl+Shift+T "accidentally" set the size because this new tab was made active.

On a slightly related note, I think it would better if a drag-n-dropped tab would become active in its new window – any opinions here?
Comment 4 Egmont Koblinger 2014-05-29 11:04:48 UTC
Comment on attachment 277388 [details] [review]
fix

d3e593e
Comment 5 Christian Persch 2014-05-29 11:57:47 UTC
Sorry, but I don't like this patch. It's totally unobvious why this fixes the problem, and why this is necessary only here for tab-added not for tab-removed. 

Adding the tab to the window will make it the current tab, and the the handler for that updates the size already. So why is this not enough?
Comment 6 Christian Persch 2014-05-29 12:01:02 UTC
(In reply to comment #3)
> On a slightly related note, I think it would better if a drag-n-dropped tab
> would become active in its new window – any opinions here?

Hmm it does become active here...
Comment 7 Egmont Koblinger 2014-05-29 12:02:26 UTC
(In reply to comment #5)
> Sorry, but I don't like this patch. It's totally unobvious why this fixes the
> problem, and why this is necessary only here for tab-added not for tab-removed. 

For tab-removed (mdi_screen_removed_cb()) this code is already there.

> Adding the tab to the window will make it the current tab, and the the handler
> for that updates the size already. So why is this not enough?

It's not made the current when you drop it in the terminal area (rather than on the tab bar).

I'd vote for changing this behavior, but even then I think it's good to have the current patch.
Comment 8 Christian Persch 2014-05-29 12:08:08 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > Sorry, but I don't like this patch. It's totally unobvious why this fixes the
> > problem, and why this is necessary only here for tab-added not for tab-removed. 
> 
> For tab-removed (mdi_screen_removed_cb()) this code is already there.

I missed this. Ok then, I guess this is good enough. Re-committed.

> > Adding the tab to the window will make it the current tab, and the the handler
> > for that updates the size already. So why is this not enough?
> 
> It's not made the current when you drop it in the terminal area (rather than on
> the tab bar).
> 
> I'd vote for changing this behavior, but even then I think it's good to have
> the current patch.

Ah, right, I tested by dropping on the tab bar. Yes, let's make this consistent.