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

editor: introduce anchor selection #97912

Merged
merged 2 commits into from
May 18, 2020
Merged

editor: introduce anchor selection #97912

merged 2 commits into from
May 18, 2020

Conversation

isidorn
Copy link
Contributor

@isidorn isidorn commented May 15, 2020

fixes #95894

This PR introduces the following commands:

  • Set Selection Anchor Cmd + K, B keybinding
  • Go to Selection Anchor
  • Select from Anchor to Cursor Cmd + K, K keybdining
  • Cancel Selection Anchor Esc keybinding

As for changning default keybindings I woud like to have this in insiders so users try it out and then we can change it.

Potential follow up work:

  1. Unit tests - not sure how much we gain
  2. Make the Anchor Selection color themable - currently is just usses the status bar default blue
  3. Better solution than using width with !important.

@alexdima let me know what you think

@isidorn isidorn added the accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues label May 15, 2020
@isidorn isidorn added this to the May 2020 milestone May 15, 2020
@isidorn isidorn requested a review from alexdima May 15, 2020 13:13
@isidorn isidorn self-assigned this May 15, 2020
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

LGTM. I just left some small tweaks suggestions.

@@ -45,6 +45,7 @@ import 'vs/editor/contrib/viewportSemanticTokens/viewportSemanticTokens';
import 'vs/editor/contrib/wordHighlighter/wordHighlighter';
import 'vs/editor/contrib/wordOperations/wordOperations';
import 'vs/editor/contrib/wordPartOperations/wordPartOperations';
import 'vs/editor/contrib/anchorSelect/anchorSelect';
Copy link
Member

Choose a reason for hiding this comment

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

Small nit: this list was alphabetically sorted.

precondition: SelectionAnchorSet,
kbOpts: {
kbExpr: EditorContextKeys.editorTextFocus,
primary: KeyChord(KeyMod.CtrlCmd | KeyCode.KEY_K, KeyCode.KEY_K),
Copy link
Member

Choose a reason for hiding this comment

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

We should look if cmd+k cmd+k is available. You can check the availability across OSes here -- https://github.com/microsoft/vscode-docs/tree/master/build/keybindings

precondition: undefined,
kbOpts: {
kbExpr: EditorContextKeys.editorTextFocus,
primary: KeyChord(KeyMod.CtrlCmd | KeyCode.KEY_K, KeyCode.KEY_B),
Copy link
Member

Choose a reason for hiding this comment

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

We should look if cmd+k cmd+b is available. You can check the availability across OSes here -- https://github.com/microsoft/vscode-docs/tree/master/build/keybindings

className: 'selection-anchor'
}
}]);
this.decorationId = newDecorationId.length === 1 ? newDecorationId[0] : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

It is safe to use newDecorationIds[0]. The length will always be 1 because 1 new decoration is passed in above.

private editor: ICodeEditor,
@IContextKeyService contextKeyService: IContextKeyService
) {
this.selectionAnchorSetContextKey = SelectionAnchorSet.bindTo(contextKeyService);
Copy link
Member

Choose a reason for hiding this comment

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

I think this should also listen to editor.onDidChangeModel and reset the context key if the editor gets a new model set.

@isidorn
Copy link
Contributor Author

isidorn commented May 18, 2020

@alexdima thanks a lot for the review. Tackled all comments.
Also updated keybinding to be cmd +k, cmd +b, cmd+k, cmd+k
Only the cmd+k, cmd+k was taken but only for the keybindings editor. So I think we are good.

Merging.

@isidorn isidorn merged commit 7f49bf1 into master May 18, 2020
@isidorn isidorn deleted the isidorn/selection-anchor branch May 18, 2020 13:24
@github-actions github-actions bot locked and limited conversation to collaborators Jul 2, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility: Keyboard shortcuts to set start and end selection
2 participants