Fixes 2019: Increment number crashes

[greg-enbala]

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!

[greg-enbala]

If it helps the discussion, why I'm a little lost is say you have selected all the - in the following: -1-1-2-1-1 So your selection would be: (-)1(-)1(-)2(-)1(-)1

When you increment the result will be: 00-100 And your selection would be: (0)(0)(-)1(0)(0)

When you then increment again, the result will be 01-99

There are 3 less number objects than your original selection had, and placing the extra cursors anywhere other than on the 2 remaining number objects seems arbitrary. If you wanted to force the same number of selections you could: (0)(1)(-)(9)(9) But that feels forced and doesn't represent any useful action you could take beyond (0)1(-)99

Say you had all charactes selected in -10 by selecting .{1} what are you then selecting when the result is -9? It seems there are cases where you can't have the same number of selections, and cases where having the same number of selections feels arbitrary and perhaps less useful than selecting some portion of what helix considers to be the resulting changes.

[the-mikedavis]

I would imagine the (-)1(-)1(-)2(-)1(-)1 case not making any changes at all for C-a/C-x

Instead C-a/C-x would only work if the were a number under the selection, and they would only touch the number under the selection, so

(-1)(-1)(-2)(-1)(-1)
(0)(0)(-1)(0)(0)
(1)(1)(0)(1)(1)

etc.

The behavior of C-a/C-x that tries to find the next number to increment breaks the select-then-act paradigm and is what makes the code for it complex.

[greg-enbala]

Gotcha. Possibly unintended consequences:

• 10(0)01 would become 10(1)01
• 10(9)00 would become 10(10)00
• 201(9)-12-12 would become 201(10)-12-12.
• ( ) 10 would be no changes

etc.

Happy to change the algorithm if that's the preferred direction!

[the-mikedavis]

Yep those are all expected. I'm not sure about how others feel about this change. The jumping behavior is convenient but I think it's a Vim relic that doesn't fit the paradigm.

[greg-enbala]

Whatcha wanna do when a selection spans multiple "incrementable" things.

For example: A selection that spans multiple individual numbers: (10 xx 10) or a large document where someone selects the whole document and C-as .

If the answer is "the exact selection must be parsable as something incrementable" then easy peasy.

If it's something like, "increment all the incrementable things inside a selection" it's a little more complicated. There's also the DateTimeIncrementor. Does the algorithm need to determine which incrementor to use based on the first incrementable thing or split them up somehow?

[the-mikedavis]

Yeah, "the exact selection must be parsable as something incrementable" is what I had in mind. Just trying each incrementor in sequence should do the trick - no two incrementors should be able to successfully handle the same input

[greg-enbala]

@the-mikedavis Significant changes to the algorithm pushed. Now it only operates on what is directly in the exact selection. Notable changes:

• Dates can only increment by day, DateTimes by minute
• Math for integers is saturating rather than wrapping.
• Incrementor trait removed as the incrementors just take and return data now, no searching needed.

[greg-enbala]

@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!

[itsgreggreg]

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