Add action for focusing Terminal Panel
AcceptedPublic

Authored by ngraham on Mar 2 2018, 6:12 AM.

Details

Reviewers
elvisangelaccio
rominf
Group Reviewers
Dolphin
Summary

Add an action for focusing and de-focusing the Terminal Panel.

FEATURE: 185096
FIXED-IN 19.04.0

Test Plan
  • Hit Ctrl++F4 or click ToolsFocus Terminal Panel or ControlToolsFocus Terminal Panel
  • If the Terminal Panel was closed, it opens and gains focus
  • If the Terminal Panel was open but unfocused, it gains focus
  • If the Terminal Panel was open and focused, focus returns to the view

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D10959
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9958
Build 9976: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Dolphin. · View Herald TranscriptMar 2 2018, 6:12 AM
rominf requested review of this revision.Mar 2 2018, 6:12 AM
rominf edited the summary of this revision. (Show Details)Mar 2 2018, 6:14 AM
rominf edited the test plan for this revision. (Show Details)
rominf added a reviewer: Dolphin.
rominf added a project: Dolphin.
rkflx added subscribers: ngraham, rkflx.EditedMar 2 2018, 1:01 PM

Another idea would be to make F4 open the Terminal if it was closed, and focus the Terminal if it was already open. This would be in line with standard behaviour for text-input widgets toggled by a keyboard shortcut (see also D10246#200226). Kate already has this behaviour: Set F4 for Focus Terminal, and you'll even have de-focus right there.

The only caveat would be that closing the Terminal would have to be done differently (Kate also has this problem), but I guess Terminal users know about Ctrl+D? Or we could always show the Close button? Or the menu entry is enough?

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.
  • This Diff: Make Focus also defocus the Terminal.
  • New Diff: Discuss re-assigning F4 and how to handle the caveats.

@ngraham Thoughts?


Also, the patch is broken: Triggering the shortcut without the Terminal panel open crashes Dolphin for me.


Edit: The linked bug wants defocus too!

rominf updated this revision to Diff 28402.Mar 2 2018, 1:30 PM
  • Show terminal panel on Focus Terminal action
rominf updated this revision to Diff 28403.Mar 2 2018, 1:44 PM
  • Unfocus on Focus Terminal action if terminal panel is already focused
rominf added a comment.Mar 2 2018, 1:44 PM

Another idea would be to make F4 open the Terminal if it was closed, and focus the Terminal if it was already open. This would be in line with standard behaviour for text-input widgets toggled by a keyboard shortcut (see also D10246#200226). Kate already has this behaviour: Set F4 for Focus Terminal, and you'll even have de-focus right there.

The only caveat would be that closing the Terminal would have to be done differently (Kate also has this problem), but I guess Terminal users know about Ctrl+D? Or we could always show the Close button? Or the menu entry is enough?

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.

rkflx added a comment.Mar 2 2018, 1:52 PM

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).

rominf added a comment.Mar 2 2018, 1:55 PM

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.

rkflx added a comment.Mar 2 2018, 2:03 PM

It moves focus to the previously activated widget.

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.

rominf updated this revision to Diff 28406.Mar 2 2018, 2:11 PM
  • Make action Focus Terminal unfocus terminal
rominf added a comment.Mar 2 2018, 2:12 PM

It moves focus to the previously activated widget.

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);

rkflx added a comment.Mar 2 2018, 2:16 PM

Unfocussing does not work for me:

  • Use arrow keys to move between files.
  • Press shortcut, type command, press shortcut again.
  • Try to use arrow keys again, observe they do not work.

The de-focus doesn't quite work for me; the terminal does correctly lose focus, but it isn't clearly moved anywhere else.

rominf updated this revision to Diff 28411.Mar 2 2018, 2:47 PM
  • Make Focus Terminal correctly de-focus
rominf added a comment.Mar 2 2018, 2:47 PM

The de-focus doesn't quite work for me; the terminal does correctly lose focus, but it isn't clearly moved anywhere else.

My bad. Now it works fine.

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.

rkflx added a comment.Mar 2 2018, 4:04 PM

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…

elvisangelaccio requested changes to this revision.Mar 3 2018, 12:27 PM
elvisangelaccio added a subscriber: elvisangelaccio.
elvisangelaccio added inline comments.
src/dolphinmainwindow.cpp
42

