Bug 172042 - Konsole window resizes on new tab to last resized window
Summary: Konsole window resizes on new tab to last resized window
Status: RESOLVED FIXED
Alias: None
Product: konsole
Classification: Applications
Component: general (show other bugs)
Version: 2.2
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: Konsole Developer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-02 16:22 UTC by Thomas Zander
Modified: 2008-10-15 01:08 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Restore window size only once (1.27 KB, patch)
2008-10-14 14:28 UTC, Stefan Becker
Details
Restore window size only once (1.25 KB, patch)
2008-10-14 18:05 UTC, Stefan Becker
Details
Restore window size only once, but allow resize from session restore (1.41 KB, patch)
2008-10-14 20:03 UTC, Stefan Becker
Details
Only restore window size when needed (1.89 KB, patch)
2008-10-14 22:42 UTC, Rafael Fernández López
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Zander 2008-10-02 16:22:49 UTC
Version:           2.2 (using 4.1.68 (KDE 4.1.68 (KDE 4.2 >= 20080926)), compiled sources)
Compiler:          gcc
OS:                Linux (i686) release 2.6.25-2-686

First note that I can reproduce this bug in both 4.1 branch and trunk (4.2)

To reproduce;

start a konsole
start a second konsole window.

Scale window one to be a little smaller.
Create a new tab in the second konsole.

What happens is that I see the second window with its new tab change size. It takes on the size of the last resized konsole window.

What I expected is that konsole never resizes my window automatically after the initial opening of the window.
Comment 1 David Faure 2008-10-02 18:09:42 UTC
Ah, -that- is why my konsole window keeps resizing itself. Good catch. The question is where is the applyMainWindowSettings call that triggers the resizing (can't be the one called by setAutoSaveSettings, since that's done only when creating a new window).
Comment 2 Robert Knight 2008-10-02 22:37:14 UTC
I can confirm this.  I wish Bugzilla wouldn't automatically confirm bugs from certain users.
Comment 3 Stefan Becker 2008-10-12 17:31:44 UTC
Ahhh, so once again I'm not imagining things *AND* this has nothing to do with the fix for bug #152761 which has been backported to 4.1.2 for Fedora 9/10.
Comment 4 Stefan Becker 2008-10-13 20:17:03 UTC
I digged a little into this problem. I added some debugging output to the functions:

  TerminalDisplay::calcGeometry()        [called when TD receives a resizeEvent]
  TabbedViewContainerV2::setActiveView() [called when a new tab gets created]

Setup: one window open, open new window, create new tab in first window:

konsole(23390): Session::Session() 
konsole(23390): TVCV2::setActiveView() BEFORE setCurrentWidget() 
konsole(23390): TD::calcGeometry() BEGIN 1 x 1 ( QRect(0,0 595x462) ) 
konsole(23390): TD::calcGeometry() END 82 x 28 
konsole(23390): TVCV2::setActiveView() BEFORE setCurrentIndex() 
konsole(23390): TVCV2::setActiveView() END 

-> OK, no resize

Test step: resize second window, create new tab in first window:

konsole(23390): Session::Session() 
konsole(23390): TVCV2::setActiveView() BEFORE setCurrentWidget() 
konsole(23390): TD::calcGeometry() BEGIN 1 x 1 ( QRect(0,0 595x462) ) 
konsole(23390): TD::calcGeometry() END 82 x 28 
konsole(23390): TVCV2::setActiveView() BEFORE setCurrentIndex() 
konsole(23390): TD::calcGeometry() BEGIN 82 x 28 ( QRect(0,0 595x654) ) 
konsole(23390): TD::calcGeometry() END 82 x 40 
konsole(23390): TVCV2::setActiveView() END 

-> first window resized to be the same size as second window

So I think it is narrowed down to the call "_tabBar->setCurrentIndex(index)" that for some reason causes the window to resize. Not much yet, but maybe someone has a brilliant idea what causes this...
Comment 5 Stefan Becker 2008-10-13 22:49:44 UTC
What I can't figure out: where does konsole store the information "this is the default window size"?

That information gets obviously updated when you resize a window. A newly opened window will use this size, which is OK. Unfortunately the same information is also used when a new tab is added, i.e. the window with the new tab gets resized to the "default size".

This information must be stored in some shared data structure, but where? As far as I have been able to figure out these are the shared data structures inside the konsole code:

    Application
    SessionManager
    Profile        (if all tabs use the same profile)
    environment variable

