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

On Type Rename for #88424 #92597

Merged
merged 24 commits into from
Mar 18, 2020
Merged

On Type Rename for #88424 #92597

merged 24 commits into from
Mar 18, 2020

Conversation

octref
Copy link
Contributor

@octref octref commented Mar 12, 2020

This fixes #88424.

  • Adding editor option editor.renameOnType
  • Adding proposed API languages.registerOnTypeRenameProvider
  • Adding onTypeRenameProvider for HTML

Proposed API:

namespace languages {
	/**
	 * Register a rename provider that works on type.
	 *
	 * Multiple providers can be registered for a language. In that case providers are sorted
	 * by their [score](#languages.match) and the best-matching provider is used. Failure
	 * of the selected provider will cause a failure of the whole operation.
	 *
	 * @param selector A selector that defines the documents this provider is applicable to.
	 * @param provider An on type rename provider.
	 * @param stopPattern Stop on type renaming when input text matches the regular expression. Defaults to `^\s`.
	 * @return A [disposable](#Disposable) that unregisters this provider when being disposed.
	 */
	export function registerOnTypeRenameProvider(selector: DocumentSelector, provider: OnTypeRenameProvider, stopPattern?: RegExp): Disposable;
}

I think most of the cases work fine. There are 2 failing tests but I'm not sure why they happen, since the behavior in the editor is correct but the test still fails.

The two cases that's failing in the editor are:

  • Option + Delete and then undo for <div|></div>
  • Paste (with content div) and then undo for <div|></div>

The cause seems to be that setting EditorOperationType doesn't work for paste/delete-word.

But other than that, this seems to work fine. I'm planning to put it in insiders and turn it on for people who have enabled html.mirrorCursorOnType.

@octref octref added this to the March 2020 milestone Mar 12, 2020
@octref octref requested a review from alexdima March 12, 2020 19:26
@octref octref self-assigned this Mar 12, 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.

Looks good! I just left a few comments.

@@ -3316,7 +3316,7 @@ export const enum EditorOption {
autoClosingQuotes,
autoIndent,
automaticLayout,
autoRename,
renameOnType,
Copy link
Member

Choose a reason for hiding this comment

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

I like to keep these sorted alphabetically, please move it down a bit.

autoRename: register(new EditorBooleanOption(
EditorOption.autoRename, 'autoRename', false,
{ description: nls.localize('autoRename', "Controls whether the editor auto renames on type.") }
renameOnType: register(new EditorBooleanOption(
Copy link
Member

Choose a reason for hiding this comment

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

also here, please move it down a bit.

@@ -243,10 +283,6 @@ export class OnTypeRenameAction extends EditorAction {
kbExpr: EditorContextKeys.editorTextFocus,
primary: KeyMod.CtrlCmd | KeyCode.F2,
Copy link
Member

@alexdima alexdima Mar 13, 2020

Choose a reason for hiding this comment

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

I think Ctrl+F2/Cmd+F2 are already taken, so perhaps no keybindings is necessary here.

@alexdima
Copy link
Member

alexdima commented Mar 18, 2020

@octref There is still the problem of the Ctrl+F2 conflict, and some tests that need to be skipped.

👍 Otherwise, please merge. I suggest other work in HTML to adopt this feature should IMHO be done in another PR.

@octref octref merged commit f76ca9f into master Mar 18, 2020
@octref octref deleted the octref/live-rename branch March 18, 2020 19:29
@IllusionMH
Copy link
Contributor

With

Take HTML implementation out 600a776

does it really closes #88424 or it should be closed with HTML implementation merged?

@octref
Copy link
Contributor Author

octref commented Mar 18, 2020

@IllusionMH I pushed 01e01b1

@github-actions github-actions bot locked and limited conversation to collaborators May 2, 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.

Improve mirror cursor implementation with Synced Regions
3 participants