| Summary: | Split View crash with keyboard navigation | ||
|---|---|---|---|
| Product: | [Applications] konsole | Reporter: | Nikolay Zlatev <nik> |
| Component: | split-view | Assignee: | Konsole Bugs <konsole-bugs-null> |
| Status: | RESOLVED FIXED | ||
| Severity: | minor | ||
| Priority: | NOR | ||
| Version First Reported In: | 19.08.0 | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| Latest Commit: | https://commits.kde.org/konsole/b9d2cd668bc0e5bb65316419da0dbcf33dcc0799 | Version Fixed/Implemented In: | |
| Sentry Crash Report: | |||
| Attachments: |
Layout
Debug session |
||
|
Description
Nikolay Zlatev
2019-08-28 13:59:56 UTC
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 |