View doesn't scroll down after Edit > Move Line Down

[lkcampbell]

Fix issue #7458 and issue #5226: Select Line + Move Line Up = Line disappears at top of editor

[redmunds]

@lkcampbell This works good for a single selection except I am seeing 1 quirk:

Recipe

1. Turn off word wrap
2. Open a file that is wider than viewport (so it has a horizontal scrollbar)
3. Select a line that is wider than viewport
4. Hold down Ctrl-Shift-Down until the line hits the EOF

Results
Editor properly scrolls down so line is always on screen, but when line hits the EOF the editor scrolls to the right. I think this is due to the fact the the trailing newline is dropped from the line being moved so it goes to the last line, but the end-of-selection is horizontally scrolled into view.

Expected
Editor does not scroll horizontally.

This might not be easy to fix, and is not critical, but would be nice.

[redmunds]

Using the first() and last() selection (according to position in doc) here is not correct behavior for multiple selections. This is a cool effect when all selections are in the viewport, but that may not be true.

Try this out with multiple selections that are too far apart to both fit in viewport -- the page jumps when you reverse line move direction, so it's not desirable.

The safe fallback behavior for multiple selections is to operate on the "current" (i.e. last added) selection, which you can get with editor.getSelection().

You are welcome to try to implement "use first selection in viewport when move line up" and "use last line in viewport when line move down" behavior, but it will take a bit of work to determine which selection to use. Another option may be to only do this if all selections are in viewport.

[lkcampbell]

@redmunds, my original one line fix for this problem was to scroll "current selection" into view, as you suggested. I posed the design question here: #7458 (comment).

Pulling in @njx for a design opinion. I can do whatever you guys want, just need to know the expected behavior for multiple selects contained in view and multiple selects beyond the view.

[redmunds]

A little more info: when there are multiple selections, they are ordered by the order that they appear in the document, so, if you have the numbers of the top & bottom lines visible in the viewport (which I think you can get with cm.visibleLines()), it's easy to determine "first selection in viewport" and "last selection in viewport".

Note that the "primary" selection can be in any position.

[lkcampbell]

@redmunds, okay. So, for multiple selections, if first and last selection are in viewport, scroll to first on move up and last on move down, else, scroll to primary selection, correct?

cc: @njx

[njx]

Ah, I hadn't thought about when selections are outside the viewport.

I think @redmunds suggestion was: find the first and last selection that are in the viewport. Then, on move line up, if the first selection in the viewport crosses the top viewport boundary, then scroll up, otherwise do nothing. On move line down, if the last selection in the viewport crosses the bottom viewport boundary, then scroll down, otherwise do nothing.

[lkcampbell]

Ah, I understand now. Thanks for the clarification. Yes, that sounds like good behavior to me. I will start working on it.

[lkcampbell]

@njx, FYI, cm.getViewport() has too large of a range. It gives the lines that are rendered, which is bigger than what is displayed. I checked this for a file that displays lines 118-169 and it returns 95-176. I'm looking for something more narrow.

[njx]

Hmmm, you're right. One option might be to call getScrollInfo() to get the pixel positions of the top and bottom of the visible scroll area (you probably need to use info.top and info.top + info.clientHeight), then map those to lines using coordsChar().

[njx]

(The tricky part is knowing what to pass to coordsChar() as the coordinate system...the default is "page", which I think means relative to the viewport (not the virtual scroll area), so you might actually just be able to pass 0 and info.clientHeight for the line numbers. I always have to double-check though :))

[TomMalbran]

BTW in View Command Handlers I made a function to get the visible first and last line but it is a bit complex, and maybe there is an easier way to do it now that CM has new APIs. Also the function _getLinesInView isn't great for exporting since the parameters aren't great. I used it to calculate the visible lines several time in the past to deal with inline editors, but now that it is called once, it should just receive an editor and do some of the calculations already done in _scrollLine.

Saying that probably using codemirror.getScrollInfo() and codemirror.lineAtHeight() can make this a lot easier.

[redmunds]

@lkcampbell Done with initial review.

[lkcampbell]

@redmunds, yes it looks like the case where the user selects the full line is going to be a special case. That trailing newline is breaking in my current code. I will focus first on getting the regular selections working and then I will address the Select Line case, which should address your comment above as well.

[lkcampbell]

@redmunds and @njx, I was able to get the first and last viewport line numbers working today, but getting all of the multi-selection scenarios to behave correctly is trickier than I thought it would be. Every time I fix it in one case, other cases break.

That one-line fix I originally proposed is starting to look better and better. It just makes sure that the primary selection is always scrolled into view, which does fix both bugs and provides good behavior for the single line selection, which is the most common scenario for move line. I suggest I roll back what I have here and submit the simple fix instead. Try it out, see how it feels. Then, if we want to make it more responsive to multiple selections, as we have discussed above, we file another issue.

[redmunds]

@lkcampbell

I suggest I roll back what I have here and submit the simple fix instead. Try it out, see how it feels.

Sounds good. That's the agile way!

[lkcampbell]

@redmunds, here's the "one-line" version that turned out not to be one-line after addressing your code review suggestions :). This fix focuses on always scrolling the primary selection correctly and doesn't consider any other selections in multiple selection. It fixes both bugs and addresses the special case recipe you provided above.

[lkcampbell]

@redmunds, also, I noticed that, with your special case recipe, the move down view scroll is off by one line. Not a huge deal but that trailing newline makes the code think that two lines are selected instead of one, which is partially correct, but unexpected, behavior.

At any rate, Code Review changes committed.

[redmunds]

@lkcampbell

Not a huge deal but that trailing newline makes the code think that two lines are selected instead of one, which is partially correct, but unexpected, behavior.

Yes, there are a few places in the code where this requires special handling.

[redmunds]

Note: While testing this, I discovered a weird case with undo that dos not seem to be related to this change, so I opened a separate issue: #7896

[redmunds]

Why is editor.setSelections(newSels) called in the up case but not the down case?

I tested with this code and also with that line commented out and couldn't find any unexpected behavior in either case, so is this necessary?

[lkcampbell]

@redmunds, this is discussed in the issue starting with #7458 (comment). Although I can't say I fully understand the explanation. I just chose to avoid it since it was already in there and I wanted to be cautious.

@njx, can you think of a recipe to exploit the special case so we can see exactly why this piece of code is necessary?

[njx]

I think my explanation in #7458 (comment) is right. Maybe you could try commenting out that call, then running the unit tests and seeing which case breaks (if any). That would give you a clue.

[redmunds]

With that line commented out, the problem case is when you use Move Line up with full lines selected -- it starts doing some really weird line duplications. There are 3 unit tests that fail.

[redmunds]

Done with code review.

[redmunds]

Looks good. Merging.