microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.45k stars 29.36k forks source link

Improve mirror cursor implementation with Synced Regions #88424

Closed octref closed 4 years ago

octref commented 4 years ago

Although some people like mirror cursor, it currently has issues. Many of these issues are existing behavior of multi-cursor. Some are hard to change, some can only be worked around in sophisticated ways, and some can only be fixed by code in vscode core.

UI wise, it has issues as well:

And in my own experience, it doesn't play nicely with Vim extension.

In short, multi-cursor was the closest we could get to auto rename tag. We had to choose it because it's the only option for making 2 changes with 1 document edit. But it's not exactly the behavior people want (people want changes reflected in the mirrored region, not adding one more cursor).

Additionally, people has expressed interest in having mirror cursor for JSX: microsoft/TypeScript#51832 And we think it would be pretty cool, if this works for loop variables, function params, etc. For example:

for (let i|=0; i| < 10; i|++) {
  count += i|
}

To fix the issues, and to make this feature available to more languages, @alexdima, @jrieken and I think it's best to add a new editor concept, synced regions. It would work this way:

Errors4l commented 4 years ago

This proposed solution seems like a good way to mirror the cursor without the current annoyances (or bugs).

That said, I think it's important not to take it too far when it comes to automatically mirroring other ranges. Changing html tags is relatively safe, but variable names could quickly cause problems that the user might not see until it's too late. We already have F2 for that (though the same could be said for HTML tags, I suppose)

Of course, it all depends on the implementation of the provider. But still: QKpx6O2t2S

octref commented 4 years ago

That said, I think it's important not to take it too far when it comes to automatically mirroring other ranges.

Agreed. We'll make this an opt-in.

ramyhhh commented 4 years ago

I would add tow more problems:

  1. copying and especially cutting causes unwanted behavior if open and close tag were on the same line.
  2. line moving (Holding Alt + up/down) also behaves weirdly

I suggest to tweak mirror cursor by only activating it when the whole tag is selected (which is easily done with a double click), it's true that this way is a bit limiting in terms of per character editing, but in most cases re-typing the whole tag again is not so bad.

octref commented 4 years ago

Just an idea: Having a key to enter the mode always makes it less discoverable. Maybe one implementation could be:

For example:

octref commented 4 years ago

I have tried out an editor contribution implementation with ICodeEditor#executeEdits and ICodeEditor#onDidChangeModelContent. Here's how it current works:

I also cannot interleave type with my edits precisely, so when user types fast, this breaks.

The good things are this implementation would fix most of the complaints:

synced-region

What I would need is an editor core concept/API, here's my sketches with @rebornix so far:

@alexdima Would love to hear your input here.

bmix commented 4 years ago

Nice, this definitely needs built in editor support!

As somebody working a lot with XML files, I'd really appreciate a solid mirroring of element tags and relatives. My use cases are:

While some of these currently can be done with various add-ons, they tend to break and the solutions are not solid.

May I advice against needing to select the whole tag before this action? That would make editing very anti-fluid, especially in context of intellisense completions or correcting typos.

Some other places, where this, or similar issues have arisen:

Of course, designing this API would require some philosophical evaluation about editor support vs. LSP support. My choice would be to keep this as simple as possible and leave the heavy lifting to an LSP, with a fallback for editor-only scenarios.

Thank you for your consideration.

octref commented 4 years ago

Reopening to track API proposal and other polish items.

octref commented 4 years ago

Summary of the changes:

When you disable editor.renameOnType, which is the default, you can use Cmd+Shift+F2 (Ctrl+Shift+F2 on Win/Linux) to enter the synced regions:

demo

When you enable editor.renameOnType, you would enter the synced region mode when your cursor moves onto it.

This feature is only available for HTML now. But other language extensions can easily adopt it:

/**
 * The rename provider interface defines the contract between extensions and
 * the live-rename feature.
 */
