Bug 411387 - Split View crash with keyboard navigation
Summary: Split View crash with keyboard navigation
Status: RESOLVED FIXED
Alias: None
Product: konsole
Classification: Applications
Component: split-view (show other bugs)
Version: 19.08.0
Platform: Other Linux
: NOR minor
Target Milestone: ---
Assignee: Konsole Developer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-08-28 13:59 UTC by Nikolay Zlatev
Modified: 2019-10-03 16:55 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
Layout (60.70 KB, image/png)
2019-08-28 13:59 UTC, Nikolay Zlatev
Details
Debug session (807.91 KB, image/png)
2019-08-30 07:11 UTC, Nikolay Zlatev
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolay Zlatev 2019-08-28 13:59:56 UTC
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) {
Comment 1 Kurt Hindenburg 2019-08-30 00:52:05 UTC
I can't reproduce the crash - I'll keep trying - checking for nullptr seems reasonable.
Comment 2 Nikolay Zlatev 2019-08-30 07:11:21 UTC
Created attachment 122418 [details]
Debug session
Comment 3 Nikolay Zlatev 2019-08-30 07:11:31 UTC
This only happens on some split layouts and only with keyboard navigation.

targetSplitter->widget(0) returns Konsole::ViewSplitter

and then casting to TerminalDisplay fail.
Comment 4 Nikolay Zlatev 2019-08-30 08:38:55 UTC
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) {
Comment 5 Nikolay Zlatev 2019-08-30 09:15:57 UTC
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()
Comment 6 Kurt Hindenburg 2019-09-10 02:21:44 UTC
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.
Comment 7 Nikolay Zlatev 2019-09-10 06:45:24 UTC
(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.
Comment 8 Kurt Hindenburg 2019-09-18 12:35:58 UTC
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;
Comment 9 Nikolay Zlatev 2019-09-19 08:01:25 UTC
> // 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)
Comment 10 Kurt Hindenburg 2019-10-02 15:53:48 UTC
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
Comment 11 Kurt Hindenburg 2019-10-02 15:53:50 UTC
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
Comment 12 Nikolay Zlatev 2019-10-03 08:40:22 UTC
... 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
Comment 13 Nikolay Zlatev 2019-10-03 10:37:28 UTC
// 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)
Comment 14 Nikolay Zlatev 2019-10-03 11:00:38 UTC
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));)
Comment 15 Kurt Hindenburg 2019-10-03 13:48:01 UTC
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
Comment 16 Nikolay Zlatev 2019-10-03 13:54:43 UTC
(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)
Comment 17 Kurt Hindenburg 2019-10-03 14:20:46 UTC
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
Comment 18 Kurt Hindenburg 2019-10-03 14:20:48 UTC
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
Comment 19 Kurt Hindenburg 2019-10-03 16:55:19 UTC
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
Comment 20 Kurt Hindenburg 2019-10-03 16:55:19 UTC
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
Comment 21 Kurt Hindenburg 2019-10-03 16:55:23 UTC
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
Comment 22 Kurt Hindenburg 2019-10-03 16:55:23 UTC
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