Implement saving sessions recursively
ClosedPublic

Authored by tcanabrava on Apr 3 2019, 10:26 AM.

Details

Summary

Session Save / Restore.
The old session code saved the sessions in random order, the restore
order was wrong and it ignored the splits (even the old style splits
where ignored, I'm not talking about the new style). This new session
/ restore code ignores nothing: It will save and restore your whole
Terminal Hierarchy, with splits, splits-in-splits, recursing everything
where needed.

bug: unfocused terminal tabs lacks title untill focused. should be
easy to fix and not a blocker.

Test Plan
  • Too many Logouts / Logins to test this.

Diff Detail

Repository
R319 Konsole
Branch
reEnableSessionsV2
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10617
Build 10635: arc lint + arc unit
tcanabrava created this revision.Apr 3 2019, 10:26 AM
Restricted Application added a project: Konsole. · View Herald TranscriptApr 3 2019, 10:26 AM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
tcanabrava requested review of this revision.Apr 3 2019, 10:26 AM

log out with the konsole's sessions you want to test and log back in - you could use 2nd system, VM, docker, etc as well

files are under ~/.config/session/ for viewing purposes

tcanabrava updated this revision to Diff 55360.Apr 3 2019, 5:22 PM
  • Implement the Load of Sessions
  • Solve crash while quitting konsole via Session End
  • Still Broken, do not review unless you can help. :)

Well, I finally managed to discover something:
when MainWindow::saveProperties is triggered we already destroyed the sessions and tabs, and that's the reason it's not saving anything.
I'm trying to debug the destructor order now to see if I can change something.

tcanabrava updated this revision to Diff 55411.Apr 4 2019, 1:36 PM
  • messageHandler + tons of debug
  • Sabe json correctly in the configuration

I'll remove the debugs / message handlers as soon as everything is working. but it's one of the fastest ways for me to keep the debug messages between runs so I can understand what's wrong.
the session is correctly saved, and the SessionManager correctly loads all the sessions back, the ViewManager however is broken on load. that's what I'm debugging now. so even tougth we don't display any terminal, they are up and running.

tcanabrava updated this revision to Diff 55415.Apr 4 2019, 2:57 PM
  • Sessions partially working
  • Code cleanup

This code can load sessions, it has a better behavior than the old session load code, but still has bugs. I'll finish this luckly today / tomorrow.

tcanabrava updated this revision to Diff 55735.Apr 8 2019, 12:00 PM

Session Save / Restore

tcanabrava retitled this revision from [WIP] Implement saving sessions recursively to Implement saving sessions recursively.Apr 8 2019, 12:03 PM
tcanabrava edited the summary of this revision. (Show Details)
tcanabrava edited the test plan for this revision. (Show Details)
tcanabrava added a reviewer: ngraham.
tcanabrava edited the summary of this revision. (Show Details)Apr 8 2019, 12:10 PM
tcanabrava edited the test plan for this revision. (Show Details)

This seems to fix the issue I started having recently where Konsole crashes at logout and then opens as a big white window on login after session restoration.

src/MainWindow.h
68 ↗(On Diff #55735)

Unnecessary whitespace change

src/ViewManager.cpp
910–912

w isn't a very descriptive variable name :)

984–985

TODOs written in the first person are weird. Who is "I"?

Also we should probably just fix this (if possible) rather than adding a new TODO to the code.

src/ViewSplitter.h
55 ↗(On Diff #55735)

Unnecessary whitespace change

src/main.cpp
69 ↗(On Diff #55735)

Unnecessary whitespace change

hindenburg edited the summary of this revision. (Show Details)Apr 8 2019, 1:55 PM

This seems to fix the issue I started having recently where Konsole crashes at logout and then opens as a big white window on login after session restoration.

it indeed does fix that. I have no idea *why* it crashes (if you close konsole, it doesn not crash, only when it's closed via exiting the session).

tcanabrava updated this revision to Diff 55742.Apr 8 2019, 2:07 PM
tcanabrava marked an inline comment as done.
tcanabrava edited the test plan for this revision. (Show Details)
  • Whitespace / Todos removal
tcanabrava updated this revision to Diff 55743.Apr 8 2019, 2:22 PM
  • Better variable name
tcanabrava marked 2 inline comments as done.Apr 8 2019, 2:46 PM
ngraham accepted this revision.Apr 8 2019, 3:18 PM

LGTM!

This revision is now accepted and ready to land.Apr 8 2019, 3:18 PM

waiting for @hindenburg ok before landing. :)

ngraham requested changes to this revision.Apr 8 2019, 5:17 PM

Actually now this breaks the ctrl+PgUp/PgDn shortcuts for switching between tabs.

This revision now requires changes to proceed.Apr 8 2019, 5:17 PM

Hm? This doesn’t touches that.

Em seg, 8 de abr de 2019 às 19:17, Nathaniel Graham <
noreply@phabricator.kde.org> escreveu:

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed. View Revision
https://phabricator.kde.org/D20224

Actually now this breaks the ctrl+PgUp/PgDn shortcuts for switching
between tabs.

*REPOSITORY*
R319 Konsole

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

*To: *tcanabrava, Konsole, hindenburg, ngraham
*Cc: *konsole-devel, gennad, thsurrel, ngraham, maximilianocuria,
hindenburg

Nonetheless, with git master, the tab switching keyboard shortcuts work for me, and with this patch, they do not. :(

Nonetheless, with git master, the tab switching keyboard shortcuts work for me, and with this patch, they do not. :(

It works here w/ this patch - I will say I've notice some issues w/ master and changing tabs w/ shortcuts but nothing reproducible ATM

A couple of clazy warning if you want to look at

src/ViewManager.cpp
910

FYI, loop variable 'widgetJsonValue' is always a copy because the range of type 'QJsonArray' does not return a reference

931

Missing reference in range-for with non trivial type (QJsonValue) [-Wclazy-range-loop

tcanabrava added inline comments.Apr 9 2019, 9:03 AM
src/ViewManager.cpp
910

Having the reference of a temporary is not a problem, and considering the clazy warning on your other comment I belive this is the way to go.

931

That's a QJsonArray too. I'll just add the reference to the temporary.

tcanabrava updated this revision to Diff 55803.Apr 9 2019, 9:08 AM
  • Use reference with range for loops

I'm fine w/ this

@ngraham can you accept even with the shortcut issue that you are having? I have other reviews that depend on this one. I promise you to fix the shortcut bug <3

ngraham accepted this revision.Apr 9 2019, 3:08 PM

Ok, Ok. :)

This revision is now accepted and ready to land.Apr 9 2019, 3:08 PM
This revision was automatically updated to reflect the committed changes.