[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 Kurt Hindenburg changed: What|Removed |Added Latest Commit|https://invent.kde.org/kde/ |https://commits.kde.org/kon |konsole/commit/ba84640a7bd8 |sole/ba84640a7bd872831f8330 |72831f8330540081f7ddd0ff308 |540081f7ddd0ff308b |b | --- Comment #21 from Kurt Hindenburg --- 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 cherry-picked from 63ef8872bfc485248a8de6c7b45909afc68bfcc9 M +11 -7src/ViewSplitter.cpp https://commits.kde.org/konsole/ba84640a7bd872831f8330540081f7ddd0ff308b -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 Kurt Hindenburg changed: What|Removed |Added Latest Commit|https://commits.kde.org/kon |https://invent.kde.org/kde/ |sole/b27c8e7fd3b047cf4893c7 |konsole/commit/b9d2cd668bc0 |eb202c4861bccc21e8 |e5bb65316419da0dbcf33dcc079 ||9 --- Comment #19 from Kurt Hindenburg --- 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 (cherry picked from commit b27c8e7fd3b047cf4893c7eb202c4861bccc21e8) M +2-2src/ViewSplitter.cpp https://invent.kde.org/kde/konsole/commit/b9d2cd668bc0e5bb65316419da0dbcf33dcc0799 -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 Kurt Hindenburg changed: What|Removed |Added Latest Commit|https://invent.kde.org/kde/ |https://commits.kde.org/kon |konsole/commit/ba84640a7bd8 |sole/ba84640a7bd872831f8330 |72831f8330540081f7ddd0ff308 |540081f7ddd0ff308b |b |https://commits.kde.org/kon |https://commits.kde.org/kon |sole/b9d2cd668bc0e5bb653164 |sole/ba84640a7bd872831f8330 |19da0dbcf33dcc0799 |540081f7ddd0ff308b | --- Comment #21 from Kurt Hindenburg --- 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 cherry-picked from 63ef8872bfc485248a8de6c7b45909afc68bfcc9 M +11 -7src/ViewSplitter.cpp https://commits.kde.org/konsole/ba84640a7bd872831f8330540081f7ddd0ff308b --- Comment #22 from Kurt Hindenburg --- 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 (cherry picked from commit b27c8e7fd3b047cf4893c7eb202c4861bccc21e8) M +2-2src/ViewSplitter.cpp https://commits.kde.org/konsole/b9d2cd668bc0e5bb65316419da0dbcf33dcc0799 -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 Kurt Hindenburg changed: What|Removed |Added Latest Commit|https://commits.kde.org/kon |https://invent.kde.org/kde/ |sole/b27c8e7fd3b047cf4893c7 |konsole/commit/b9d2cd668bc0 |eb202c4861bccc21e8 |e5bb65316419da0dbcf33dcc079 |https://invent.kde.org/kde/ |9 |konsole/commit/b9d2cd668bc0 |https://invent.kde.org/kde/ |e5bb65316419da0dbcf33dcc079 |konsole/commit/ba84640a7bd8 |9 |72831f8330540081f7ddd0ff308 ||b --- Comment #19 from Kurt Hindenburg --- 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 (cherry picked from commit b27c8e7fd3b047cf4893c7eb202c4861bccc21e8) M +2-2src/ViewSplitter.cpp https://invent.kde.org/kde/konsole/commit/b9d2cd668bc0e5bb65316419da0dbcf33dcc0799 --- Comment #20 from Kurt Hindenburg --- 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 cherry-picked from 63ef8872bfc485248a8de6c7b45909afc68bfcc9 M +11 -7src/ViewSplitter.cpp https://invent.kde.org/kde/konsole/commit/ba84640a7bd872831f8330540081f7ddd0ff308b -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 Kurt Hindenburg changed: What|Removed |Added Latest Commit|https://invent.kde.org/kde/ |https://commits.kde.org/kon |konsole/commit/b27c8e7fd3b0 |sole/b27c8e7fd3b047cf4893c7 |47cf4893c7eb202c4861bccc21e |eb202c4861bccc21e8 |8 | --- Comment #18 from Kurt Hindenburg --- 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 M +2-2src/ViewSplitter.cpp https://commits.kde.org/konsole/b27c8e7fd3b047cf4893c7eb202c4861bccc21e8 -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 Kurt Hindenburg changed: What|Removed |Added Status|REOPENED|RESOLVED Resolution|--- |FIXED Latest Commit|https://commits.kde.org/kon |https://invent.kde.org/kde/ |sole/63ef8872bfc485248a8de6 |konsole/commit/b27c8e7fd3b0 |c7b45909afc68bfcc9 |47cf4893c7eb202c4861bccc21e ||8 --- Comment #17 from Kurt Hindenburg --- 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 M +2-2src/ViewSplitter.cpp https://invent.kde.org/kde/konsole/commit/b27c8e7fd3b047cf4893c7eb202c4861bccc21e8 -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 --- Comment #16 from Nikolay Zlatev --- (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) -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 --- Comment #15 from Kurt Hindenburg --- 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 -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 --- Comment #14 from Nikolay Zlatev --- 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(targetSplitter->widget(0));) -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387
--- Comment #13 from Nikolay Zlatev ---
// 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)
--
You are receiving this mail because:
You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 Nikolay Zlatev changed: What|Removed |Added Ever confirmed|0 |1 Status|RESOLVED|REOPENED Resolution|FIXED |--- --- Comment #12 from Nikolay Zlatev --- ... 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 -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 Kurt Hindenburg changed: What|Removed |Added Latest Commit|https://invent.kde.org/kde/ |https://commits.kde.org/kon |konsole/commit/63ef8872bfc4 |sole/63ef8872bfc485248a8de6 |85248a8de6c7b45909afc68bfcc |c7b45909afc68bfcc9 |9 | --- Comment #11 from Kurt Hindenburg --- 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 Related: bug 412020 M +11 -8src/ViewSplitter.cpp https://commits.kde.org/konsole/63ef8872bfc485248a8de6c7b45909afc68bfcc9 -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 Kurt Hindenburg changed: What|Removed |Added Latest Commit||https://invent.kde.org/kde/ ||konsole/commit/63ef8872bfc4 ||85248a8de6c7b45909afc68bfcc ||9 Status|REPORTED|RESOLVED Resolution|--- |FIXED --- Comment #10 from Kurt Hindenburg --- 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 Related: bug 412020 M +11 -8src/ViewSplitter.cpp https://invent.kde.org/kde/konsole/commit/63ef8872bfc485248a8de6c7b45909afc68bfcc9 -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 --- Comment #9 from Nikolay Zlatev --- > // 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) -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 --- Comment #8 from Kurt Hindenburg --- 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; -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 --- Comment #7 from Nikolay Zlatev --- (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. -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 --- Comment #6 from Kurt Hindenburg --- 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. -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387
--- Comment #5 from Nikolay Zlatev ---
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(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(child)) {
-terminal->setFocus(Qt::OtherFocusReason);
+focusTerminal = terminal;
} else if (qobject_cast(child) != nullptr) {
auto targetSplitter = qobject_cast(child->parent());
-auto splitterTerminal =
qobject_cast(targetSplitter->widget(0));
-splitterTerminal->setFocus(Qt::OtherFocusReason);
+focusTerminal =
qobject_cast(targetSplitter->widget(0));
} else if (qobject_cast(child) != nullptr) {
-TerminalDisplay *terminalParent = nullptr;
-while(terminalParent == nullptr) {
-terminalParent =
qobject_cast(child->parentWidget());
+while(child != nullptr && focusTerminal == nullptr) {
+focusTerminal =
qobject_cast(child->parentWidget());
child = child->parentWidget();
}
-terminalParent->setFocus(Qt::OtherFocusReason);
}
+
+if (focusTerminal != nullptr)
+focusTerminal->setFocus(Qt::OtherFocusReason);
}
void ViewSplitter::focusUp()
--
You are receiving this mail because:
You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387
--- Comment #4 from Nikolay Zlatev ---
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(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(child) != nullptr) {
auto targetSplitter = qobject_cast(child->parent());
auto splitterTerminal =
qobject_cast(targetSplitter->widget(0));
-splitterTerminal->setFocus(Qt::OtherFocusReason);
+if (splitterTerminal != nullptr)
+splitterTerminal->setFocus(Qt::OtherFocusReason);
} else if (qobject_cast(child) != nullptr) {
TerminalDisplay *terminalParent = nullptr;
while(terminalParent == nullptr) {
--
You are receiving this mail because:
You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 --- Comment #3 from Nikolay Zlatev --- This only happens on some split layouts and only with keyboard navigation. targetSplitter->widget(0) returns Konsole::ViewSplitter and then casting to TerminalDisplay fail. -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 --- Comment #2 from Nikolay Zlatev --- Created attachment 122418 --> https://bugs.kde.org/attachment.cgi?id=122418=edit Debug session -- You are receiving this mail because: You are watching all bug changes.
[konsole] [Bug 411387] Split View crash with keyboard navigation
https://bugs.kde.org/show_bug.cgi?id=411387 --- Comment #1 from Kurt Hindenburg --- I can't reproduce the crash - I'll keep trying - checking for nullptr seems reasonable. -- You are receiving this mail because: You are watching all bug changes.
