Skip to content

Fix issues causing RStudio keybindings to be incorrectly shown in some contexts #7362

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

Merged
merged 7 commits into from
Apr 25, 2025

Conversation

jmcphers
Copy link
Collaborator

@jmcphers jmcphers commented Apr 23, 2025

Fixes an issue causing RStudio keybindings to show up in unwanted places. It turns out to be intractable to make extension keybindings work the way we want, so the fix is to delete the extension and move these keybindings into a new "Positron Keybindings" contrib, where we can avoid registering them entirely if they are not enabled.

A few notes:

  • Now that these keybindings aren't dynamic, changing the setting requires a restart.
  • Existing users will need to migrate to the new setting manually.
  • There is some ambiguity around Cmd and Ctrl handling on macOS. RStudio's behavior is to register keybindings against both (so that either works), but VS Code's behavior is typically to register keybindings against Cmd only (i.e. CmdCtrl resolves to Cmd on macOS and Ctrl elsewhere). In most cases I have tried to preserve the RStudio behavior by registering both, even though this pollutes the keymap more. (Confusingly enough the Ctrl key on macOS has the identifier WinCtrl in VS Code.)

Addresses #3993.

Release Notes

New Features

  • N/A

Bug Fixes

  • Fix issue causing RStudio keybindings to show up when they aren't enabled (RStudio Keymap shortcuts are visible even with keymap disabled #3993). Note that this necessitated moving the keybindings preference to Workbench > Keybindings (from Extensions > RStudio Keymap), so you will need to re-enable RStudio keybindings again if you use them.

QA Notes

As noted in the setting, a restart is required to get these bindings to become active.

@jmcphers jmcphers requested a review from melissa-barca April 23, 2025 20:00
Copy link

github-actions bot commented Apr 23, 2025

E2E Tests 🚀
This PR will run tests tagged with: @:critical

readme  valid tags

ContextKeyExpr.not('findInputFocussed'),
ContextKeyExpr.not('replaceInputFocussed')
),
primary: KeyMod.CtrlCmd | KeyMod.Shift | KeyCode.Enter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question here

"mac": "ctrl+shift+enter",
"when": "config.rstudio.keymap.enable && editorTextFocus && editorLangId == quarto && !findInputFocussed && !replaceInputFocussed",
"command": "quarto.runCurrentCell"

Comment on lines 266 to 269
id: 'workbench.view.scm',
weight: KeybindingWeight.WorkbenchContrib,
primary: KeyMod.CtrlCmd | KeyMod.Alt | KeyCode.KeyM
}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WinCtrl here as well?

"mac": "ctrl+alt+m",
"win": "ctrl+alt+m",
"linux": "ctrl+alt+m",
"key": "ctrl+alt+m",
"when": "config.rstudio.keymap.enable",
"command": "workbench.view.scm"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call, updated!

jmcphers and others added 2 commits April 24, 2025 14:06
@jmcphers jmcphers requested a review from melissa-barca April 24, 2025 21:12
Copy link
Contributor

@melissa-barca melissa-barca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jmcphers jmcphers merged commit 9c91cb3 into main Apr 25, 2025
8 checks passed
@jmcphers jmcphers deleted the bugfix/rstudio-keymap-visibility branch April 25, 2025 19:01
@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 2025
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.

2 participants