From my poking around I'm pretty sure that the information is not stored in Profile or an environment variable.

Is this maybe caused by some "feature" of the base class KUniqueApplication?

Anyone?
Comment 6 Ivo Anjo 2008-10-13 22:59:20 UTC
I might be wrong, but afaik KXmlGuiWindow is a subclass of KMainWindow, and KMainWindow supposedly saves the window size and position.
Comment 7 Robert Knight 2008-10-13 23:08:39 UTC
Hi Stefan,

As Ivo mentioned, KMainWindow takes care of setting remembering the size of a Konsole window between closing it and opening it again.  This is true of most KDE apps.  The settings will be stored in the global Konsole settings file (~/.kde/share/config/konsolerc)

The only special case is for the first time Konsole is started by a user, in which case it defaults to ~80 columns by ~40 rows in the default font.  This size is set in ViewManager::createView().  This sets the size hint for the TerminalDisplay widget and Qt's layouting takes care of making the window the right size.
Comment 8 Robert Knight 2008-10-13 23:15:23 UTC
Had a quick dig.  It looks like something changed in kdelibs recently.  The backtrace which leads to the first window being resized, starting from the point where the new tab becomes active (and its SessionController emits a focused() signal) is:

#13 0xb7dd8e9e in KMainWindow::restoreWindowSize (this=0x98b5c30, config=@0xbff854d4)                     
    at /home/robert/kde4/src/trunk/kdelibs/kdeui/widgets/kmainwindow.cpp:888                              
#14 0xb7dd8fcb in KMainWindow::applyMainWindowSettings (this=0x98b5c30, cg=@0xbff854d4, force=false)      
    at /home/robert/kde4/src/trunk/kdelibs/kdeui/widgets/kmainwindow.cpp:711                              
#15 0xb7e15892 in KXmlGuiWindow::applyMainWindowSettings (this=0x98b5c30, config=@0xbff854d4,             
    force=<value optimized out>) at /home/robert/kde4/src/trunk/kdelibs/kdeui/xmlgui/kxmlguiwindow.cpp:346
#16 0xb7e15a2d in KXmlGuiWindow::finalizeGUI (this=0x98b5c30)                                             
    at /home/robert/kde4/src/trunk/kdelibs/kdeui/xmlgui/kxmlguiwindow.cpp:338                             
#17 0xb7e13c56 in KXMLGUIBuilder::finalizeGUI (this=0x98b5c48)                                            
    at /home/robert/kde4/src/trunk/kdelibs/kdeui/xmlgui/kxmlguibuilder.cpp:409                            
#18 0xb7e15847 in KXmlGuiWindow::finalizeGUI (this=0x98b5c30, client=0x99bb58c)                           
    at /home/robert/kde4/src/trunk/kdelibs/kdeui/xmlgui/kxmlguiwindow.cpp:355                             
#19 0xb7e20bc0 in KXMLGUIFactory::addClient (this=0x9880560, client=0x99bb58c)                            
    at /home/robert/kde4/src/trunk/kdelibs/kdeui/xmlgui/kxmlguifactory.cpp:272                            
#20 0xb7f5102e in Konsole::MainWindow::activeViewChanged (this=0x98b5c30, controller=0x99bb578)           
    at /home/robert/kde4/src/trunk/kdebase/apps/konsole/src/MainWindow.cpp:203                            
#21 0xb7f5122b in Konsole::MainWindow::qt_metacall (this=0x98b5c30, _c=QMetaObject::InvokeMetaMethod,     
---Type <return> to continue, or q <return> to quit---                                                    
    _id=9, _a=0xbff8575c) at /home/robert/kde4/build/trunk/kdebase/apps/konsole/src/MainWindow.moc:106
#22 0xb75f3cbb in QMetaObject::activate (sender=0x98720f0, from_signal_index=6, to_signal_index=6,
    argv=0xbff8575c) at kernel/qobject.cpp:3031
#23 0xb75f4245 in QMetaObject::activate (sender=0x98720f0, m=0xb7f66230, local_signal_index=2,
    argv=0xbff8575c) at kernel/qobject.cpp:3101
#24 0xb7f3e653 in Konsole::ViewManager::activeViewChanged (this=0x98720f0, _t1=0x99bb578)
    at /home/robert/kde4/build/trunk/kdebase/apps/konsole/src/ViewManager.moc:172