export interface OnTypeRenameProvider {
    /**
     * Provide a list of ranges that can be live renamed together.
     *
     * @param document The document in which the command was invoked.
     * @param position The position at which the command was invoked.
     * @param token A cancellation token.
     * @return A list of ranges that can be live-renamed togehter. The ranges must have
     * identical length and contain identical text content. The ranges cannot overlap.
     */
    provideOnTypeRenameRanges(document: TextDocument, position: Position, token: CancellationToken): ProviderResult<Range[]>;
}

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;
}

Known issues:

Fixes:

IllusionMH commented 4 years ago

Thanks! Really intrigued by these changes, will check them tomorrow.

One question

Typing or pasting content starting with whitespace will exit this mode

It will exit mode if content starts with space or contains space anywhere?

If I select h1 in opening tag <|h1|>Top</h1> and paste part copied from other tag h2 class="featured" it will: only replace open tag content, closing remains h1, update closing tag to h2 or else?

IllusionMH commented 4 years ago

The answer is - pasted as in in both places, so closing tag would also have class attribute :(

@octref is it possible to

  1. distinguish between renges where stopPattern should be checked/enforced (closing tag in HTML) and where it should apply all edits (opening tag in HTML)
  2. use \s as stopPattern instead of ^\s, and only apply part of that precedes stop pattern (first space in case of HTML) to closing tag and all to opening
bmix commented 4 years ago

Thank you for looking into this! :-)

This looks uncomfortable in so far, as it requires additional work, while typing.

Ideally, when you type < it offers you completion while you type, then, when you hit space it offers you attribute name completion, after you did =" it offers you attribute value completion and when you close via > it automagically creates the ending tag and when you add the / just before > then it removes the ending tag, making your element empty.

Renaming should happen from wherever you place the cursor in the element (start or end) and go on from there. If you paste text into the element name, it should just mirror this on the other side.

P.S. I am aware, that text-completion is job of the LSP backend, I just mentioned it for sake of completeness, so I can describe.

It would be really important, to have this feature work as seamlessly as possible, being part of the typing flow.

P.P.S. What screen recorders do you guys use? I tried to find an easy to use, up to date and free one Windows, that would allow me to create screen records for Github, but I found none.

Gama11 commented 4 years ago

@bmix On Windows, ShareX is the best option by far I think.

IllusionMH commented 4 years ago

Renaming should happen from wherever you place the cursor in the element (start or end) and go on from there. If you paste text into the element name, it should just mirror this on the other side.

It worked for me like this with "editor.renameOnType": true not sure what problem do you experience here.

P.P.S. What screen recorders do you guys use? I tried to find an easy to use, up to date and free one Windows, that would allow me to create screen records for Github, but I found none.

I've tried only few but found https://www.screentogif.com/ (+ pointed to downloaded https://ffmpeg.org/download.html ) as the best option for me. ffmpeg allows more export options from ScreenToGif + much better compression/quality

gjsjohnmurray commented 4 years ago

@bmx I have been having good results with ScreenToGif

octref commented 4 years ago

@IllusionMH

It will exit mode if content starts with space or contains space anywhere?

Any content that matches /^s/.

If I select h1 in opening tag <|h1|>Top and paste part copied from other tag h2 class="featured" it will: only replace open tag content, closing remains h1, update closing tag to h2 or else?

It doesn't match /^s so changes will be duplicated.

distinguish between renges where stopPattern should be checked/enforced (closing tag in HTML) and where it should apply all edits (opening tag in HTML)

No since we decided to make the feature language agnostic to editor, so other programming languages can implement this support.

use \s as stopPattern instead of ^\s, and only apply part of that precedes stop pattern (first space in case of HTML) to closing tag and all to opening

People will complain about pasting class="foo bar" doesn't work.

@bmix

Please open new issues with exact reproducible steps and expected behaviors.

IllusionMH commented 4 years ago

No since we decided to make the feature language agnostic to editor, so other programming languages can implement this support.

That's why my idea was to provide enforceStopPattern flag alongside of each range (or maybe inverted flag depending on defaults).