Please remove the unrelated changes to the includes

1239–1245

Please add braces. Maybe this could also go in a private slot rathern than a lambda.

src/panels/terminal/terminalpanel.h
59–65

QWidget::setFocus(), QWidget::hasFocus() and QWidget::setFocus(Qt::FocusReason reason) are not virtual, I'm not sure this is a good idea.

This revision now requires changes to proceed.Mar 3 2018, 12:27 PM
rominf updated this revision to Diff 28481.Mar 3 2018, 1:55 PM

Add braces

rominf marked an inline comment as done.Mar 3 2018, 1:59 PM
rominf added inline comments.
src/panels/terminal/terminalpanel.h
59–65

I've tried to use focusProxy but it didn't work (maybe it's just me). What do you propose?

rominf updated this revision to Diff 28546.Mar 4 2018, 7:08 AM

Rebase on master

rominf updated this revision to Diff 28549.Mar 4 2018, 7:15 AM

Remove the removal of unused #include

rominf marked an inline comment as done.Mar 4 2018, 7:16 AM
rominf updated this revision to Diff 28674.Mar 5 2018, 8:06 AM

Rebase on master

rominf updated this revision to Diff 29140.Mar 10 2018, 9:28 AM
  • Use focusProxy for Terminal Panel
rominf marked 2 inline comments as done.Mar 10 2018, 9:28 AM

Please review

src/dolphinmainwindow.cpp
1236

This action is not the Tools menu. Either add it there or remove Tools from the translation context ;)

1238–1248

Please use a dedicated function.

src/panels/terminal/terminalpanel.h
59

Should be const

rominf updated this revision to Diff 29828.Mar 18 2018, 12:00 PM
rominf marked 3 inline comments as done.
  • Take criticism into account
rominf updated this revision to Diff 29829.Mar 18 2018, 12:04 PM
  • "Focus Terminal" -> "Focus Terminal Panel"
rominf updated this revision to Diff 29830.Mar 18 2018, 12:06 PM
  • focusTerminal -> focusTerminalPanel

Am I wrong of defocus does not work? Did we agree to ignore it?

Am I wrong of defocus does not work? Did we agree to ignore it?

It works!

ngraham commandeered this revision.Feb 22 2019, 3:06 PM
ngraham added a reviewer: rominf.
Restricted Application added a subscriber: kfm-devel. · View Herald TranscriptFeb 22 2019, 3:06 PM
ngraham updated this revision to Diff 52308.Feb 22 2019, 3:25 PM

Yoink and rebase on current master

ngraham updated this revision to Diff 52310.Feb 22 2019, 3:47 PM

Try that again

ngraham updated this revision to Diff 52315.Feb 22 2019, 4:13 PM
  • Make it work
  • Add shortcut
  • Add menu items to Tools menu in main window, dolphinpart, and control menu
ngraham updated this revision to Diff 52316.Feb 22 2019, 4:21 PM

Use a more appropriate icon

ngraham edited the summary of this revision. (Show Details)Feb 22 2019, 4:24 PM
ngraham edited the test plan for this revision. (Show Details)

All right, I think this is ready for review again.

rominf accepted this revision.Feb 23 2019, 2:25 PM

Looks good for me!

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" ?

ngraham updated this revision to Diff 54158.Mar 17 2019, 11:29 PM

Conditionally update menu item text

src/dolphinmainwindow.cpp
1675

Please use the same context we use above ("@action:inmenu Tools")

Same for the i18n calls below.

ngraham updated this revision to Diff 54284.Mar 18 2019, 11:02 PM

Add context to i18n calls

ngraham marked an inline comment as done.Mar 18 2019, 11:02 PM
elvisangelaccio accepted this revision.Mar 20 2019, 7:20 PM
This revision is now accepted and ready to land.Mar 20 2019, 7:20 PM

Thanks! 19.04 or master?

We are still in time for 19.04 ;)

ngraham updated this revision to Diff 54520.Mar 21 2019, 11:25 PM

Rebase on Applications/19.04

@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.

Too late for 19.04 anyway, sorry for the delay :(

Can you check again on master?