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

keep (cursor) position when exactly replacing text #5930

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pascalkuthe
Copy link
Member

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 pascalkuthe added C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 11, 2023
@pascalkuthe
Copy link
Member Author

pascalkuthe commented Feb 11, 2023

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)

@archseer
Copy link
Member

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

@pascalkuthe
Copy link
Member Author

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 pascalkuthe added the A-core Area: Helix core improvements label Feb 12, 2023
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

I like the look of this 👍. I left a minor typo-fix comment and looks like this will need to be rebased on top of the recent optimizations

helix-core/src/transaction.rs Outdated Show resolved Hide resolved
the-mikedavis
the-mikedavis previously approved these changes Jul 9, 2023
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.

only keep selections invariant under replacement

Keeping selections unchanged if they are inside an exact replacement
is intuitive. However, for diagnostics this is not desirable as
helix would otherwise fail to remove diagnostics if replacing parts
of the document.
@archseer archseer added this to the next milestone May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Helix core improvements C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cursor shifts on multiple selection letter case changes
3 participants