ViewSplitter is not destroyed after all TerminalDisplay are closed. STEPS TO REPRODUCE 1. Split +-------------------------------------+ | | | Terminal 0 | | | +------------------+------------------+ | | | | Terminal 1 | Terminal 2 | | | | +------------------+------------------+ 2. Close Terminal 0 3. Close Terminal 1 OBSERVED RESULT Parent tree after few iterations widget: 0x55a584096c10 (Konsole::TerminalDisplay) parent: 0x55a583fb4490 (Konsole::ViewSplitter) parent: 0x55a58411d350 (Konsole::ViewSplitter) parent: 0x55a583fb3b20 (Konsole::ViewSplitter) parent: 0x55a5841548d0 (Konsole::ViewSplitter) parent: 0x55a583f93920 (Konsole::ViewSplitter) parent: 0x55a584171990 (Konsole::ViewSplitter) parent: 0x55a583ffff60 (Konsole::ViewSplitter) parent: 0x55a58410cf10 (Konsole::ViewSplitter) parent: 0x55a58405a350 (Konsole::ViewSplitter) parent: 0x55a583d59ab0 (QStackedWidget) parent: 0x55a583d14b10 (Konsole::TabbedViewContainer) parent: 0x55a583d05ce0 (Konsole::MainWindow)
What are you using that shows those lines?
(In reply to Kurt Hindenburg from comment #1) > What are you using that shows those lines? Breeze::WidgetExplorer::eventFilter https://github.com/KDE/breeze/blob/master/kstyle/debug/breezewidgetexplorer.cpp https://github.com/KDE/breeze/blob/master/kstyle/breeze.kcfg#L181
Also I think this is the reason for https://bugs.kde.org/show_bug.cgi?id=411741
The ViewSplitter is destroyed as soon as all of their children are destroyed. From the picture I can see that you first did a horizontal split and then a vertical split, closing 0 kills the Terminal there, but the split is still alive as it holds Terminal 1 / 2. Closing 1 still keeps the splitter alive as it holds 2. If you killed 2 and 1, on the other hand, the internal splitter deletes itself. I'll take a look to see how complex is to simplify the split hierarchy after a terminal has been removed
"how complex is to simplify" (best sentence ever) Most complex thing for any developer :) :) :) pseudo code idea void afterTerminalDetached () { if (this->childrenCount == 1 && this->child(0).class == "ViewSplitter") { auto child = this->child(0); this->parent().swap(this, child); } }
pseudocode seems nice, I'll try to implement something like that on the weekend.
Created attachment 123271 [details] ugly patch Working ugly patch/hack Also this fix/resolve #411741
Created attachment 123274 [details] attachment-905-0.html Really thanks for getting your hands dirty :) I'll do a test today on it. the code seems a bit fragile but sometimes code will be fragile no matter what. On Thu, Oct 17, 2019 at 1:47 PM Nikolay Zlatev <bugzilla_noreply@kde.org> wrote: > https://bugs.kde.org/show_bug.cgi?id=412596 > > --- Comment #7 from Nikolay Zlatev <nik@astrapaging.com> --- > Created attachment 123271 [details] > --> https://bugs.kde.org/attachment.cgi?id=123271&action=edit > ugly patch > > Working ugly patch/hack > > Also this fix/resolve #411741 > > -- > You are receiving this mail because: > You are on the CC list for the bug.
Created attachment 123276 [details] Patch v2 This is one of the ugliest code i have wrote.
Told you that it was non trivial. :)
Tomaz, do you agree w/ the patch as-is or do you have any suggestions?
Git commit c33eba8c60b99d00cd73c400d23b6f338ef32a85 by Kurt Hindenburg, on behalf of Carlos Alves. Committed on 02/11/2020 at 18:14. Pushed by hindenburg into branch 'master'. Fix konsole crashes when rearranging split views I'll try to explain here what I saw in this bug. Following the BUG instructions, drag the terminal[2] and drop (here the "drop event" starts) -> the dragged terminal "ViewSplitter" become scheduled for deletion, but it will only happens when it is in event loop. -> And of course it is not in event loop yet, it is still the "drop event" and it call the "addTerminalDisplay" to add the dragged terminal to a "ViewSplitter". -> This makes count() counts this scheduled for deletion "ViewSplitter" and goes to the wrong "if". -> It adds the "ViewSplitter" with the terminals into a "marked for deletion" one, finishes the "drop event" and enters event loop (a crash without a memleak! everything in the tab is properly deleted). Related: bug 411741 FIXED-IN: 20.12 M +4 -0 src/widgets/ViewSplitter.cpp https://invent.kde.org/utilities/konsole/commit/c33eba8c60b99d00cd73c400d23b6f338ef32a85
(In reply to Kurt Hindenburg from comment #12) > Git commit c33eba8c60b99d00cd73c400d23b6f338ef32a85 by Kurt Hindenburg, on > behalf of Carlos Alves. > Committed on 02/11/2020 at 18:14. > Pushed by hindenburg into branch 'master'. > > Fix konsole crashes when rearranging split views > > I'll try to explain here what I saw in this bug. > Following the BUG instructions, drag the terminal[2] and drop > (here the "drop event" starts) > -> the dragged terminal "ViewSplitter" become scheduled for deletion, > but it will only happens when it is in event loop. > -> And of course it is not in event loop yet, it is still the > "drop event" and it call the "addTerminalDisplay" to add the dragged > terminal to a "ViewSplitter". > -> This makes count() counts this scheduled for deletion "ViewSplitter" > and goes to the wrong "if". > -> It adds the "ViewSplitter" with the terminals into a > "marked for deletion" one, finishes the "drop event" and enters > event loop (a crash without a memleak! everything in the tab > is properly deleted). > Related: bug 411741 > FIXED-IN: 20.12 > > M +4 -0 src/widgets/ViewSplitter.cpp > > https://invent.kde.org/utilities/konsole/commit/ > c33eba8c60b99d00cd73c400d23b6f338ef32a85 This commit doesn't fix the issue described here Parent tree Breeze::WidgetExplorer::eventFilter widget: 0x557e59a12c10 (Konsole::TerminalDisplay) parent: 0x557e59d8db80 (Konsole::ViewSplitter) parent: 0x557e59cc6800 (Konsole::ViewSplitter) parent: 0x557e599aeb10 (Konsole::ViewSplitter) parent: 0x557e59925c90 (Konsole::ViewSplitter) parent: 0x557e59926070 (Konsole::ViewSplitter) parent: 0x557e595eb2f0 (Konsole::ViewSplitter) parent: 0x557e59b450a0 (Konsole::ViewSplitter) parent: 0x557e59aa0c80 (Konsole::ViewSplitter) parent: 0x557e59a00230 (Konsole::ViewSplitter) parent: 0x557e5984ada0 (Konsole::ViewSplitter) parent: 0x557e59970f30 (Konsole::ViewSplitter) parent: 0x557e599c28c0 (Konsole::ViewSplitter) parent: 0x557e5962a530 (Konsole::ViewSplitter) parent: 0x557e5946ba10 (QStackedWidget) parent: 0x557e59424510 (Konsole::TabbedViewContainer) parent: 0x557e594143e0 (Konsole::MainWindow)
the viewsplitter is destroyed when there's nothing inside of it. those viewsplitters are still holding the internal view splitter that ultimately holds a Terminal. The memory loss is negligigle, it does not introduces crashes or leaks, there's no user visible issues on because of that But I agree that it would be nice to fix, and I agree that the original issue is not fixed.