Fix jump_backwards behaviour when jumplist is at capacity

[thomasschafer]

This PR resolves some issues with the jumplist when the capacity is reached. In particular:

• Decrements the current position in the jumplist if elements are removed from the front: if an element in the jumplist shifts backwards then the index pointing to that element should shift backwards with it.
• Checks whether the previous position in the jumplist is the position that the cursor is currently on: if so, move another step back in the jumplist, as otherwise the cursor stays in the same place and nothing happens.

Fixes #10967.

[thomasschafer]

Realised that tests are failing - away from my laptop but will fix when I can

[the-mikedavis]

Thanks for opening this up! I've seen this bug before but couldn't figure out how to reproduce the behavior.

As you note in a comment in backwards, the issue is that we're using current as a sort of pointer and when we pop items from the front, the index of our "current" item changes. The fix for this I believe is smaller than what you have at the moment here: in backward we can make current mut and subtract the number of items we will remove in push (self.jumps.len() - JUMP_LIST_CAPACITY + 1) in the if self.current == self.jumps.len() block.

[thomasschafer]

Thanks for opening this up! I've seen this bug before but couldn't figure out how to reproduce the behavior.

As you note in a comment in backwards, the issue is that we're using current as a sort of pointer and when we pop items from the front, the index of our "current" item changes. The fix for this I believe is smaller than what you have at the moment here: in backward we can make current mut and subtract the number of items we will remove in push (self.jumps.len() - JUMP_LIST_CAPACITY + 1) in the if self.current == self.jumps.len() block.

Thank you for taking a look!

Is it not preferable to update current in push, rather than backwards, as that ensures that any other uses of push also ensure that current is valid? I can't see why we'd want to use push while allowing elements to be popped from the front of the jump list and not also keep current in sync.

Also, I know that the prev_jump == &cur_jump is a slightly different issue, but I still think it's worth fixing in this PR for the rare occasion where the cursor is already on the last jump position, to avoid jump_backward doing nothing in that case - unless you disagree?

[the-mikedavis]

push can modify self.current as it currently does on master but it can't automatically adjust the current binding in backward. I mean the current binding in the if let Some here:

helix/helix-view/src/view.rs

Line 66 in dbacaad

if let Some(current) = self.current.checked_sub(count) {

The current changes to push actually do not change the behavior. Decrementing self.current while popping elements isn't necessary because self.current isn't read anywhere between the popping and assignment of self.current = self.jumps.len(). And moving that assignment to the end of the block also doesn't have an effect because self.jumps.truncate(self.current) implies that self.current then equals self.jumps.len()

[thomasschafer]

push can modify self.current as it currently does on master but it can't automatically adjust the current binding in backward. I mean the current binding in the if let Some here:

helix/helix-view/src/view.rs

Line 66 in dbacaad

if let Some(current) = self.current.checked_sub(count) {

The current changes to push actually do not change the behavior. Decrementing self.current while popping elements isn't necessary because self.current isn't read anywhere between the popping and assignment of self.current = self.jumps.len(). And moving that assignment to the end of the block also doesn't have an effect because self.jumps.truncate(self.current) implies that self.current then equals self.jumps.len()

Ah I see, you're right - thanks for explaining. I've removed the changes from push. I've also updated backward in line with your comment here with a slight modification: I felt uneasy about performing the subtraction in backward to try and determine how many elements had been popped from the front in push, so I've updated push to return the number of elements removed from the front and backward now subtracts those from current, as per your comment. Hopefully this is okay but happy to make changes if not!