Skip to content

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.

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.

precondition: ContextKeyExpr.and(EditorContextKeys.writable, EditorContextKeys.hasRenameProvider),
kbOpts: {
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

4 participants