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.

Sign up for GitHub

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

Jump to bottom

Separate jump behavior from increment/decrement #4123

Merged
merged 22 commits into from
Jan 16, 2023
Merged

Separate jump behavior from increment/decrement #4123

merged 22 commits into from
Jan 16, 2023

Conversation

greg-enbala
Copy link
Contributor

@greg-enbala greg-enbala commented Oct 6, 2022 •
edited

Fixes: #2019

This fixes a couple situations I found that cause undesired behavior when incrementing integers:

Trailing underscores, eg: 9_. When incrementing can cause an infinite loop.

I fixed this by not including trailing underscores in the found character Range. This means the incrementing code can
not care about them. Alternatively we could generate the range as it currently is and make the incrementing code ignore the trailing underscores.

A number that changes width when incrementing eg -1 to 0 or 9 to 10 can crash.

Crux of the problem: When multiple numbers are incremented at the same time, there can be multiple shifts in the document length. For example, selecting all the negative signs in -1 -1 -1 and incrementing will cause the document to be 3 characters shorter. If you naively kept the selection positions tied to the changes, you'd end up with invalid selections that may be entirely off the end of the resulting document. This PR attempts to solve this problem by keeping a running count of how many characters are added or subtracted per applied change and adjusting the resulting selections accordingly.

Notable code structure changes in this PR

  • NumberIncrementor renamed to IntegerIncrementor to more accurately reflect what it does.
  • Incrementor trait removed.

Notable behavior changes in this PR

  • Incrementing only effects what is directly selected. This means the number of selections will never change and selections don't go searching to the end of the line to find something to increment.

  • DateTimeIncrementor can now only ever increment the Day if what is selected is a Date, or the minutes if what is selected is a DateTime. This is a consequence of only operating directly on what is currently selected.

  • Numbers can no longer wrap on increment/decrement. The addition/subtraction is saturating. I made this change as I feel it's surprising behavior.

Happy to change this PR to work however folks want it to, add tests, rename things, change the algorithm, whatever folks think is best for helix. Hope this helps!

@kirawi kirawi added A-core Area: Helix core improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Oct 6, 2022
@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Oct 6, 2022
@greg-enbala greg-enbala marked this pull request as ready for review October 7, 2022 19:45
@kirawi kirawi added C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2022
@greg-enbala greg-enbala force-pushed the 2019_increment_number_crashes branch 2 times, most recently from db28af9 to db96d60 Compare October 9, 2022 10:19
the-mikedavis
the-mikedavis reviewed Oct 12, 2022
View reviewed changes
helix-core/src/increment/integer.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
greg and others added 2 commits October 11, 2022 18:29
shorten algorithm a little bit
80beea7
@greg-enbala @the-mikedavis
use method min rather than function
a8524b6 
Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
@greg-enbala greg-enbala force-pushed the 2019_increment_number_crashes branch 2 times, most recently from 033ced4 to 1f9d56b Compare October 12, 2022 03:58
@archseer archseer added this to the 22.11 milestone Nov 17, 2022
the-mikedavis
the-mikedavis reviewed Nov 21, 2022
View reviewed changes
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.

Ah sorry, this fell off of my radar for a while 😅

Since we're deleting so much of both modules here, I wonder if we should pull the increment function for datetimes and the increment function for numbers into just one helix_core::increment module as datetime and integer functions. What do you think? OTOH having separate modules is nice since they're small and well-contained

helix-core/src/increment/date_time.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Nov 22, 2022
@archseer archseer removed this from the 22.11 milestone Nov 29, 2022
@greg-enbala
Copy link
Contributor Author

@the-mikedavis

Ah sorry, this fell off of my radar for a while 😅

Since we're deleting so much of both modules here, I wonder if we should pull the increment function for datetimes and the increment function for numbers into just one helix_core::increment module as datetime and integer functions. What do you think? OTOH having separate modules is nice since they're small and well-contained

All good! Sorry to take so long to respond, I was on a bit of a vacation.

Either in their own modules or in one module is fine by me. I've left them in their own modules for now as it's nice to have the tests contained per type but if folks want them in the same module that's all good by me!

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 21, 2022
@dxmh dxmh mentioned this pull request Dec 22, 2022
Closed
@the-mikedavis the-mikedavis added this to the next milestone Jan 14, 2023
the-mikedavis
the-mikedavis approved these changes Jan 16, 2023
View reviewed changes
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 pushed a merge commit to resolve conflicts and bring in the changes from #4418.

Thanks for working on this! I like how much this cleaned up and simplified the increment/decrement code 🚀

@the-mikedavis the-mikedavis added the R-breaking-change This PR is a breaking change for some behavior label Jan 16, 2023
@the-mikedavis the-mikedavis changed the title Fixes 2019: Increment number crashes Separate jump behavior from increment/decrement Jan 16, 2023
@the-mikedavis the-mikedavis merged commit 60f84be into helix-editor:master Jan 16, 2023
@itsgreggreg
Copy link

Hey good stuff! Same person as the greg-enbala, I'll be using this account instead of that one from now on.

gibbz00 pushed a commit to gibbz00/helix that referenced this pull request Jan 17, 2023
@greg-enbala @gibbz00
Separate jump behavior from increment/decrement (helix-editor#4123)
f57a865 
increment/decrement (C-a/C-x) had some buggy behavior where selections
could be offset incorrectly or the editor could panic with some edits
that changed the number of characters in a number or date. These stemmed
from the automatic jumping behavior which attempted to find the next
date or integer to increment. The jumping behavior also complicated the
code quite a bit and made the behavior somewhat difficult to predict
when using many cursors.

This change removes the automatic jumping behavior and only increments
or decrements when the full text in a range of a selection is a number
or date. This simplifies the code and fixes the panics and buggy
behaviors from changing the number of characters.
kirawi pushed a commit to kirawi/helix that referenced this pull request Jan 25, 2023
@greg-enbala
Separate jump behavior from increment/decrement (helix-editor#4123)
820e469 
increment/decrement (C-a/C-x) had some buggy behavior where selections
could be offset incorrectly or the editor could panic with some edits
that changed the number of characters in a number or date. These stemmed
from the automatic jumping behavior which attempted to find the next
date or integer to increment. The jumping behavior also complicated the
code quite a bit and made the behavior somewhat difficult to predict
when using many cursors.

This change removes the automatic jumping behavior and only increments
or decrements when the full text in a range of a selection is a number
or date. This simplifies the code and fixes the panics and buggy
behaviors from changing the number of characters.
@the-mikedavis the-mikedavis mentioned this pull request Feb 11, 2023
Closed
@the-mikedavis the-mikedavis mentioned this pull request Mar 7, 2023
Closed
This was referenced Jun 13, 2023
Closed
Closed
@pascalkuthe pascalkuthe mentioned this pull request Jul 16, 2023
Closed
@the-mikedavis the-mikedavis mentioned this pull request Aug 13, 2023
Closed
@pascalkuthe pascalkuthe mentioned this pull request Apr 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@the-mikedavis the-mikedavis the-mikedavis approved these changes

Assignees
No one assigned
Labels
A-core Area: Helix core improvements C-bug Category: This is a bug R-breaking-change This PR is a breaking change for some behavior S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
23.03
Development

Successfully merging this pull request may close these issues.

Panic when incrementing two -1 values
5 participants
@greg-enbala @the-mikedavis @itsgreggreg @archseer @kirawi