Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add context keys for platforms #54894

Merged
merged 2 commits into from
Aug 7, 2018
Merged

Add context keys for platforms #54894

merged 2 commits into from
Aug 7, 2018

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Jul 23, 2018

Fixes #8962


This allows the following when clauses:

  • "when": "isLinux"
  • "when": "isMac"
  • "when": "isWindows"
  • "when": "!isLinux" (isMac || isWindows)
  • "when": "!isMac" (isLinux || isWindows)
  • "when": "!isWindows" (isLinux || isMac)

@alexdima
Copy link
Member

👍

@alexdima alexdima removed their assignment Jul 30, 2018
@alexdima
Copy link
Member

This PR will make PR #45995 obsolete

@Tyriar Tyriar added this to the August 2018 milestone Jul 31, 2018
@Tyriar Tyriar requested a review from bpasero July 31, 2018 01:47
@gandalfsaxe
Copy link

gandalfsaxe commented Aug 14, 2018

Does this work for settings as well? Or should I make a separate issue for that?

Awesome feature by the way!

@Tyriar
Copy link
Member Author

Tyriar commented Aug 14, 2018

@gandalfsaxe it doesn't work for settings as they don't support when clauses, I image the solution there would look similar to language-specific settings:

{
  [os=linux]: {
    blah
  }
}

I think there's an issue tracking this.

@gandalfsaxe
Copy link

@Tyriar That makes sense. That'd be really great to have!

I don't think there is an existing issue. The only I could find was #5595, which was closed with reference to the very issue this PR fixed, #8962. Unless you can find a relevant issue, I'll make a new one shortly 🙂

@Tyriar
Copy link
Member Author

Tyriar commented Aug 15, 2018

@gandalfsaxe I opened that issue, thanks.

@gandalfsaxe
Copy link

gandalfsaxe commented Aug 16, 2018

@Tyriar Did this PR stop working? I can make a separate issue, but they don't seem to work for me anymore. It's like they're completely ignored now.

See the following video, where I try to set a shortcut for git.clean (discard):

I'm on latest 1.27 Insiders btw.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 23, 2018

@gandalfsaxe seems to be working for me? 😕

Does the actual keybinding work? Maybe it's an issue with the context menu?

@gandalfsaxe
Copy link

@Tyriar Sorry I was too quick there. I did a bit of testing, and it turned out to be quite a subtle issue not related to this PR at all - it seems to work perfectly.

I tried to use ⌃⌘D as a shortcut, and it turnes out that this is an undocumented global shortcut for looking up the selected word in the macOS Dictionary app. But it doesn't show up in System Preferences -> Keyboard -> Shortcuts. Instead you have to run a terminal command and restart: https://apple.stackexchange.com/questions/22785/how-do-i-disable-the-command-control-d-word-definition-keyboard-shortcut-in-os-x

It seems like some other people were affected by this in Emacs. Good to know, and bad on Apple for hiding this shortcut so thoroughly. Anyway, the when clause works fine 🙂

Btw It seems like the issue with the context menu illustrated above is half fixed:

  • It no longer shows the Windows shortcut when there is a Mac shortcut also
  • However if only a Windows shortcut is set, it does show this even There is still a bit of an issue in that the context menu show the Windows shortcut even though it doesn't actually work - so it's cosmetic, but misleading.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 24, 2018

@sbatten keybindings that don't match the context are showing in the context menu? See #54894 (comment)

@sbatten
Copy link
Member

sbatten commented Aug 24, 2018

@Tyriar yea, I just tested this on an old 1.22 build, it looks like the context menus and application menu ignore context for keybindings.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OS Specific Keybindings
5 participants