Created attachment 122401 [details] Layout Konsole crash when keyboard navigation is used to switch between TerminalDisplay auto splitterTerminal = qobject_cast<TerminalDisplay*>(targetSplitter->widget(0)); return nullptr and then splitterTerminal->setFocus(Qt::OtherFocusReason); is used https://cgit.kde.org/konsole.git/tree/src/ViewSplitter.cpp#n171 STEPS TO REPRODUCE 1. Split terminal in --------------------------- | | | | | | | | | --------------------------- | | | | | | --------------------------- 2. navigate from bottom to top (ctrl + shift + arrow_up) OBSERVED RESULT Crash EXPECTED RESULT Not Crash Arch KDE Plasma Version: 5.16.4 KDE Frameworks Version: 5.61.0 Qt Version: 5.13.0 Work around diff --git a/src/ViewSplitter.cpp b/src/ViewSplitter.cpp index 7e31172c..2ab15e5c 100644 --- a/src/ViewSplitter.cpp +++ b/src/ViewSplitter.cpp @@ -168,7 +168,7 @@ void ViewSplitter::handleFocusDirection(Qt::Orientation orientation, int directi } else if (qobject_cast<QSplitterHandle*>(child) != nullptr) { auto targetSplitter = qobject_cast<QSplitter*>(child->parent()); auto splitterTerminal = qobject_cast<TerminalDisplay*>(targetSplitter->widget(0)); - splitterTerminal->setFocus(Qt::OtherFocusReason); + if (splitterTerminal != nullptr) splitterTerminal->setFocus(Qt::OtherFocusReason); } else if (qobject_cast<QWidget*>(child) != nullptr) { TerminalDisplay *terminalParent = nullptr; while(terminalParent == nullptr) {
I can't reproduce the crash - I'll keep trying - checking for nullptr seems reasonable.
Created attachment 122418 [details] Debug session
This only happens on some split layouts and only with keyboard navigation. targetSplitter->widget(0) returns Konsole::ViewSplitter and then casting to TerminalDisplay fail.
It appears this is theme related issue. When I use Breeze everything is OK. For Breeze const auto handleWidth = parentSplitter->handleWidth() <= 1 ? 4 : parentSplitter->handleWidth(); returns 4 as a constant. For Kvantum -> Adpata parentSplitter->handleWidth() is used (4 pixels again, but this is the width of the splitter border) then topSplitter->childAt(newPoint) returns ViewSplitter instead of TerminalDisplay This fixes issue for me const auto handleWidth = parentSplitter->handleWidth() <= 1 ? 4 : parentSplitter->handleWidth() + 1; ---------------------------------------------------------------------- diff --git a/src/ViewSplitter.cpp b/src/ViewSplitter.cpp index 7e31172c..24b49334 100644 --- a/src/ViewSplitter.cpp +++ b/src/ViewSplitter.cpp @@ -147,7 +147,7 @@ void ViewSplitter::handleFocusDirection(Qt::Orientation orientation, int directi auto parentSplitter = qobject_cast<ViewSplitter*>(terminalDisplay->parentWidget()); auto topSplitter = parentSplitter->getToplevelSplitter(); - const auto handleWidth = parentSplitter->handleWidth() <= 1 ? 4 : parentSplitter->handleWidth(); + const auto handleWidth = parentSplitter->handleWidth() <= 1 ? 4 : parentSplitter->handleWidth() + 1; const auto start = QPoint(terminalDisplay->x(), terminalDisplay->y()); const auto startMapped = parentSplitter->mapTo(topSplitter, start); @@ -168,7 +168,8 @@ void ViewSplitter::handleFocusDirection(Qt::Orientation orientation, int directi } else if (qobject_cast<QSplitterHandle*>(child) != nullptr) { auto targetSplitter = qobject_cast<QSplitter*>(child->parent()); auto splitterTerminal = qobject_cast<TerminalDisplay*>(targetSplitter->widget(0)); - splitterTerminal->setFocus(Qt::OtherFocusReason); + if (splitterTerminal != nullptr) + splitterTerminal->setFocus(Qt::OtherFocusReason); } else if (qobject_cast<QWidget*>(child) != nullptr) { TerminalDisplay *terminalParent = nullptr; while(terminalParent == nullptr) {
Final patch diff --git a/src/ViewSplitter.cpp b/src/ViewSplitter.cpp index 7e31172c..38c576a9 100644 --- a/src/ViewSplitter.cpp +++ b/src/ViewSplitter.cpp @@ -147,7 +147,7 @@ void ViewSplitter::handleFocusDirection(Qt::Orientation orientation, int directi auto parentSplitter = qobject_cast<ViewSplitter*>(terminalDisplay->parentWidget()); auto topSplitter = parentSplitter->getToplevelSplitter(); - const auto handleWidth = parentSplitter->handleWidth() <= 1 ? 4 : parentSplitter->handleWidth(); + const auto handleWidth = parentSplitter->handleWidth() <= 1 ? 4 : parentSplitter->handleWidth() + 1; const auto start = QPoint(terminalDisplay->x(), terminalDisplay->y()); const auto startMapped = parentSplitter->mapTo(topSplitter, start); @@ -163,20 +163,21 @@ void ViewSplitter::handleFocusDirection(Qt::Orientation orientation, int directi const auto newPoint = QPoint(newX, newY); auto child = topSplitter->childAt(newPoint); + TerminalDisplay *focusTerminal = nullptr; if (auto* terminal = qobject_cast<TerminalDisplay*>(child)) { - terminal->setFocus(Qt::OtherFocusReason); + focusTerminal = terminal; } else if (qobject_cast<QSplitterHandle*>(child) != nullptr) { auto targetSplitter = qobject_cast<QSplitter*>(child->parent()); - auto splitterTerminal = qobject_cast<TerminalDisplay*>(targetSplitter->widget(0)); - splitterTerminal->setFocus(Qt::OtherFocusReason); + focusTerminal = qobject_cast<TerminalDisplay*>(targetSplitter->widget(0)); } else if (qobject_cast<QWidget*>(child) != nullptr) { - TerminalDisplay *terminalParent = nullptr; - while(terminalParent == nullptr) { - terminalParent = qobject_cast<TerminalDisplay*>(child->parentWidget()); + while(child != nullptr && focusTerminal == nullptr) { + focusTerminal = qobject_cast<TerminalDisplay*>(child->parentWidget()); child = child->parentWidget(); } - terminalParent->setFocus(Qt::OtherFocusReason); } + + if (focusTerminal != nullptr) + focusTerminal->setFocus(Qt::OtherFocusReason); } void ViewSplitter::focusUp()
the null check looks reasonable even though I can't it to crash here with any theme. The "+1" is worrisome as it appears to be theme specific.
(In reply to Kurt Hindenburg from comment #6) > the null check looks reasonable even though I can't it to crash here with > any theme. The "+1" is worrisome as it appears to be theme specific. const auto handleWidth = parentSplitter->handleWidth() <= 1 ? 4 :... "? 4" is also worrisome :) In fact this "4" constant prevent konsole to crash under "Breeze" theme. When handleWidth = parentSplitter->handleWidth(); is used, konsole crash under all themes. I have tested with "const auto handleWidth = parentSplitter->handleWidth() + 3;" and everything seems to work fine.
I'm testing this which should be more reliable. I'm also finding other issues with different themes. // Find the theme's splitter width + extra space to find valid terminal const auto handleWidth = parentSplitter->style()->pixelMetric(QStyle::PM_SplitterWidth) + 2;
> // Find the theme's splitter width + extra space to find valid terminal > const auto handleWidth = parentSplitter->style()->pixelMetric(QStyle::PM_SplitterWidth) + 2; On Breeze handleWidth is now "3" (1 + 2) and konsole crash. That why I have used "+3" (min handleWidth=4, 1 + 3)
Git commit 63ef8872bfc485248a8de6c7b45909afc68bfcc9 by Kurt Hindenburg. Committed on 02/10/2019 at 15:52. Pushed by hindenburg into branch 'master'. Fix split view crashes when using keyboard navigation with some themes The current code uses hard-coded values for some theme items. This fix uses the scrollbar width. There are other theme issues with split views. Test using Oxygen application style (+ increase scrollbar width) Most patch by Nikolay Zlatev <nik@astrapaging.com> Related: bug 412020 M +11 -8 src/ViewSplitter.cpp https://invent.kde.org/kde/konsole/commit/63ef8872bfc485248a8de6c7b45909afc68bfcc9
Git commit 63ef8872bfc485248a8de6c7b45909afc68bfcc9 by Kurt Hindenburg. Committed on 02/10/2019 at 15:52. Pushed by scmsync into branch 'master'. Fix split view crashes when using keyboard navigation with some themes The current code uses hard-coded values for some theme items. This fix uses the scrollbar width. There are other theme issues with split views. Test using Oxygen application style (+ increase scrollbar width) Most patch by Nikolay Zlatev <nik@astrapaging.com> Related: bug 412020 M +11 -8 src/ViewSplitter.cpp https://commits.kde.org/konsole/63ef8872bfc485248a8de6c7b45909afc68bfcc9
... pixelMetric(QStyle::PM_SplitterWidth) + 2 "+2" is not enough for Breeze breeze.h Breeze::Metrics::Splitter_SplitterWidth = 1; https://github.com/KDE/breeze/blob/master/kstyle/breezestyle.cpp#L615 QT docs If you set handleWidth to 1 or 0, the actual grab area will grow to overlap a few pixels of its respective widgets. https://doc.qt.io/qt-5/qsplitter.html#handleWidth-prop I don't known if there is a method to get "grab area" size. P.S. Breeze::WidgetExplorer::eventFilter returns 5 for width
// Find the theme's splitter width + extra space to find valid terminal // https://doc.qt.io/qt-5/qsplitter.html#handleWidth-prop // By default, this property contains a value // that depends on the user's platform and style preferences. // // If you set handleWidth to 1 or 0, // the actual grab area will grow to overlap a few // pixels of its respective widgets. const auto handleWidth = parentSplitter->handleWidth() + 3; P.S. pixelMetric(QStyle::PM_SplitterWidth) always returns theme value not actual rendered size I have tested on Breeze app->setStyleSheet(QStringLiteral("QSplitter::handle {width: 8px; height: 8px; border: 1px solid red}")); in main.cpp pixelMetric(QStyle::PM_SplitterWidth) returns 1 handleWidth() returns 10 (width+border*2)
Breeze handleWidth = ...pixelMetric(QStyle::PM_SplitterWidth) + 2; +------------------------+ | | | 0 | | | +------------------------+ | | | 1 | | | +------------------------+ | | | 2 | | IN FOCUS | +------------------------+ CTRL + UP Expected: 1 to be in focus Result: 0 is in focus (qobject_cast<TerminalDisplay*>(targetSplitter->widget(0));)
Ok thanks for testing - I misunderstood what is going on - to be clear "const auto handleWidth = parentSplitter->handleWidth() + 3;" works for all your testing? Also, are you not getting crashes when you do splits? 412020
(In reply to Kurt Hindenburg from comment #15) > Ok thanks for testing - I misunderstood what is going on - to be clear > "const auto handleWidth = parentSplitter->handleWidth() + 3;" works for all > your testing? yes works fine. > > Also, are you not getting crashes when you do splits? 412020 Only with Oxygen (I will post in 412020)
Git commit b27c8e7fd3b047cf4893c7eb202c4861bccc21e8 by Kurt Hindenburg. Committed on 03/10/2019 at 14:19. Pushed by hindenburg into branch 'master'. Correct split view and keyboard navigation Rework of pervious commit that actually works 63ef8872bfc485248a8de6c7b45909afc68bfcc9 Thanks to Nikolay Zlatev <nik@astrapaging.com> M +2 -2 src/ViewSplitter.cpp https://invent.kde.org/kde/konsole/commit/b27c8e7fd3b047cf4893c7eb202c4861bccc21e8
Git commit b27c8e7fd3b047cf4893c7eb202c4861bccc21e8 by Kurt Hindenburg. Committed on 03/10/2019 at 14:19. Pushed by scmsync into branch 'master'. Correct split view and keyboard navigation Rework of pervious commit that actually works 63ef8872bfc485248a8de6c7b45909afc68bfcc9 Thanks to Nikolay Zlatev <nik@astrapaging.com> M +2 -2 src/ViewSplitter.cpp https://commits.kde.org/konsole/b27c8e7fd3b047cf4893c7eb202c4861bccc21e8
Git commit b9d2cd668bc0e5bb65316419da0dbcf33dcc0799 by Kurt Hindenburg. Committed on 03/10/2019 at 16:53. Pushed by hindenburg into branch 'Applications/19.08'. Correct split view and keyboard navigation Rework of pervious commit that actually works 63ef8872bfc485248a8de6c7b45909afc68bfcc9 Thanks to Nikolay Zlatev <nik@astrapaging.com> (cherry picked from commit b27c8e7fd3b047cf4893c7eb202c4861bccc21e8) M +2 -2 src/ViewSplitter.cpp https://invent.kde.org/kde/konsole/commit/b9d2cd668bc0e5bb65316419da0dbcf33dcc0799
Git commit ba84640a7bd872831f8330540081f7ddd0ff308b by Kurt Hindenburg. Committed on 03/10/2019 at 16:52. Pushed by hindenburg into branch 'Applications/19.08'. Fix split view crashes when using keyboard navigation with some themes The current code uses hard-coded values for some theme items. This fix uses the scrollbar width. There are other theme issues with split views. Test using Oxygen application style (+ increase scrollbar width) Most patch by Nikolay Zlatev <nik@astrapaging.com> cherry-picked from 63ef8872bfc485248a8de6c7b45909afc68bfcc9 M +11 -7 src/ViewSplitter.cpp https://invent.kde.org/kde/konsole/commit/ba84640a7bd872831f8330540081f7ddd0ff308b
Git commit ba84640a7bd872831f8330540081f7ddd0ff308b by Kurt Hindenburg. Committed on 03/10/2019 at 16:52. Pushed by scmsync into branch 'Applications/19.08'. Fix split view crashes when using keyboard navigation with some themes The current code uses hard-coded values for some theme items. This fix uses the scrollbar width. There are other theme issues with split views. Test using Oxygen application style (+ increase scrollbar width) Most patch by Nikolay Zlatev <nik@astrapaging.com> cherry-picked from 63ef8872bfc485248a8de6c7b45909afc68bfcc9 M +11 -7 src/ViewSplitter.cpp https://commits.kde.org/konsole/ba84640a7bd872831f8330540081f7ddd0ff308b
Git commit b9d2cd668bc0e5bb65316419da0dbcf33dcc0799 by Kurt Hindenburg. Committed on 03/10/2019 at 16:53. Pushed by scmsync into branch 'Applications/19.08'. Correct split view and keyboard navigation Rework of pervious commit that actually works 63ef8872bfc485248a8de6c7b45909afc68bfcc9 Thanks to Nikolay Zlatev <nik@astrapaging.com> (cherry picked from commit b27c8e7fd3b047cf4893c7eb202c4861bccc21e8) M +2 -2 src/ViewSplitter.cpp https://commits.kde.org/konsole/b9d2cd668bc0e5bb65316419da0dbcf33dcc0799