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

Improve mirror cursor implementation with Synced Regions #88424

Closed
octref opened this issue Jan 10, 2020 · 23 comments · Fixed by #92597
Closed

Improve mirror cursor implementation with Synced Regions #88424

octref opened this issue Jan 10, 2020 · 23 comments · Fixed by #92597
Assignees
Labels
editor-synced-region Issues related to synced region functionality in editor feature-request Request for new features or functionality on-testplan
Milestone

Comments

@octref
Copy link
Contributor

octref commented Jan 10, 2020

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:

  • It's confusing which cursor in the main cursor
  • Content changes can happen outside viewport, and there's no indication
  • The multiple cursors aren't shown in scrollbar/minimap

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:

  • We introduce an API, OnTypeRenameProvider
  • When cursor moves in a doc, VS Code asks this provider for a list of ranges
  • These ranges are marked as in sync
  • Any content change (type, delete, paste) will cause all other synced regions to update in one edit
  • We can have a setting to switch between semi-automatic (press a shortcut to enter this mode) and full-automatic (like current mirror cursor).
@octref octref added feature-request Request for new features or functionality editor-synced-region Issues related to synced region functionality in editor labels Jan 10, 2020
@octref octref added this to the Backlog milestone Jan 10, 2020
@Errors4l
Copy link

Errors4l commented Jan 13, 2020

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
Copy link
Contributor Author

octref commented Jan 13, 2020

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
Copy link

ramyhhh commented Feb 10, 2020

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
Copy link
Contributor Author

octref commented Feb 11, 2020

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

  • When you are on the edge of a synced region
  • You made a delete edit that changed content of one synced region
  • Automatically enter this mode

For example:

  • Source is <div|></div>, the two div being the synced regions
  • You press delete
  • Editor sees that you did a delete on the edge of one region
  • Editor enters synced region mode and execute the delete on both regions, so the content becomes <di|></di|> and your changes are synced

@octref
Copy link
Contributor Author

octref commented Feb 18, 2020

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

  • I type i
  • I call executeEdits(source, edits, endCursorState) to make an edit and set cursor ending position
  • 🐛 The end of typing i sets cursor state, so my endCursorState is lost

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:

  • Space and paste with leading space could work
  • Editor commands only run with one cursor, so they are not duplicated

synced-region

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

  • Core: Have a type/delete/paste/cut interceptor that dispatches the mirrored changes
  • Editor contrib: Query the OnTypeRenameProvider for synced regions. Keep all synced regions as markers

@alexdima Would love to hear your input here.

@bmix
Copy link

bmix commented Mar 10, 2020

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:

  • cut/delete element tag
  • rename element tag
  • un/indent element tag
  • visually mark opening and closing element tag
  • jump to matching (opening/closing) element tag
  • if I close an element inline (making it effectively an empty node, like <elem attr="value" />), the closing tag, that was before, needs to be erased as well.
  • commenting out a single element tag should also comment out its closing part, but leave all other nodes, in-between, untouched:
<doc>
  <!-- <element1> -->
	<element2>
    </element2>
  <!-- </element1> -->
</doc>
  • outside of XML scope: parens, curlies, quotes, etc.

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
Copy link
Contributor Author

octref commented Mar 18, 2020

Reopening to track API proposal and other polish items.

@octref
Copy link
Contributor Author

octref commented Mar 18, 2020

Summary of the changes:

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

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:

  • All changes in the synced regions are synced
  • Use ESC to exit this mode
  • Typing or pasting content starting with whitespace will exit this mode

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

bmix commented Mar 19, 2020

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
Copy link
Contributor

Gama11 commented Mar 19, 2020

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

@IllusionMH
Copy link
Contributor

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
Copy link
Contributor

@bmx I have been having good results with ScreenToGif

@octref
Copy link
Contributor Author

octref commented Mar 19, 2020

@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
Copy link
Contributor

IllusionMH commented Mar 19, 2020

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 be ProviderResult<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
Copy link
Contributor Author

octref commented Mar 19, 2020

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
Copy link
Contributor

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
Copy link

@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
Copy link
Contributor

@octref any feedback about behavior proposed in #88424 (comment) ?

@octref
Copy link
Contributor Author

octref commented Apr 2, 2020

@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
Copy link
Contributor Author

octref commented Apr 2, 2020

API proposal is tracked in #94316. Will need to do that first before applying that to JSX / XML / etc.

@IllusionMH
Copy link
Contributor

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.

@github-actions github-actions bot locked and limited conversation to collaborators May 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
editor-synced-region Issues related to synced region functionality in editor feature-request Request for new features or functionality on-testplan
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants