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.

[lkcampbell]

@njx, ok, well, if that is legacy code, and it worked in Sprint 37, that's probably not where I want to try and fix this problem.

[TomMalbran]

I didn't wrote the original code, but I did fixed several edge cases when working in inline editors.

[njx]

Reviewed, leaving in 1.0.

[lkcampbell]

@njx and @peterflynn, I have a simple, one-liner fix for this issue.

Before I submit it I want to make sure you are okay with its behavior.

If you do a multiple selection and move the selections up or down a line, the editor always scrolls to show the primary selection (i.e. the last selection in your multiple selection).

An alternative solution would be to ignore the primary selection in multiple selections. Instead, scroll the editor to the topmost selection when moving up a line and scroll it to the bottommost selection when moving down a line.

If you are okay with always scrolling to the primary selection I will submit my easy fix now. If not, I will continue to work on the alternative solution.

[njx]

The alternative solution sounds better to me, but if it's significant work it seems like it would be fine to get the simpler fix in now (given that it's broken anyway) and do the better fix later.

[lkcampbell]

@njx, that's fine, I think the alternative solution is better as well. It's not really that much more work. I just like one-line fixes :).

[lkcampbell]

Fix submitted.

[redmunds]

FBNC to @peterflynn , changed milestone from Brackets 1.0 to Release 40.

[lkcampbell]

@peterflynn, FYI, we did end up solving this with a solution that always scrolls to the primary selection because it was less complex to implement. This means the behavior with multiple selections may feel a bit odd, but the behavior with a single selection, which is more common, should be what is expected.

[peterflynn]

@lkcampbell @njx Out of curiosity, do we understand why CM didn't naturally scroll the cursor into view after the edit? Do CM edit operations not do that by default?

[lkcampbell]

@peterflynn, I would expect the edit operations -- in this case, the batched replaceRange operations -- to scroll correctly by default. After all, the move line code did work before multiple cursors were added to CodeMirror. It is probable that the real problem is either with the CodeMirror batched, multiple selection, replaceRange operations, or the way the Brackets code is using those operations.

When I first traced through all of that code, it felt a bit like a rabbit hole, so I decided the quick and simple solution was to fix the view after the scroll got misplaced. A better solution is to find out why the scroll is lost and try to fix it at the source, but that would take more time and effort.

[njx]

@peterflynn Hmmm, that's a good question. I haven't looked into it, but you're right that it seems like it should just work (at least in the non-multicursor case).

@lkcampbell The code on the Brackets side is a bit complex, but if there's only one selection the net result should just be a single replaceRange() call (possibly in an operation). It might be worth seeing if the non-scrolling behavior repros in CodeMirror in a test case that just does the equivalent replaceRange(). If so, we should file a CM bug.

[njx]

Er, it looks like there would be multiple replaceRange()s in this case, actually, batched together.

[njx]

Looking briefly at the CM code, it looks like replaceRange() ends up (after a chain of calls) calling makeChangeSingleDoc(), which calls setSelectionNoUndo() with sel_dontScroll, causing it to skip a call to ensureCursorVisible(). That kind of makes sense - replaceRange() is a general edit op and you don't necessarily always want to scroll to the selection. But it doesn't seem like the original (pre-CMv4) move line code in Brackets did anything other than replaceRange() (at least in the move line down case, it didn't then call setSelection()), so it's not obvious why that should have worked differently.

[lkcampbell]

@njx and @peterflynn, so, do you want me to continue to investigate before you close this bug or should I move onto something else?

[njx]

Sorry, I didn't read back and see we already merged the fix. I'm fine just closing this for now (if @peterflynn is) since it doesn't seem urgent to figure out, but if we see other cases where scrolling isn't doing what we expect we might want to dig into it further at that point.

[njx]

Actually, I'll open a low pri bug to myself on that question just to keep it around.