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

Git: add default keybinding for Stage / Revert / Unstage selected lines #93564

Closed
isidorn opened this issue Mar 27, 2020 · 10 comments · Fixed by #93706
Closed

Git: add default keybinding for Stage / Revert / Unstage selected lines #93564

isidorn opened this issue Mar 27, 2020 · 10 comments · Fixed by #93706
Assignees
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@isidorn
Copy link
Contributor

isidorn commented Mar 27, 2020

Git extension needs to use the context isInDiffEditor and add keybindings only when that context is set.

Ctrl/Cmd + K, Cltr/Cmd + S / R / U makes good sense for defaults.

fyi @joaomoreno @jvesouza

@isidorn isidorn added help wanted Issues identified as good community contribution opportunities feature-request Request for new features or functionality accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues labels Mar 27, 2020
@isidorn isidorn added this to the March 2020 milestone Mar 27, 2020
@isidorn isidorn self-assigned this Mar 27, 2020
@sportshead
Copy link
Contributor

@isidorn Some shortcut confilcts:
CmdKCmdS is used by show keyboard shortcuts.
CmdKCmdU is used by remove line comment.

@isidorn
Copy link
Contributor Author

isidorn commented Mar 27, 2020

@sportshead thanks for pointing out. Yeah we need to find some free keybindings.
However this would only be in the diff editor view. So even if we overwrite some like those I think it is fine.

@sportshead
Copy link
Contributor

sportshead commented Mar 27, 2020

@isidorn I suppose we could overwrite Remove Line Comment, but I don't think it'd be best to overwriteShow Keyboard Shortcuts, as that's a settings page and we'll probably get an issue or too about that sometime down the road.
CmdKCmdG is free, perhaps implement another chord? But maybe another accessibility issue for nested chords...

@ChrisPapp
Copy link
Contributor

@isidorn Following discussion at #93498, I would like to work on this issue, too.

I also think that we should not override "Open Keyboard Shortcuts".
Since staging, is (in my mind) connected with "git add", maybe we could use
Ctrl/Cmd + K,Ctrl/Cmd + A

While Ctrl/Cmd + K,Ctrl/Cmd + G is also free, but the 'G' key is close to the 'R' key, so it's easier to call the Revert command by mistake.

@jvesouza
Copy link

Forgive my ignorance, but would it be possible to use something like ctrl + k ctrl + shift + s or ctrl + k alt + s?
Thanks.

@ChrisPapp
Copy link
Contributor

@jvesouza @isidorn

OK, so I am sending a PR with the following keybindings:
git: Stage selected ranges ctrl + k, ctrl + shift + s
git: Unstage selected ranges ctrl + k, ctrl + u
git: Revert selected ranges ctrl + k, ctrl + r

@ChrisPapp
Copy link
Contributor

Actually
git: Stage selected ranges ctrl + k, ctrl + alt + s
is better because ctrl + k, ctrl + shift + s is used for Saving without Formatting.

ChrisPapp added a commit to ChrisPapp/vscode that referenced this issue Mar 29, 2020
git.stageSelectedRanges   -> ctrl + k, ctrl + alt + s
git.unstageSelectedRanges -> ctrl + k, ctrl + u
git.revertSelectedRanges  -> ctrl + k, ctrl + r

Closes microsoft#93564
@isidorn
Copy link
Contributor Author

isidorn commented Mar 30, 2020

Great, thanks a lot. I have commented on the PR.
As mentioned in the PR @joaomoreno is back from vacation and this is not urgent, thus assigning to him and April milestone.

@isidorn isidorn removed the help wanted Issues identified as good community contribution opportunities label Mar 30, 2020
@isidorn isidorn modified the milestones: March 2020, April 2020 Mar 30, 2020
@ChrisPapp
Copy link
Contributor

ChrisPapp commented Mar 30, 2020

@isidorn @joaomoreno
Thanks for the quick review.
However I found another possible conflict with the default keybindings.
On my VS Code version which I have downloaded and installed,
Ctrl + K Ctrl + R - the keybinding we want to assign to Revert Selected Ranges- is used by the command "Help: Keyboard Shortcuts Reference".

However this command is not available on the version of VSCode I built from the source code, that's why I did not notice it at first.
For some reason KeybindingsReferenceAction.AVAILABLE is false and the command is not registered:

if (KeybindingsReferenceAction.AVAILABLE) {
registry.registerWorkbenchAction(SyncActionDescriptor.create(KeybindingsReferenceAction, KeybindingsReferenceAction.ID, KeybindingsReferenceAction.LABEL, { primary: KeyChord(KeyMod.CtrlCmd | KeyCode.KEY_K, KeyMod.CtrlCmd | KeyCode.KEY_R) }), 'Help: Keyboard Shortcuts Reference', helpCategory);
}

Also I don't understand why the GitHub CI threw an error. Should I worry about it?

@joaomoreno joaomoreno removed their assignment Mar 31, 2020
@isidorn isidorn added the verification-needed Verification of issue is requested label Apr 28, 2020
@roblourens roblourens added the verified Verification succeeded label Apr 29, 2020
@github-actions github-actions bot locked and limited conversation to collaborators May 24, 2020
@isidorn
Copy link
Contributor Author

isidorn commented Jun 25, 2020

Please note that due to conflict with the uncomment line which @sportshead mentioned I had to change the unstange from ctrl+k, ctrl+u to ctrl+k, ctrl+n

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants