Differential D23601

Fix the fallback code used when restoring session fails
ClosedPublic
Actions

Authored by jpalecek on Aug 31 2019, 10:24 AM.

Details

Reviewers
hindenburg
Group Reviewers
Konsole
Commits
R319:1df621390e82: Fix the fallback code used when restoring session fails
R319:656f6804ea2c: Fix the fallback code used when restoring session fails
Summary

When restoring session in Konsole fails, the fallback code should
create a default view and session instead. However, the code omitted
adding this view to the container, which makes

  • the window black (or grey in my case), see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=935031
  • konsole crash later, when you add a new tab and close it, because code in ViewManager expects TerminalDisplays to be children of ViewSplitter, which is not the case

    This fix adds the view to activeController(), just like every other code that creates TerminalDisplay.
Test Plan
  1. find a session in your $HOME/.config/session/ from konsole before 19.08
  2. konsole -session xxxx (where xxxx are the numbers in the session file) -> there should be a konsole window with usable terminal
  3. add a new tab with C-S-T and close it ->there should be no crash

Diff Detail

Repository
R319 Konsole
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jpalecek created this revision.Aug 31 2019, 10:24 AM
Restricted Application added a project: Konsole. · View Herald TranscriptAug 31 2019, 10:24 AM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
jpalecek requested review of this revision.Aug 31 2019, 10:24 AM
jpalecek added a subscriber: tcanabrava.Aug 31 2019, 10:37 AM

BTW it is even possible to load session from older kopete using the old code, see this patch in Debian. That would provide the users a seamless experience on upgrades.

@tcanabrava Was this considered when making the session save/restore code change? Maybe it is too old for that now, but why wasn't this approach taken then?

hindenburg added a comment.Sep 1 2019, 9:22 PM

This looks fine - I'll test the restore patch

hindenburg accepted this revision.Sep 1 2019, 10:29 PM
hindenburg edited the test plan for this revision. (Show Details)

Thanks, I'll look at the other patch shortly. This regression should have been caught in my testing.

This revision is now accepted and ready to land.Sep 1 2019, 10:30 PM
hindenburg edited the test plan for this revision. (Show Details)Sep 1 2019, 10:31 PM
Closed by commit R319:656f6804ea2c: Fix the fallback code used when restoring session fails (authored by jpalecek, committed by hindenburg). · Explain WhySep 1 2019, 10:47 PM
This revision was automatically updated to reflect the committed changes.
hindenburg added a comment.Sep 2 2019, 12:58 AM

On the 19.08 branch I get a lot of these when exiting a konsole session that uses this code. I don't see this warning on master.

QObject::startTimer: Timers can only be used with threads started with QThread

tcanabrava added a comment.Sep 2 2019, 7:11 AM

When I tested I kept a old profile but probably forgot to use during
development. Sorry for the trouble :)

Em sáb, 31 de ago de 2019 às 12:37, Jiří Paleček <
noreply@phabricator.kde.org> escreveu:

jpalecek added a subscriber: tcanabrava.
jpalecek added a comment. View Revision
https://phabricator.kde.org/D23601

BTW it is even possible to load session from older kopete using the old
code, see this patch in Debian
https://salsa.debian.org/qt-kde-team/kde/konsole/commit/f07e71f72b2c464d1559b57e9ed2d5c5f6bf1f1b?merge_request_iid=2.
That would provide the users a seamless experience on upgrades.

@tcanabrava https://phabricator.kde.org/p/tcanabrava/ Was this
considered when making the session save/restore code change? Maybe it is
too old for that now, but why wasn't this approach taken then?

*REPOSITORY*
R319 Konsole

*REVISION DETAIL*
https://phabricator.kde.org/D23601

*To: *jpalecek, hindenburg
*Cc: *tcanabrava, konsole-devel, fbampaloukas, thsurrel, ngraham,
maximilianocuria, hindenburg

Revision Contents
Changeset List

PathPackages
Msrc/ViewManager.cpp (2 lines)

Diff 65183

View Options

src/ViewManager.cpp

Loading...