Do not add intermediate lines to jumplist with :<linenum> command.

[askreet]

Based on this discussion. (@the-mikedavis)

This patch:

• Only appends to the jumplist when the :<linenum> invocation is accepted with Enter (PromptEvent::Validate).
• Fixes what appears to be a bug in the jumplist behavior where the current pointer points past the end of the list when it's appended to, resulting in a "dead" invocation of C-o. This caused issues with the previous implementation especially, as it would append to the list twice implicitly during validate (since the movement logic previous lived outside the handler for the PromptEvent::Update event.

[pascalkuthe]

One small nit so this becomes easier to review.

I am not quite sure if the second fix is correct. Looking trough the code it seems intentional that current may point one past the end (the backward function explicitly checks for that) but I am not sure

[askreet]

I removed the jumplist changes but now it's in an interesting state where the jumplist works as expected, unless you rewind and then play back the jumplist to the end. If you do that, then doing another :<linenum> invocation replaces the tail of the jumplist. I'll do some more debugging later.

[pascalkuthe]

I removed the jumplist changes but now it's in an interesting state where the jumplist works as expected, unless you rewind and then play back the jumplist to the end. If you do that, then doing another :<linenum> invocation replaces the tail of the jumplist. I'll do some more debugging later.

You are onto something and that triggered me to take a hard look/think about the jumplist code (something I haven't looked at in too much detail so far).

It works as follows:

• push is called during a normal jump before the jump occurs so the location to jump pack to is saved. Current is placed one past the end here so that if you go backwards by one you end up at the the last saved jump
• If we jump backwards and are one past the end then the current location is pushed to the top of the jumplist so we van rewind back to it with forward
• You can not go one past the end with forward because what was previously one past the end is the positioning previously saved during backwards (forwarding past the end doesn't make sense since there is no jump saved)

That means you previous fix was indeed wrong and the code is working as intended. The rope as on for the odd behaviour you are seeing is that push_junp is being called at the wrong place here. It should be called before the new selection is set. If you look at the old implementation push_jump is called before doc.set"selection.So now you need to call push_jump before goto_line_without_jumplist is called.

We only ever save locations to junp back to and current points at the current position which we don't ever want to jump back to (unless we go forward) so it makes sense that push would overwrite that as it's essentially starying a new history from the cureent point. I think what is a bit confusing is the jumplist get initalized with a single jump. In my opinion initializing it as empty would be more correct as that single jump is always overwritten and never actually has an effext. That's only a mater if code clarity tough and does not need to be addressed here.

So for this PR you just need to move push_jump before goto_line_without_jumplist is called.

[askreet]

Thanks @pascalkuthe for the clarification, that does make sense. I think the code clarity could be improved and may try to rationalize about that in another PR, but will fix this one for now. Thanks again!

[askreet]

Alright I had an interesting thought about this whole thing this morning. Since we want to set a jumplist entry before moving the cursor to allow the user to navigate back to where they started and we need support for aborting the command, can we not just use the jumplist to accomplish both tasks?

The updated commit does the following:

• Track whether we are previewing a goto_line_number command within Editor, instead of tracking the last line number.
• Push a jump when we start previewing the command.
• Call jump_backward if the command is aborted.

This has the added benefit of retaining not only the starting line number, but the starting selection(s) as well. As far as I can tell, it works ideally when invoked with :goto <linenum>. However, the special handling of :<linenum> creates some edge cases:

• If the line entry is cleared by typing :12 followed by two backspaces, no abort event is sent to goto_line_number to clear the flag.
• If the line entry becomes invalid by typing, for example, :12 abc it stops sending events to goto_line_number and the flag is not cleared.

I have some ideas about how to resolve these, but am pretty new to the project so greatly interested in others' thoughts. My ideas are:

• Naïvely clear the flag when command mode is invoked (in command_mode) with a comment.
• If unmatched typed input (e.g. empty string or :12 ab) is found in command_mode's closure and the flag is set, send the Abort event to goto_line_number.

Architecturally I have an idea that might improve this, too. What if typed commands were trait objects that has their own state and tracked in the command mode handler as an Option<dyn TypedCommand> (where TypedCommand is a hypothetical trait, not the current struct)? Whenever an active typed command session was in progress but the command no longer matched its invocation pattern we could reason about having to abort that typed command in order to restore the Option to None and allow another typed command session to start. The added benefit here is there are a few flags on Editor that exist for tracking state of a typed command session that could be stored directly on the struct that implements TypedCommand. I can move this idea to a discussion if anyone thinks it has merit.

[the-mikedavis]

Pushing to the jumplist is potentially destructive: if you jump backwards with C-o a few times you can hit C-i to move forward again in the jumplist. But jump backwards with C-o and then push to the jumplist, the old history going forward is lost. Ideally we should only ever use the jumplist for real jumps rather than re-using it for implementation details like this.

I think the Option<dyn TypedCommand> sounds needlessly more complicated. Most typable commands don't need to store any internal state anyways and we try to minimize the use of traits in general since they bring a lot of complexity.

[pascalkuthe]

Pushing to the jumplist is potentially destructive: if you jump backwards with C-o a few times you can hit C-i to move forward again in the jumplist. But jump backwards with C-o and then push to the jumplist, the old history going forward is lost. Ideally we should only ever use the jumplist for real jumps rather than re-using it for implementation details like this.

I think the Option<dyn TypedCommand> sounds needlessly more complicated. Most typable commands don't need to store any internal state anyways and we try to minimize the use of traits in general since they bring a lot of complexity.

I agree I preferred the previous simpler fix just with the push_jump moved to the right place as I sugessted.
I would avoids messing with the jumplist which should really only be used for jumps that actually occur and never for previews.

If you want to restore the selection when aborting why not just store the previous selection on the editor instead of the previous line number (like we do now).
I think that might be a nice change in general if you call it last_selection as that field could be reused for other typable commands that preview the location we jump to which are not line based.

[askreet]

Pushing to the jumplist is potentially destructive: if you jump backwards with C-o a few times you can hit C-i to move forward again in the jumplist. But jump backwards with C-o and then push to the jumplist, the old history going forward is lost.

Ah, good point. Oh well, it was a fun exercise to learn more of the code :-).

I think the Option sounds needlessly more complicated. Most typable commands don't need to store any internal state

Fair point that there aren't many implementations that require this, I suppose I'm imagining it being generally useful for future commands. More or less complicated might be a matter of taste, I found it surprising that state was stashed in the editor struct disjoint from context of the typed command and the typed commands source file.

I agree I preferred the previous simpler fix just with the push_jump moved to the right place as I sugessted.

I don't remember the details but it ended up being a bit more complicated. I'll add some context when I work on the next commit if I can remember it. I think your suggestion below might address it.

If you want to restore the selection when aborting why not just store the previous selection on the editor instead of the previous line number (like we do now).

I like this idea, I'll pursue that next.

[askreet]

Stumbled on a neat "bug" in this last pass where we were passing line_number as the scrolloff value of ensure_cursor_in_view. Fixed in the last iteration.

[pascalkuthe]

that could cause very weird issues. Nice catch

[askreet]

New iteration is ready for review, appreciate all the pointers on this @the-mikedavis @pascalkuthe @trink!

I still think there's some edge cases around the flow of aborting the implicit :<linenum> syntax, but they weren't as obvious as with the jumplist approach (since the temporary jumplist entry would be 'leaked').

[pascalkuthe]

Going in the right direction 👍 The last_selection turned out to be an even better idea then I thaught :D There are still some edgecases left tough, I left comments about those.

[pascalkuthe]

I am pretty sure that PromptEvent::Validate can happen immidietly without reciving a PromptEvent::Update. In that case the command does nothing now wheras it worked correctly before. I think you can restructure the funciton slightly to account for that. First check if we are aborting => call abort_goto_line_number_preview and return.

Then run the code that is currently in the update branch.

Then check if we are validating and run the code here. last_selection can then be obtained with cx.editor.last_selection.clone().unwrap() cx.editor.last_selection.take().unwrap() (see below about the take, unwrap is fine as it's always initalized a few lines prior)

[askreet]

I am pretty sure that PromptEvent::Validate can happen immidietly without reciving a PromptEvent::Update.

How would that happen? Keybinding?

[pascalkuthe]

I think keybindings may do that. Haven't tested it but all other typed commands work when imminently called with PromptEvent::Validate so I wouldn't want to diverge from that here.

[askreet]

Re-reading your comment:

First check if we are aborting => call abort_goto_line_number_preview and return.

What do you mean by "first check"? The function only handles one event at a time, and it does call the abort function and return then an abort event is fired.

[pascalkuthe]

Right now you do a match on the event:

match event{
PromptEvent::Abort => /* abort */,
...
}

Instead use a simple if:

if event == PromptEvent::Abort{
// abort ...
return
}

// update selection

if event == PromptEvent::Validate{
// jumplist entry and clear last_selection
}

[askreet]

Sorry, but I don't understand the difference, both constructs result in the function returning after calling abort, no?

[pascalkuthe]

The difference is that you don't need to copy all the update code into the validate branch.

With a match the full thing would be:

match event{
PromptEvent::Abort => /* abort */,
PromptEvent::Update => /* update selection  */,
PromptEvent::Validate => /* update selection, add jumplist entry and clear last_selection */,
...
}

I suggested the structure above so you can share the update code between the Update and the Validate branch

[askreet]

Okay, I see what you're saying. I'm trying to figure out the case where validate is called with arguments without update having been called. If I can find that, I'll restructure the code accordingly.

[askreet]

@pascalkuthe You were, of course, exactly right -- binding a key in insert mode to :goto 123 sends only the Validate event, which currently doesn't work. New commit addresses that. Thanks for catching that and the detailed explanation!

[pascalkuthe]

Just one last nit, Apart from this, this LGTM now. Nice improvement both fixing the jumplist bug and making sure that we rust the full selection instead of just placing the primary cursor back on the original line.

[pascalkuthe]

Now that last_selection is always populated in update_goto_line_number_preview you should call unwrap here instead of if let. If last_selection somehow became None in the meantime that is a bug and therefore correct to panic rather than silently ignoring the problem. Fail hard and fail fast...

[askreet]

Fail hard and fail fast...

Interesting. I did consider this code path and had the opposite instinct. In the event of a code change that results in a bug I didn't want to crash and potentially lose a user's unsaved editor changes just because a jumplist entry couldn't be set. Is this approach to error handling standard for the project, and if so, documented for contributors?

Alternatively, we could use either a debug_assert! or else conditional with logging if you're concerned about detecting such a bug.

[pascalkuthe]

you can bail too in the else case if you are super concerned about the else case causing data loss but If you look throughout the codebase, unwrap and expect are used liberally for impossible code-paths. There was some discussion about adding a panic handler that backups unsaved work. Rust autoamtically panics on many common programmar bugs (out of bounds indexing, etc.) so that problem is not specific to this one case.

Right now the code looks like it's perfectly valid for there to be no last_selection when it should really be impossible. That is an anitpattern to me.. Using unwrap for code-paths that should never happens is a pretty common idiom in rust and something that makes code easier to read (which we care about) and makes it easier to find bugs.

[askreet]

No problem, happy to be consistent with the rest of the project. Pushed a fix for that and the clippy warnings.