Fsharp: Ctrl+; keybinding (search solution explorer) doesn't trigger with F# files are in focus

[cartermp]

Repro steps

1. Create a new F# project.

2. Click in the editor on an F# project.

3. Press Ctrl+; keybinding.

Expected behavior

Focus is brought into the Solution Explorer search bar.

Actual behavior

Nothing happens. Doesn't appear broken on C# or XAML.

Known workarounds

None.

Related information

Latest F# nightly.

[saul]

This is also an issue with Ctrl+Q (and Alt+Enter?). Was it not fixed in #2606?

[vasily-kirichenko]

Alt+', Ctrl+;, Ctrl+Q do not work for me on latest nightly. Alt+Enter works, but I set the binding manually yesterday (and for Alt+' as well, it's stopped working).

[cartermp]

Seems like something serious has regressed and we're swallowing common keybindings. I have no idea what could be causing this, and lack context on anything which could have changed on the VS side of things. @JoshVarty would you know more?

[JoshVarty]

On 15.4.1.17031702 I can repro for Ctrl+; and Ctrl+Q. (Both global commands)

Alt+Enter and Alt+' work for me. For these two ensure you've installed 15.4.1.17031601 or later. Also ensure that you close all open tabs at least once after you've installed.

I'll investigate why global commands seem to be swallowed.

[JoshVarty]

Interestingly not all global commands are affected. Ctr+' (Team.TeamExplorerSearch) and Ctr+, (Edit.GoToAll) work fine.

Also this issue was also reported here: https://github.com/Microsoft/visualfsharp/issues/2335

[JoshVarty]

This doesn't look related to the F# Editor Factory. I'm still unable to use these commands in editors created by other factories. I open a file via:

1. Right click file in Solution Explorer
2. Choose Open With...
3. Select Source (Text) Editor Factory

More info:

Chord: Ctr+Q
pguidCmdGroup : {d63db1f0-404e-4b21-9648-ca8d99245ec3}
commandId : 18
CommandName: VSStd11CmdID.GlobalSearch

Chord: Alt+'
pguidCmdGroup : {d63db1f0-404e-4b21-9648-ca8d99245ec3}
commandId : 20
CommandName VSStd11CmdID.SolutionExplorerSearch

Both commands are routed through here for both F# and C#. This suggests that there aren't any problems with Roslyn's AbstractOleCommandTarget.

Next I'll look at the editor chain of IOleCommandTargets to figure out where this command should end up and why it's not making it there.

[majocha]

Commenting out the export here
https://github.com/Microsoft/visualfsharp/blob/30cd23e6f70b7688c034e2c1ea5eef9eed9b2be4/vsintegration/src/FSharp.Editor/Commands/FsiCommandService.fs#L76

fixes Ctrl-Q and Ctrl-;

[JoshVarty]

I believe the problem lies here: https://github.com/Microsoft/visualfsharp/blob/30cd23e6f70b7688c034e2c1ea5eef9eed9b2be4/vsintegration/src/FSharp.Editor/Commands/FsiCommandService.fs#L39-L44

I haven't written F# much but I believe this is always returning VSConstants.S_OK so the command is never routed back to the shell where it would be properly handled. Returning VSConstants.S_OK essentially says "I have handled the command".

I think you want something roughly like:

if nCmdId = uint32 VSConstants.VSStd11CmdID.ExecuteSelectionInInteractive then
    Hooks.OnMLSend projectSystemPackage.Value FsiEditorSendAction.ExecuteSelection null null
    VSConstants.S_OK
elif nCmdId = uint32 VSConstants.VSStd11CmdID.ExecuteLineInInteractive then
    Hooks.OnMLSend projectSystemPackage.Value FsiEditorSendAction.ExecuteLine null null
    VSConstants.S_OK
else
    OleConstants.OLECMDERR_E_NOTSUPPORTED

I'll test out a PR for this.

[JoshVarty]

@majocha I believe that should fix these keybindings but probably breaks Alt+Enter and Alt+'

[majocha]

Yes, it's just indication of the problem. I think the logic in the ifs ladder is a bit wrong and it returns S_OK where it shouldn't, swallowing some commands.

[majocha]

Probably we should rewrite it all using Roslyn helpers...

[JoshVarty]

It should be alright for now. I'm actually (in my normal work) redesigning commanding within the editor so Roslyn, F# etc. don't need to use this IOleCommandTarget at all. I'm hoping that should simplify things for all language services.

[majocha]

F# interface IOleCommandTarget with member x.Exec (pguidCmdGroup, nCmdId, nCmdexecopt, pvaIn, pvaOut) = if pguidCmdGroup = VSConstants.VsStd11 && nCmdId = uint32 VSConstants.VSStd11CmdID.ExecuteSelectionInInteractive then do Hooks.OnMLSend projectSystemPackage.Value FsiEditorSendAction.ExecuteSelection null null VSConstants.S_OK elif pguidCmdGroup = VSConstants.VsStd11 && nCmdId = uint32 VSConstants.VSStd11CmdID.ExecuteLineInInteractive then do Hooks.OnMLSend projectSystemPackage.Value FsiEditorSendAction.ExecuteLine null null VSConstants.S_OK elif pguidCmdGroup = Guids.guidInteractive && nCmdId = uint32 Guids.cmdIDDebugSelection then do Hooks.OnMLSend projectSystemPackage.Value FsiEditorSendAction.DebugSelection null null VSConstants.S_OK elif not (isNull nextTarget) then nextTarget.Exec(&pguidCmdGroup, nCmdId, nCmdexecopt, pvaIn, pvaOut) else VSConstants.E_FAIL
should work...

[JoshVarty]

Out of curiosity, do I need the do keyword on Hooks.OnMLSend projectSystemPackage.Value FsiEditorSendAction.ExecuteLine null null?

It seems work without it, but I don't know what it does.

[majocha]

No,, it's optional. I used it to indicate what doesn't return a value.

[saul]

@majocha while you're looking at this stuff, do you mind taking a look at why the "Move up" and "Move down" context menu commands on folders (I think?) in the solution explorer. I imagine it's exactly the same bug that you're seeing here.

Great work by the way :)

[JoshVarty]

@saul It's probably unrelated since the solution explorer handles commands independently from the editor. Solution folders have been a constant source of pain and introduce a class of problems of their own. :(

[majocha]

Yes, from debugging it seems FSharpFolderNode is never being queried for any FSharpProjectCmdSet commands.