View doesn't scroll down after Edit > Move Line Down #12612
Comments
|
|
|
|
|
Yup! Thanks. |
|
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. |
|
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. |
|
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. |
|
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 |
|
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. |
|
|
|
I didn't wrote the original code, but I did fixed several edge cases when working in inline editors. |
|
Reviewed, leaving in 1.0. |
|
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. |
|
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. |
|
|
|
Fix submitted. |
|
FBNC to |
|
|
|
|
|
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. |
|
|
|
Er, it looks like there would be multiple |
|
Looking briefly at the CM code, it looks like |
|
|
|
Sorry, I didn't read back and see we already merged the fix. I'm fine just closing this for now (if |
|
Actually, I'll open a low pri bug to myself on that question just to keep it around. |
Wednesday Apr 09, 2014 at 06:16 GMT
Originally opened as adobe/brackets#7458
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.The text was updated successfully, but these errors were encountered: