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

Implement virtual space #228680

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Implement virtual space #228680

wants to merge 29 commits into from

Conversation

x17jiri
Copy link

@x17jiri x17jiri commented Sep 15, 2024

Implementation of virtual space as discussed here: #13960

Examples of this feature:
rec.webm
rec2.webm

Most of the changes fall into one of these categories:

  • updated move commands to allow moving the cursor beyond end of line
  • code updated to properly handle column numbers beyond the end of line
  • CursorPosition.leftoverVisibleColumns replaced with columnHint, which contains the column number that we want to stay in. This makes the calculations easier, but the change needed to be propagated to a few places.

Notable changes in specific files:

  • replaceCommand.ts - updated to add spaces when typing beyond end of line
  • mouseTarget.ts - updated to generate column for mouse positions beyond end of line
  • viewLines.ts - this change was necessary to allow virtual space also on last line of a file
  • cursorWordOperations.ts - without this change, Ctrl + Right Arrow (next word) beyond end of line goes to the end of the line instead of the next line

Testing:

  • I added a simple unit test but can implement more

@x17jiri
Copy link
Author

x17jiri commented Sep 15, 2024

@microsoft-github-policy-service agree

@m6502
Copy link

m6502 commented Oct 12, 2024

I have been using @x17jiri 's fork for nearly 80 hours already and haven't crossed a single problem. Please review and approve this pull request.

@m6502
Copy link

m6502 commented Oct 14, 2024

@alexdima

Copy link

@m6502 m6502 left a comment

Choose a reason for hiding this comment

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

This is working very well for me.

Copy link

@kyriacosmichael kyriacosmichael left a comment

Choose a reason for hiding this comment

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

works ok

@r2d2Proton
Copy link

r2d2Proton commented Oct 19, 2024

Virtual Space is an absolute must. It is extremely annoying, very frustrating trying to find the cursor. But I am impressed with what has been done with VSCode. I am a little unsure, goofy new keyboard from Logitech, but it does seem the input response is a little slow. Have not worked with VSCode for very long, a couple of days but will revert back to VS if needed.

And how do I add it? Check for updates in VSCode - nothing. Enable Experimental Features?

@x17jiri
Copy link
Author

x17jiri commented Oct 19, 2024

@r2d2Proton This is not merged in official VSCode yet. If you want to try it you can download build artifacts from my fork:

https://github.com/x17jiri/vscode/actions/runs/11200839841

Then go to "File / Preferences / Settings" and find virtual space

@m6502
Copy link

m6502 commented Oct 22, 2024

@alexdima What more is required for this PR to be accepted?

@m6502
Copy link

m6502 commented Oct 31, 2024

So this is moving forward after all, thanks @alexdima, it's very much appreciated. I still not understand the "community pr approvals", do these have to be from random people from the community or they have to be from "official" collaborators from the project? If these have to be from official collaborators, could you please ask someone to take a look?

@m6502
Copy link

m6502 commented Nov 11, 2024

@aiday-mar Bless us! :-)

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.

This is an impressive PR and is very high quality! Thank you 👏

We do however have a few notes (thanks to @aiday-mar for testing this thoroughly):

  • this functionality is enabled in simple code editors like the one used for the git commit message, we need to disable it in such contexts.
  • dragging & moving the mouse over the scrollbar puts the cursor at the end of the line
  • the scroll width computation needs to be updated to take into account the cursor position, e.g. when pressing and holding arrow right the cursor goes beyond the viewport but the editor doesn't scroll to reveal the cursor because the scroll width of the editor is defined only by the maximum line width.
  • rendering of selection should become full-width and is incorrect when the first line starts on virtual whitespace

But even before tackling these issues, I think there is one fundamental change that I think we must do:

  • We should maintain the invariant that the cursor model position is clamped to the text length. The cursor state (SingleCursorState) contains its view position/selection, its model position/selection and some more state (like the leftoverVisibleColumns which is used when moving it up and down to recover from short lines). The view position is the position of the cursor on the view model (which is obtained by transforming the model with wrapping points, folding lines, etc.). Most of the code in our code-base only sees / deals with the model position. So most of the code in our code-base has no idea that the editor's view does wrapping, etc. I suggest we tackle virtual whitespace in the same way and we "hide" from the outside world (maybe reusing SingleCursorState's view position/selection or maybe with the help of additional fields?) that the cursor floats in virtual space and we only add this information to the ones who really need to know. So I would like that such a final PR would not touch the text buffer implementation at all, including that it has no visible consequence in the vscode API (the cursors would just appear as if they sit at the end of the line despite them being rendered in virtual whitespace).