Return type of provideOnTypeRenameRanges will beProviderResult<Range[] | Array<{range: Range; enforceStopPattern?: boolean}>>;

HTML/JSX will have /\s/ as stop pattern TSX will have /[\s<]/ (because of generic components).

In this 3 cases LS will provide this flag when ranges should have different behavior between ranges - open/closing tags: everything is pasted in first range (open tag), and only part before stop pasted into second range (closing tag).

Other languages (or cases) - won't provide this flag - all places will have same treatment.

That's rough idea, however I'd like see what drawbacks it has.

octref commented 4 years ago

I'm not sure how that would help with replacing <|h1|></h1> with h2 class="foo".

Let's assume the stopPattern is only enforced for the opening tag but not closing tag, what error cases does that prevent?

IllusionMH commented 4 years ago

Looks like I wasn't clear what I mean under "stopPattern is enforced" and that meaning of stopPattern probably changes. So idea is like

In case of <|h1|></h1> with h2 class="foo" in my theory it should be: Return type

[
    {
        range: new Range(1, 2, 1, 4)
    },
    {
        range: new Range(1, 7, 1, 9),
        enforceStopPattern: true
    }
]

when paste of pastedValue = 'h2 class="foo"', stopPattern = /\s/ applied to:

  1. First range - stop pattern is not enforced. replaceString = pasteValue therefore h1 replaced with h2 class="foo" as is
  2. Second range - pattern is enforced. replaceString = pasteValue.split(stopPattern)[0] therefore h1 replaced with just h2

(yes, splitting is not optimal, but simples expample of how to take part before stop) If it's enforced for open tag, but not close tag result will be other way around - closing tag would have class, and opening tag will be `h2.


Not sure yet sure what behavior is better if pasted value starts with stopPattern.

Currently pasting class="foo" (starts with space) when <|h1|></h1> will result in < class="foo"></h1>. To reproduce this behavior it will be no-op for cases when enforceStopPattern: true, but paste as is to other places with enforceStopPattern: false

bergamin commented 4 years ago

@bmix In case you are looking for a mobile one as well, I've been using GIF Maker-Editor. It doesn't keep a logo or a button on top all the time and let you edit so you can cut off the start and end of the video/gif for not showing the parts you commanded it to start/end recording

I just wish it could share with Giphy app to instantly upload it there

IllusionMH commented 4 years ago

@octref any feedback about behavior proposed in https://github.com/microsoft/vscode/issues/88424#issuecomment-601415971 ?

octref commented 4 years ago

@IllusionMH I think stopPattern means full stop, not really "stop until here". Pasting to duplicate full content or not duplicating at all would be easy to understand. Pasting to only replace some content but not others is confusing, especially because the pattern is not visible to users.

Currently pasting class="foo" (starts with space) when <|h1|> will result in < class="foo">.

Without this feature, is that an edit you would do? If user's intention is to add class="foo" to h1, I think 99% of the cases is they either paste the attribute with or without leading space right after h1. As I said, the goal is not to guarantee all your edits result in valid HTML, the goal is to provide editing helper that's easy to understand and streamline your common edits.

octref commented 4 years ago

API proposal is tracked in https://github.com/microsoft/vscode/issues/94316. Will need to do that first before applying that to JSX / XML / etc.

IllusionMH commented 4 years ago

I think stopPattern means full stop, not really "stop until here"

can be changed to any meaningful name e.g. safeInsertPattern (which should match part that can be safely inserted where safeInsertPattern is enforced) etc. Main point - differentiate between ranges that should accept whole clipboard content vs. ranges that accept only "safe" part

Without this feature, is that an edit you would do? This is example of current behavior which won't be preserved if there are 2 kind of ranges described above.

No I will not paste [space]attr="value" while I have h1 selected, however I will definitely paste h2 class="featured" which unfortunately invalidates closing tag (and I'm not alone) and it might not be noticeable while closing tag is not visible in editor.