Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

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

Merged
merged 2 commits into from
May 23, 2014
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d540f7
Fix for issue #7458: View doesn't scroll down after Edit > Move Line …
lkcampbell May 12, 2014
b04029c
Simplify solution to only scroll to primary selection
lkcampbell May 20, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 24 additions & 9 deletions src/editor/EditorCommandHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,14 +726,16 @@ define(function (require, exports, module) {
return;
}

var doc = editor.document,
lineSelections = editor.convertToLineSelections(editor.getSelections()),
isInlineWidget = !!EditorManager.getFocusedInlineWidget(),
firstLine = editor.getFirstVisibleLine(),
lastLine = editor.getLastVisibleLine(),
totalLines = editor.lineCount(),
lineLength = 0,
edits = [];
var doc = editor.document,
lineSelections = editor.convertToLineSelections(editor.getSelections()),
isInlineWidget = !!EditorManager.getFocusedInlineWidget(),
firstLine = editor.getFirstVisibleLine(),
lastLine = editor.getLastVisibleLine(),
totalLines = editor.lineCount(),
lineLength = 0,
edits = [],
newSels = [],
pos = {};

_.each(lineSelections, function (lineSel) {
var sel = lineSel.selectionForEdit,
Expand Down Expand Up @@ -799,11 +801,24 @@ define(function (require, exports, module) {
break;
}
});

// Make sure selections are correct and primary selection is scrolled into view
if (edits.length) {
var newSels = doc.doMultipleEdits(edits);
newSels = doc.doMultipleEdits(edits);

pos.ch = 0;

if (direction === DIRECTION_UP) {
editor.setSelections(newSels);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pos.line = editor.getSelection().start.line;
} else if (direction === DIRECTION_DOWN) {
pos.line = editor.getSelection().end.line;
} else {
console.error("EditorCommandHandler.moveLine() called with invalid argument 'direction' = %d", direction);
pos = null;
}

editor._codeMirror.scrollIntoView(pos);
}
}

Expand Down