Add an action for focusing and de-focusing the Terminal Panel.
FEATURE: 185096
FIXED-IN 20.04.0
elvisangelaccio | |
rominf |
Dolphin |
Add an action for focusing and de-focusing the Terminal Panel.
FEATURE: 185096
FIXED-IN 20.04.0
No Linters Available |
No Unit Test Coverage |
Nope. I'm against changing the behaviour of F4. There could be some running application opened in the panel (watch ls for example). That means that Ctrl+D is not longer an option. Showing the Close button is not an option too because it's inconsistent with other panels (I mean when the panels are locked).
My concern here is mainly that having an extra/different shortcut is not very intuitive or user-friendly. I'd suggest two steps:
- This Diff: Make Focus also open the Terminal.
Done.
- This Diff: Make Focus also defocus the Terminal.
Done.
- New Diff: Discuss re-assigning F4 and how to handle the caveats.
Declined. See above.
Also, the patch is broken: Triggering the shortcut without the Terminal panel open crashes Dolphin for me.
Thank you. I fixed it.
Much better than before, but I think when moving the focus back from the Terminal, it should be in the main viewport and not in the URL bar (there are separate shortcuts for focussing that already).
It moves focus to the previously activated widget. It's not hard to focus on the main viewport (actually this variant was implemented in the previous commit), but I think this behavior is better.
Correct, but as URL bar and main viewport are a single widget, in most cases it will not do what I believe users will expect it should do. Normally you navigate in the main view, then open the terminal, then navigate some more. Focussing the URL bar in that case is not helpful. Perhaps when this widget gets focus, it should default to the main view and not the URL bar? You might want to look into that.
actually this variant was implemented in the previous commit
Unless I missed something, your previous iteration had no unfocussing at all.
OK. Agreed. I changed back to m_activeViewContainer->setFocus(Qt::FocusReason::ShortcutFocusReason);
Unfocussing does not work for me:
The de-focus doesn't quite work for me; the terminal does correctly lose focus, but it isn't clearly moved anywhere else.
Great, now it works! I have one aesthetic/usability suggestion: if a file or folder is selected when the main panel is de-focused in favor of the terminal panel, turn the selection indicator into a thin underline. This would visually communicate that something else has the focus. Then when focus is restored, the thin underline would return to being a full selection indicator.
@ngraham That's in no way related to this patch, and you know it ;)
Besides, it is questionable: Item-focus (I'm not talking about widget focus) and selection are distinct properties, the only thing which should change on losing widget-focus is the colour saturation. Yes, you can move icon focus to elements which are not selected…
src/dolphinmainwindow.cpp | ||
---|---|---|
42 ↗ | (On Diff #28549) | Please remove the unrelated changes to the includes |
1190–1196 | Please add braces. Maybe this could also go in a private slot rathern than a lambda. | |
src/panels/terminal/terminalpanel.h | ||
58–64 | QWidget::setFocus(), QWidget::hasFocus() and QWidget::setFocus(Qt::FocusReason reason) are not virtual, I'm not sure this is a good idea. |
src/panels/terminal/terminalpanel.h | ||
---|---|---|
58–64 | I've tried to use focusProxy but it didn't work (maybe it's just me). What do you propose? |
src/dolphinmainwindow.cpp | ||
---|---|---|
1174 ↗ | (On Diff #28546) | This action is not the Tools menu. Either add it there or remove Tools from the translation context ;) |
1176–1186 ↗ | (On Diff #28546) | Please use a dedicated function. |
src/panels/terminal/terminalpanel.h | ||
58 ↗ | (On Diff #28546) | Should be const |
Sorry for the delay. The new action says "Focus Terminal Panel" even when the action will actually focus the view. Should we dynamically update the action text to "Focus/Defocus Terminal Panel" ?
src/dolphinmainwindow.cpp | ||
---|---|---|
1590 ↗ | (On Diff #28546) | Please use the same context we use above ("@action:inmenu Tools") Same for the i18n calls below. |
@elvisangelaccio I could use home help on this. After rebasing on the stable branch and testing again, the menu item does not toggle the focus like it should. I swear this worked before but now it doesn't anymore. :( m_terminalPanel->terminalHasFocus() does not seem to return the correct result and I'm having trouble figuring out why.
I think what was not working before was in fact a Konsole bug that has since been fixed in https://bugs.kde.org/show_bug.cgi?id=411181. Landing this now.