[v4] Undo/redo selection merging issue, plus other odd behavior

[njx]

I spent some time today testing out undo/redo selection in more depth, and in general it seems to be working well. I found one issue which I think is worth fixing, and another odd behavior which I think is less important to fix.

The main issue is that I think the time-based selection merging should only happen for a subset of navigation operations, and not happen by default for most things. As I was using it, it made sense to merge things like arrow-key navigations or page up/down, but it didn't make as much sense to merge things like rapid "find next" operations, for example.

My suggestion would be to add an argument to setSelection/setSelections that indicates whether this selection change is mergeable, and default it to false; then pass true from the navigate-by-char, navigate-by-word, and navigate-by-page operations.

I could make a pull request for this if you'd like.

The other slightly odd issue I found:

1. Make a selection
2. Undo selection
3. Make another selection
4. Redo selection several times

Result: Selection “redoes” jumping back to the beginning of the document, and then to the selection made in step 1. I would expect that after step 3, the "redo selection" stack should be clear (by analogy to what happens with edits), and so redo selection should do nothing in step 4.

FWIW, I noticed that Sublime has similar behavior, and it's actually even weirder - in some cases it seems to actually lose the selection undo stack as well. (Though it doesn't skip back to the beginning of the document - it just jumps straight to the first selection you made.) So I think CM's current behavior is actually better, though still a little odd.

I think the problem is that you don’t want selection changes to just clear the redo stack, because you don’t want them to blow away real edits that are on the redo stack. Ideally, I think you'd want a selection change to remove any selection changes on the redo stack up to the next edit.

However, I don't feel strongly that this needs to be fixed, especially if it would complicate the implementation unduly.

[marijnh]

The first patch should address the second issue you mention.

As for the first, I would really like to avoid having yet another boolean argument to all selection functions. The case you mention (searching) is addressed by the second patch. Are there other cases you are worried about?

[njx]

What I was thinking was that the time-based merging should really only be restricted to the basic navigation operations, and that other selection operations should never merge. So perhaps you could just have some internal argument that would let you specify merging just for the move-by-char/move-by-word/move-by-page operations, and not expose it in the public API.

The heuristic you coded up basically makes sense, but one case it wouldn't handle would be if there were something like a bookmarking feature that identified different points (cursors) in the file, and the user switched between different ones. I think they would expect those all to be recorded in the selection history.

[marijnh]

If it's going to exist internally, it will also have to be exposed externally. When people write new cursor motion commands, they will want the same kind of control that built-in commands have.

How about a method forceSelectionHistory (not sure about name) that can be called after changing the selection to ensure it is pushed into the undo history?

[njx]

That's true, but I feel like it's going to be significantly more common for people to want to create navigation operations that shouldn't have their selections merged, so it makes more sense for that to be the default rather than requiring more work. It's similar to undo - you want to merge nearby typing edits, but you don't usually want to merge most other operations.

That said, if you were planning to also keep the heuristic that avoids merging non-empty selections, then I guess you would only need to call this special function if you're setting a cursor selection and you think the previous selection might have been a cursor. That seems a little weird from an API point of view though. The boolean on the selection APIs seems cleaner, though I appreciate not wanting to have endless lists of optional arguments.

[marijnh]

Good point. I've updated the selection methods to take an options object, rather than a boolean flag, in the attached patch. If you were already using that boolean argument, you'll have to update your code again. See http://codemirror.net/4/doc/manual.html#setSelection for a description of the new behavior.

[njx]

Thanks - that looks good and seems to work well overall. The one tweak I might suggest is to go back to a longer default historyEventDelay - on my MacBook Pro with default settings, if I hold down an arrow key, the delay before the first repeat is longer than 500ms; a delay of 1250ms seems to be enough (I suspect the actual delay is about 1000ms, but setting values too close to there seems to still cause merges). But if you don't want to bake that long a delay into the defaults, we can just set the option ourselves, of course.

[marijnh]

I've bumped the default value of that option. It was sort of overly short for normal edits as well.

[njx]

Thanks. Seems to be working well, and the fix for the redo issue seems to be fine as well. Closing.

[njx]

Oops, I did find one more issue after using it for awhile. If you (relatively quickly) delete a character, hit the down arrow, then delete another character, then undo, the two edits both get undone. That's not a new behavior - it reproduces in v3 too - but the longer history event delay makes it significantly easier to run into.

I wonder if an edit after a selection change should always force a new event - i.e., you should never merge an edit, a selection change, and another edit.

[njx]

FWIW, I tried a few other editors, and they all seem to follow that heuristic - explicit selection changes between edits cause a new history item to start.

[marijnh]

That was intentional, but I guess there's a good argument to be made for the other behavior, and indeed, most other editors behave like that. Attached patch should take care of it.

[njx]

Yup, that seems to fix it. I'll let you know if we run into any issues.

[njx]

I'm running into a problem with the last patch, though it's possible that it's really that my suggestion was bad :)

I have an edit which I would like to be merged when it happens repeatedly. The edit does a replaceRange() followed by a setSelection() to select the updated content. After the patch above, it no longer merges, even if I wrap an operation() around the edit/selection change pair.

Is this a consequence of my proposed heuristic above? I would assume that if a selection change is in the same operation as an edit, it shouldn't break merging, since it's really part of the same edit and not a user-initiated separate selection change.

[marijnh]

Right. Making small tweaks to accommodate single use cases always leads to madness. Though with some more thought, such use cases can also help reach a point where we actually model the problem we want to model.

The attached patches go back to a simpler model (getting rid of the concept of a pending selection), but adds selection origins (rather than a merge flag), which work similar to change origins. I've added a test for your latest use case.

If you could verify that your other tests also still pass, I'd like to mark a release candidate today or tomorrow.

[njx]

Yes, that approach seems more consistent and makes sense, and all the merging cases I've tested seem to work fine. However, I found that the latest patch seems to have reintroduced the "redo" issue in the original bug report. As mentioned there, I don't think it's critical to re-fix this if doing so would complicate things too much, but if it's just a matter of reinstating some logic that got lost in the last patch that would be good.

[marijnh]

Indeed, it that part was accidentally deleted in the change. See attached patch.

[njx]

Seems to be working. Thanks.

[njx]

Hi Marijn - a followup to the merging changes. After the latest changes, it seems like if I do an edit and a selection (programmatically) one after the other, they won't merge, even if they have the same origin. I think I was expecting that you could use origins to merge associated edits and selections together.

I can still get them to merge if I wrap them in an operation together, but it seems intuitive that specifying the same origin for an edit and a selection would allow them to merge together as well. Would that be easy to implement?

(That said, I could also see the argument that now that selections are their own separate history events, it makes sense to use operations to group them with their associated edits. But it might be worth noting the behavior change in the upgrade guide.)

[marijnh]

This worked for origins starting with an asterisk, but not for others. I guess it is sensible to make an exception here. See attached patch.

[njx]

Looks good, thanks.