Platform context keys doesn't work for command removals

[gandalfsaxe]

Issue Type: Bug

The new platform specific context key that words for key bindings (https://github.com/Microsoft/vscode/pull/54894/commits) doesn't work for "negative" commands, i.e. when the commands starts with a minus.

Easy way to reproduce:

1. Install Markdown All In One extension.
2. Remap markdown.etension.editing.toggleBold to Ctrl+B (or something else)
3. Add && isMac to end of "when" context of "positive command" - it still works! (⌘B toggles the sidebar)
4. Add && isMac to end of "when" context of "negative command" - it doesn't work anymore! (⌘B toggles bold in Markdown)

In other words, this works:

{
    "key": "ctrl+b",
    "command": "markdown.extension.editing.toggleBold",
    "when": "editorTextFocus && !editorReadonly && editorLangId == 'markdown' && isMac"
  },
  {
    "key": "cmd+b",
    "command": "-markdown.extension.editing.toggleBold",
    "when": "editorTextFocus && !editorReadonly && editorLangId == 'markdown'"
  },

This doesn't work:

{
    "key": "ctrl+b",
    "command": "markdown.extension.editing.toggleBold",
    "when": "editorTextFocus && !editorReadonly && editorLangId == 'markdown' && isMac"
  },
  {
    "key": "cmd+b",
    "command": "-markdown.extension.editing.toggleBold",
    "when": "editorTextFocus && !editorReadonly && editorLangId == 'markdown' && isMac"
  },

VS Code version: Code - Insiders 1.27.0-insider (8727085, 2018-08-14T05:12:11.579Z)
OS version: Darwin x64 18.0.0

System Info

Item	Value
CPUs	Intel(R) Core(TM) i7-4870HQ CPU @ 2.50GHz (8 x 2500)
GPU Status	2d_canvas: enabled checker_imaging: disabled_off flash_3d: enabled flash_stage3d: enabled flash_stage3d_baseline: enabled gpu_compositing: enabled multiple_raster_threads: enabled_on native_gpu_memory_buffers: enabled rasterization: unavailable_software video_decode: enabled video_encode: enabled webgl: enabled webgl2: enabled
Load (avg)	3, 4, 4
Memory (System)	16.00GB (0.15GB free)
Process Argv	/Users/gandalf/Documents/portable/Visual Studio Code - Insiders.app/Contents/MacOS/Electron
Screen Reader	no
VM	0%

Extensions (1)

Extension	Author (truncated)	Version
markdown-all-in-one	yzh	1.6.0

[gandalfsaxe]

Would suggest assignment to @Tyriar since he implemented #54894

[Tyriar]

@gandalfsaxe why are you using a when clause for this? Why not just:

  {
    "key": "cmd+b",
    "command": "-markdown.extension.editing.toggleBold"
  }

I suspect this is as designed since there's nothing special about isMac, it's just a regular context key (with a value that never changes).

[gandalfsaxe]

@Tyriar Basically I don't have a use case for it right now, but I fee like that's more coincidence than anything else.

The cmd key happen to not exist on Windows, so I can leave the command removal as you've quoted above without a when clause. But in principle one could be in a situation where one wanted to remove a key mapping from one platform, but not another. I don't see any reason why it shouldn't work though?

Actually now I have this:

{
  // toggleItalic: KEY CONFLICT - overwrites expandLineSelection
  {
    "key": "ctrl+i",
    "command": "markdown.extension.editing.toggleItalic",
    "when": "editorTextFocus && !editorReadonly && editorLangId == 'markdown' && isMac"
  },
  {
    "key": "cmd+i",
    "command": "-markdown.extension.editing.toggleItalic",
    "when": "editorTextFocus && !editorReadonly && editorLangId == 'markdown'" // FIXME: Windows only when supported (https://github.com/Microsoft/vscode/issues/56382)
  },
  {
    "key": "ctrl+shift+alt+i",
    "command": "markdown.extension.editing.toggleItalic",
    "when": "editorTextFocus && !editorReadonly && editorLangId == 'markdown' && isWindows"
  },
  {
    "key": "ctrl+i",
    "command": "-markdown.extension.editing.toggleItalic",
    "when": "editorTextFocus && !editorReadonly && editorLangId == 'markdown'" // FIXME: Windows only when supported (https://github.com/Microsoft/vscode/issues/56382)
  }
}

The removal of ctrl+i for italic for Windows actually conflicts with settings of ctrl+i for macOS, but luckily the addition of ctrl+i takes higher precedence. Again feels like a stroke of luck, but it'd be more "correct" to have them be platform specific?

Anyway I don't have any immediate use case for this feature, I just imagine it would be nice to have just in case - and I don't see any reason why it shouldn't work as one would expect for "negative commands". But if you're confident that there will never be a use case for this, you can of course ignore this 🙂

[Tyriar]

@gandalfsaxe I believe this is as designed as you cannot remove commands based on the when clause, they have to be the same. If you have keybinding a and -b, a still exists as they're different. It might seem unintuitive but that's the way the system was designed and it's fine imo once you understand this aspect.

Also on cmd, note that this will bind the super/windows key on windows and linux which may not be desirable. This is also by design I found as I created an issue on it recently, but luckily there's a workaround for that now.