#25 0xb7f3f313 in Konsole::ViewManager::controllerChanged (this=0x98720f0, controller=0x99bb578)
    at /home/robert/kde4/src/trunk/kdebase/apps/konsole/src/ViewManager.cpp:522
#26 0xb7f4288f in Konsole::ViewManager::qt_metacall (this=0x98720f0, _c=QMetaObject::InvokeMetaMethod,
    _id=28, _a=0xbff858bc) at /home/robert/kde4/build/trunk/kdebase/apps/konsole/src/ViewManager.moc:147
#27 0xb75f3cbb in QMetaObject::activate (sender=0x99bb578, from_signal_index=9, to_signal_index=9,
    argv=0xbff858bc) at kernel/qobject.cpp:3031
#28 0xb75f4245 in QMetaObject::activate (sender=0x99bb578, m=0xb7f65870, local_signal_index=0,
    argv=0xbff858bc) at kernel/qobject.cpp:3101
#29 0xb7f17973 in Konsole::SessionController::focused (this=0x99bb578, _t1=0x99bb578)
    at /home/robert/kde4/build/trunk/kdebase/apps/konsole/src/SessionController.moc:185
#30 0xb7f1cc81 in Konsole::SessionController::eventFilter (this=0x99bb578, watched=0x99cd2a8,
    event=0xbff85d28) at /home/robert/kde4/src/trunk/kdebase/apps/konsole/src/SessionController.cpp:268
Comment 9 Thomas Zander 2008-10-13 23:38:33 UTC
When I see this;

#19 0xb7e20bc0 in KXMLGUIFactory::addClient (this=0x9880560, client=0x99bb58c)
    at /home/robert/kde4/src/trunk/kdelibs/kdeui/xmlgui/kxmlguifactory.cpp:272
#20 0xb7f5102e in Konsole::MainWindow::activeViewChanged (this=0x98b5c30, controller=0x99bb578)          ¬
    at /home/robert/kde4/src/trunk/kdebase/apps/konsole/src/MainWindow.cpp:203  

I think I should point out that the API docs give me the distinct impression you are not suppost to call addClient for each tab. But instead call it once per main window.
Comment 10 David Faure 2008-10-14 01:21:42 UTC
Calling addClient to add the GUI of a given tab, is correct xmlgui usage.

The reason for the applyMainWindowSettings call in finalizeGUI, added by ereslibre recently, is to handle hiding/showing toolbars when switching components (in konq, kontact etc.) correctly. Indeed it wasn't meant to resize windows, AFAIK. But applyMainWindowSettings resizes, by default (makes sense on startup, but not really later on).

I think we need a new flag in KMainWindow::applyMainWindowSettings, or to split it up, so that kxmlguiwindow could say "apply toolbar settings", but not "and restore window size". Splitting up is better than a flag (especially for a virtual method...).
Another (easier) solution might be to remember "I have restored window size already" and not do it twice for a given kmainwindow; ereslibre, do you see a use case where this might break?
Comment 11 Rafael Fernández López 2008-10-14 11:21:23 UTC
I don't fully like the solution of adding another flag to applyMainWindowSettings (or overload the method with a new flag...).

> Another (easier) solution might be to remember "I have restored window size
> already" and not do it twice for a given kmainwindow; ereslibre, do you see a
> use case where this might break?

I think this solution would be nice, and I can't think at the moment of any use case that could break this behavior. When get home (if not yet fixed) I will give it a shot.

Comment 12 Stefan Becker 2008-10-14 14:28:56 UTC
Created attachment 27872 [details]
Restore window size only once

OK, what about this change? Now konsole windows don't resize anymore when you add a new tab.

Only problem I noticed is that for some reason the last resize is lost and konsolerc always contains the same size. So even if you change all windows to be a new size and then quit konsole, after restart it opens the windows with the previously saved window size.
Comment 13 Rafael Fernández López 2008-10-14 16:12:00 UTC
Thanks for the patch Stefan. I cannot try it at the moment, I will be able in 6 hours more or less... However we usually use camelCase var names (isRestored instead of is_restored), and also I think you are missing the initialization of the var. I have no way of doing a diff here... but a rough idea I had to fix this was to do this check here:

