Unable to next/previous via keybindings when diff editor is not focused

[rob3c]

• VSCode Version: 1.7.0-insider
• OS Version: Windows 8

Steps to Reproduce:

1. bind ctrl+down and ctrl+up to workbench.action.compareEditor.nextChange and workbench.action.compareEditor.previousChange with when set to textCompareEditorVisible.
2. open the git panel with some changes to view.
3. single-click a file in the CHANGES list to select it and show its diff on the right.
4. try hitting ctrl+down and/or ctrl+up to navigate the changes while the git panel still has focus.

Navigating the file diff with ctrl+down/up used to work when single-clicking a file in the CHANGES list when this feature was first released. This broken sometime within the past several versions (I should've reported the regression sooner), and now you have to actually focus a file by double-clicking a CHANGES entry or manually clicking in one of the two displayed files on the right. This has become increasingly annoying enough that I finally remembered to make an issue about it :-)

Anyway, please restore the previous behavior when you have a chance, as the when context name suggests it should merely be visible rather than focused! The feature was really nice and streamlined before, but has become a bottleneck now for us keyboard navigators. With the CHANGES list in continual focus, it was really nice to be able to up/down in the list to select files, and then ctrl+up/down to scan through their changes.

Thanks in advance!

[roblourens]

Yeah, it works without the 'when', so maybe textCompareEditorVisible is only applied when the editor is focused, like you say.

[rob3c]

@roblourens that's right, it can certainly work as a global shortcut without the when context. Unfortunately, there are only so many good keyboard shortcuts that avoid random awkwardness, so it's nice that there's support for the when context available to overload nice ones like ctrl+down/up instead of having to choose a single behavior for all contexts.

For example, there's already another useful mapping for ctrl+down/up with the when: editorTextFocus context that allows keyboard scrolling in the editor when the diff viewer isn't visible. I use it heavily, even with it's current broken behavior across page boundaries (#2091). Hopefully, they'll restore the previous behavior soon so the diff files don't require focusing in order for diff navigation to work!

[bpasero]

@rob3c does it reproduce if you assign other keys (F10, F11)?

[rob3c]

@bpasero Unfortunately, no. Even using keys that don't already have other keybindings like F6 and F10 (F11 is bound to toggleFullScreen), it only works when the document in the editor has focus rather than just being visible.

It seems to be a change in the textCompareEditorVisible context behavior, because any key bindings will work when ...nextChange and ...previousChange are set as global shortcuts without the when specifier, but none of them work with that when setting without focusing the document.

UPDATE: oops, you were asking if the bug is reproduced with other keys...YES! (answered too quickly)

[bpasero]

@rob3c it does not reproduce for me on Windows so I am clueless. If someone wants to step in a provide a PR, feel free.

[roblourens]

I'm reproing it on OSX. I can try to debug if you have some pointers.

[bpasero]

Am I doing something wrong?

[roblourens]

No, that looks exactly like what I'm doing.

[rob3c]

@bpasero Yes, you're setting global handlers without the when specifier. That works fine. Try this:

{ "key": "F10",    "command": "workbench.action.compareEditor.nextChange",
                                    "when": "textCompareEditorVisible" },
{ "key": "F11",      "command": "workbench.action.compareEditor.previousChange",
                                    "when": "textCompareEditorVisible" },

[roblourens]

Oh, good catch

[bpasero]

I can reproduce but I have no clue why it fails. @alexandrudima @jrieken when textCompareEditorVisible is set to true in https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/browser/parts/editor/textDiffEditor.ts#L261 I still need to move focus into the editor before the keybindings in #14124 (comment) work

My gut feeling is that this was caused by introducing a scoped keybinding service per editor, though I thought we took this out again.

[jrieken]

I have reverted my changes a long time ago. My guess is that this happened during the editor action refactoring

[alexdima]

@jrieken IMHO this is caused by the introduction of a context per editor slot.

keyboard shortcuts:

{ "key": "F10",    "command": "workbench.action.compareEditor.nextChange",
                                    "when": "textCompareEditorVisible" },
{ "key": "F11",      "command": "workbench.action.compareEditor.previousChange",
                                    "when": "textCompareEditorVisible" }

To re-state what the OP is saying:

In the past

• have two editors side by side
• leave focus in the one in the right hand side for example
• go to the git viewlet
• click on a change
• the right hand side should turn to a diff editor
• focus stays in the git viewlet
• F10 would move the unfocused diff editor to the next change

Today

• same steps
• F10 does not move the unfocused diff editor to the next change

I have added a console.log to check what context service we get in the ctor at https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/browser/parts/editor/textDiffEditor.ts#L69 and it is not the global context key service.

Before the refactoring, there was only the global context key service and one for each editor instance. Now we have the extra 3 for the 3 slots and the diff editor is getting one of the children context key services. Therefore, when the focus is not in the diff editor, the context key is not found (in the global context).

[alexdima]

I suggest as a fix to somehow get ahold of the global context key service but even in that case it seems to be the code was buggy before, as in case you had two diffs opened, the key would be set to true twice, then when closing the first diff (meanwhile the other one is still open), the key would be set to false. @bpasero

[rob3c]

Thanks, everyone, for looking into fixing this! I'm wondering if maybe this issue is also affecting some other keyboard shortcuts more generally?

For example, I've notice that ctrl+alt+enter no longer works to replace all instances when doing search/replace (ctrl+h) in an editor, and it doesn't matter whether it's global to a file or just within a selected region. However, ctrl+shift+1 does work to just replace the next instance. The replaceAll button works, just not the default keyboard binding.

[jrieken]

This is a problem with the DiffNavigation actions because they are local to one diff editor and therefore not actionable globally. The same is true for things like 'Find References'.

As it was already mentioned, either there is only one occurrence of the textCompareEditorVisible context key and that is stored in the global context or the navigate commands work without that, first trying to find an active diff editor, like Cmd+F

[bpasero]

The command was actually always fit to be handled from the global context. Pushed a fix to not set the context per editor but globally.