helix icon indicating copy to clipboard operation
helix copied to clipboard

keep (cursor) position when exactly replacing text

Open pascalkuthe opened this issue 1 year ago • 4 comments

Fixes #5910

Whenever a document is changed helix maps various positions like the cursor or diagnostics through the ChangeSet applied to the document.

Currently, this mapping handles replacements as follows:

  • Move position to the left for Assoc::Before (start of selection)
  • Move position to the right for Assoc::After (end of selection)

However, when text is exactly replaced this can produce weird results where the cursor is moved when it shouldn't. For example if foo is selected and a separate cursor is placed on each character (s.<ret>) and the text is replaced (for example rx) then the cursors are moved to the side instead of remaining in place.

This change adds a special case to the mapping code of replacements: If the deleted and inserted text have the same (char) length then the position is returned as if the replacement doesn't exist.

pascalkuthe avatar Feb 11 '23 22:02 pascalkuthe

I was not 100% sure if this is the right behavior for diagnostics. Initally I thaught this was the case but on second thaught I came to the conclusion that this behavior is only disable for selections.

Exact replacements are pretty rare to begin with and basically always intentional by the user. However replacing one variable of equal length with another variable of equal length should instantly remove the associated diagnostic instead of keeping it. Therefore I changed the behavior described above to be configurable with a parameter that is enabled for selections but disabled for diagnostics (the only two applications for this mapping)

pascalkuthe avatar Feb 11 '23 22:02 pascalkuthe

I think this should be a different assoc type, I didn't port it all from CodeMirror: https://github.com/codemirror/state/blob/2117f5b58d040284f1e04ec054a1f2c79bf86a30/src/change.ts#L5-L16

archseer avatar Feb 12 '23 02:02 archseer

Various usages here https://github.com/search?q=org%3Acodemirror+MapMode&type=code

archseer avatar Feb 12 '23 02:02 archseer

I added the BeforeSticky and AfterSticky variants to assoc. I originally went with a separate parameter because this is an orthogonal property that only applies to a single edgecase while in most cases the behavior remained unchanged. I added some helper functions to make checking against tese variuants slightly cleaner and now it does look nicer than before.

However none of the variants from the source you linked @archseer match the behavior I implemented here so I had to comeup with my own name. I think this usecase is probably pretty specific to a multi-cursor modal editor so other implementations don't need to handle it.

pascalkuthe avatar Feb 12 '23 14:02 pascalkuthe