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

[peterflynn]

1. Press Ctrl+Shift+Down until the current line goes off the bottom of the viewport

Result: line moves off the bottom of the screen

Expected: viewport keeps scrolling down so line remains visible

It seems to work fine when moving upward, though.

It works correctly in Sprint 37, so I assume this is due to the multi-cursor changes (CC @njx). Doesn't seem major enough to block Sprint 38, though.

[pthiess]

@njx

[lkcampbell]

@njx, I would like to try and fix this issue. Can I take it over from you? Just reassign it to me if it is okay.

[njx]

Yup! Thanks.

[peterflynn]

Nominating for 1.0, though if it gets fixed along with #5226 it might be out of the picture much sooner. Adding 'needs review' to validate nomination.

[lkcampbell]

@njx, Okay, first question after doing a quick run through of the code.

Regarding the following code in EditorCommandHandlers.js currently on line 804:

            if (direction === DIRECTION_UP) {
                editor.setSelections(newSels);
            }

editor.setSelections() calls the code that eventually makes the editor scroll up and show the moved line when moving the line up. Do you know why we only call this function when we are moving a line upwards and not downwards?

FYI, I tried commenting out the conditional as a test and it still doesn't fix the problem, but I have an idea why the second failure is occurring. However, I need to understand the purpose of the conditional before I attempt to remove it.

[njx]

That logic came from the original implementation of Move Line Up, and I ported it into the multiselect version, but I didn't look at it closely to understand why. I think it's that in the move-down case, the selection should already be correct because we're just inserting stuff before the line containing the selection (and CM will automatically move the selection to fix it up), but in the move-up case, we actually have to delete the content that contains the selection and reinsert it elsewhere, so we need to explicitly re-set the selection within the new location of the moved content.

[njx]

Hmm, actually that's not really true - I think in the move up case we also just try to delete the line before and insert it afterward, so the selection should get fixed up automatically. There might be some edge-case-y reason why it doesn't get preserved properly though.

Did @TomMalbran write the Move Line Up/Down functionality originally?

[njx]

Ah, the edge case probably has to do with the fact that CM might accidentally expand the selection when we insert the previous text below the current line, if the original selection encompassed the whole line, because it assumes that an insertion exactly at the end of the selection should grow the selection to encompass the insertion, whereas an insertion at the beginning of the selection doesn't. The block on line 761 ("Make sure that CodeMirror hasn't expanded the selection...") adjusts the selection explicitly for this, so we have to re-set it afterward.