Summary: | ViewSplitter is not destroyed | ||
---|---|---|---|
Product: | [Applications] konsole | Reporter: | Nikolay Zlatev <nik> |
Component: | split-view | Assignee: | Konsole Developer <konsole-devel> |
Status: | VERIFIED REMIND | ||
Severity: | minor | CC: | nate, tcanabrava |
Priority: | NOR | ||
Version: | master | ||
Target Milestone: | --- | ||
Platform: | Other | ||
OS: | Linux | ||
Latest Commit: | https://invent.kde.org/utilities/konsole/commit/c33eba8c60b99d00cd73c400d23b6f338ef32a85 | Version Fixed In: | 20.12 |
Attachments: |
ugly patch
attachment-905-0.html Patch v2 |
Description
Nikolay Zlatev
2019-10-04 09:06:10 UTC
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. |