void KMainWindow::applyMainWindowSettings(const KConfigGroup &cg, bool force)
{
    K_D(KMainWindow);
    kDebug(200) << "KMainWindow::applyMainWindowSettings " << cg.name();

    QWidget *focusedWidget = QApplication::focusWidget();

    d->letDirtySettings = false;

    if (!d->isRestored) {
        restoreWindowSize(cg);
        d->isRestored = true;
    }
...

on kmainwindow.cpp file.
Comment 14 David Faure 2008-10-14 17:27:29 UTC
Yes ereslibre's way would be more correct; Stefan's patch would still resize the window many times if konsole was called with --geometry (ok, very unusual, but still; that's what d->care_about_geometry is about).
Comment 15 Stefan Becker 2008-10-14 18:05:24 UTC
Created attachment 27880 [details]
Restore window size only once

Updated patch according to the comments. I wonder how care_about_geometry slipped through, though :-P


But testing this simple fix revealed that session restore is completely broken too. I.e. if I restore my standard konsole session with two tabs I see this:

konsole(15134): old window size QSize(700, 420) 
konsole(15134): new window size QSize(577, 415)

 -> initial window creation, resize to default in konsolerc.
 
konsole(15134): old window size QSize(577, 415) 
konsole(15134): new window size QSize(582, 700)

 -> resize from session data. So we want this request to go through

konsole(15134): old window size QSize(582, 700) 
konsole(15134): new window size QSize(577, 415)

 -> Add first tab. Resized back to konsolerc size :-(
 
konsole(15134): old window size QSize(577, 415) 
konsole(15134): new window size QSize(577, 415) 

 -> Add second tab. Resized back to konsolerc size :-(

You of course only notice this when the size in konsolerc is different from the one in the session. I missed this fact when working on bug #152761 :-(

With the patch applied you'll see only the first resize request, i.e. the windows will only be restored to the size stored in konsolerc. I'm wondering if we can check app->isSessionRestored() in applyMainWindowSettings and if we need a second flag for that.
Comment 16 Stefan Becker 2008-10-14 20:03:17 UTC
Created attachment 27881 [details]
Restore window size only once, but allow resize from session restore

OK, I've added a second flag. Now session restore works correctly, i.e. a window will be resized twice.
Comment 17 David Faure 2008-10-14 22:05:03 UTC
I see what you mean, but the naming of the variables is really strange. If I start an application in KDE, getting d->isSessionRestored = true; is really strange. I think isRestored should be sizeApplied, and, hmm the logic around isSessionRestored needs some cleanup. Right now it's a hack that says "call restoreWindowSize twice", basically (once with isSessionRestored=false and once with isSessionRestored=true). But I think this breaks in apps using kparts, if the gui is built in two stages (app+part) before we get to the session management restoration. This could be cleaned up by using the fact that readPropertiesInternal is the one called by session management restoration.

Yeah, I think it could work with a single bool sizeApplied, false initially, true after calling restoreWindowSize, and reset to false in readPropertiesInternal in order to force applyMainWindowSettings to apply the size again (because it comes from a different config file, that's the point...).
Comment 18 Rafael Fernández López 2008-10-14 22:42:36 UTC
Created attachment 27885 [details]
Only restore window size when needed

I think this is the right patch... Stefan, have you got an SVN account ? If so, feel free to commit it. If not, I can do it giving you credits =)
Comment 19 Stefan Becker 2008-10-14 22:53:06 UTC
Ahh I just had finished testing the exact same patch :-) At least I can confirm that the change works on KDE 4.1.2.

I don't have a SVN account, so please go ahead and apply it. Please mention David too.
Comment 20 Rafael Fernández López 2008-10-14 23:00:15 UTC
SVN commit 871409 by ereslibre:

Initial patch by Stefan Becker (stefan.becker nokia com). David Faure gave good input on the process of the patch review.

BUG: 172042


 M  +10 -1     kmainwindow.cpp  
 M  +1 -0      kmainwindow_p.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=871409
Comment 21 Ivo Anjo 2008-10-14 23:05:34 UTC
Could you commit it to the 4.1 branch?
It would be a pain having to wait until 4.2 to get this fixed :)
Comment 22 Stefan Becker 2008-10-14 23:08:14 UTC
FYI: I've created a backport request for Fedora 9:

<https://bugzilla.redhat.com/show_bug.cgi?id=466979>
Comment 23 Rafael Fernández López 2008-10-15 01:08:41 UTC
SVN commit 871465 by ereslibre:

Backport:
Initial patch by Stefan Becker (stefan.becker nokia com). David Faure gave good input on the process of the patch review.
    
CCBUG: 172042


 M  +10 -1     kmainwindow.cpp  
 M  +1 -0      kmainwindow_p.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=871465