Thank you for your work and please let me know if you'd be interested in tweaking the solution.

@x17jiri
Copy link
Author

x17jiri commented Nov 14, 2024

@alexdima Thank you for review. I'll try to make the update. Is there anyone I can bother if I have questions?

@alexdima
Copy link
Member

@x17jiri Sure, I'm happy to help. I will keep an eye out, but if I'm not answering timely please drop me an email too.

@AnrDaemon
Copy link

this functionality is enabled in simple code editors like the one used for the git commit message, we need to disable it in such contexts.

Pardon me, why?

@x17jiri
Copy link
Author

x17jiri commented Nov 25, 2024

@alexdima I'm still finishing some minor details but wanted to check the basic design.

There are some differences from line wrapping which mean the information cannot stay only in the view model.

  1. The code converts positions from view model to model and back. Unlike with wrapping, information gets lost when we clip positions. One particular place where this can be problem is selection tracking during edits.
  2. edit commands need to know when to insert spaces

What I did:

  • for view model, SingleCursorState stores positions in virtual space
  • for model, positions are clipped at line length and excess columns are stored in leftoverVisualColumns
  • as long as we have a partial cursor state, we can recover the other part without loss of information

However, some parts of code use only Selection or Position, not SingleCursorState. For those:

  • I added function getSelectionInVirtualSpace() which returns model selection unclipped (column = column + leftoverVisualColumns) and used it instead of getSelection() where the information is needed
  • code that's not updated to use getSelectionInVirtualSpace() will see model selections clipped at line length

I updated two low level functions in editor/common/cursor/cursor.ts so it's easy to make updates on higher levels and often it's enough to just replace getSelection() with getSelectionInVirtualSpace()

  • when pushing edits, virtual space positions are handled by inserting spaces
  • selection tracking algorithm should now handle virtual space positions. Although this may still need some tuning

I reverted changes in text buffer and also previous commits "fix hover" and "fix move lines", which are not necessary with this new approach.

@m6502
Copy link

m6502 commented Dec 6, 2024

@x17jiri you are doing a really impressive job. Thank you very much.

@x17jiri
Copy link
Author

x17jiri commented Dec 6, 2024

@m6502 Thank you. I think I've caught most bugs in this new version, but if you have some time, any help with testing will be appreciated :-)

Build artifacts can be found here: https://github.com/x17jiri/vscode/actions/runs/12200915080

@aiday-mar aiday-mar removed their assignment Dec 11, 2024
@m6502
Copy link

m6502 commented Dec 20, 2024

@m6502 Thank you. I think I've caught most bugs in this new version, but if you have some time, any help with testing will be appreciated :-)

I have been using the latest releases for working and I'm still getting a good time with them. I'm not finding anything broken in my workflow.

- Convert virtualSpace into an editor option instead of a text model option
- Reduce public exposure of SelectionInVirtualSpace, try to encapsulate it in a few places
- Remove extra tracked ranges data and insert concrete spaces straight in the commands themselves
- Make `ICoordinatesConverter`'s `validateViewPosition` and `validateViewRange` aware of virtual space
- Breaks the status bar support which needs now to be redone
@x17jiri
Copy link
Author

x17jiri commented Jan 7, 2025

@alexdima , thank you for moving this forward. I did a few tests of this updated implementation and here is what I found:

  • Adding cursors using Alt + Shift + Up/Down clips the new cursors at the end of line. I think this is because by default, this command works with model lines

  • When there is a copilot suggestion and you press right arrow, the suggestion stays displayed and the cursor jumps after it. I think this is because the "cursor state changed" signal is not emitted when only the virtual space offset changes

  • Moving lines using Alt + Up/Down loses virtual space position

  • If the buffer is updated in the background we lose position. I use format on save and when a file is saved, the cursor may or may not get clipped depending on whether the formatter made changes. This may be difficult to fix without the extra tracked ranges data and it may actually be mitigated by auto save. At the moment of auto saving, the cursors are not in virtual space

I can help either with more testing or trying to fix some of these things.

@m6502
Copy link

m6502 commented Feb 10, 2025

I have been using the last build artifact for Windows (# 31) for two months already. As it didn't build for Linux this weekend I started using # 30 and I'm very happy with it too.

@alexdima The remaining issues seem to be just small nuisances. Can't we move forward and integrate this on the main build? This is a feature that some of us really crave about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants