Add support for moving lines #849

nangtrongvuon · 2018-09-30T08:08:12Z

This uses xi-mac #278.

This adds support for moving lines. However, it achieves this with some less than desirable workarounds, namely where it handles move line up.

EDIT: This now uses the new InsertDrift type recently introduced. There is still no word wrap support but this solution should look cleaner than the previous attempt.

An attempt at solving #793.

ryanbloom

Thanks for implementing this feature! I've been missing it for a while.

Testing this out, I ran into a few edge cases:

It doesn't work when lines are being wrapped (either column wrap or word wrap).
If you select an entire line (such that the cursor is at the beginning of the next one) and press move up, the selection expands to include one more line than it should.
If you go to the last line in the file and move it up, Syntect crashes. This one might have to do with existing code rather than the new changes.

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `30`,
 right: `0`', plugin-lib/src/state_cache.rs:255:13

ryanbloom · 2018-09-30T20:48:56Z

rust/core-lib/src/editor.rs

+            // n..n.
+            let start = range.start;
+            let end = range.end - 1;
+            eprintln!("{:?} {:?} ", start, end);


Looks like this print statement might have been left in from debugging.

ryanbloom · 2018-09-30T20:50:48Z

rust/core-lib/src/editor.rs

@@ -254,7 +254,7 @@ impl Editor {
    /// a 3-tuple containing the delta representing the changes, the previous
    /// buffer, and a bool indicating whether selections should be preserved.
    pub(crate) fn commit_delta(&mut self)
-        -> Option<(Delta<RopeInfo>, Rope, bool)> {
+        -> Option<(Delta<RopeInfo>, Rope, bool, bool)> {


Doc comment needs updating now that it's a 4-tuple.

ryanbloom · 2018-09-30T21:21:34Z

rust/core-lib/src/view.rs

@@ -1089,6 +1089,18 @@ impl View {
        self.do_set_replace(replacement, false);
    }

+    // Get the offsets for a line. Includes newline characters.
+    pub fn get_line_offset(&self, text: &Rope, line: usize) -> (usize, usize) {


This method name sounds like it returns a single offset, measured in lines (comparing it to get_line_range). I'm not well-versed in Rust naming conventions, but something like get_offsets_for_line would make more sense to me.

Also, the comment could be a doc comment.

cmyr · 2018-10-03T01:23:18Z

@ryanbloom The syntect crash you're seeing is fixed in #840.

cmyr

Interesting! This is definitely a hack, but I see what you're trying to do.

There are two bugs I can find, which are probably not your fault:

if the last line is blank, move_line_up from this lines produces an incorrect cursor position
with the entire next-to-last line selected, move_line_down + undo produces incorrect selection state.

So... I'm not sure what to think. 😕

The second issue could probably be addressed by having the view stash selections with each undo group, so that undo restores them; but this might be hiding a larger problem.

There's clearly a bug here, so I'll open an issue for that separately.

In any case, I understand why you've added an EditType but I'd like to avoid that as a long-term solution; even the Transpose probably shouldn't be there; EditType is a bit of a weird concept, but to my mind it has two main roles: letting us determine undo groupings, and communicating coarse information about an edit to plugins. It was really not intended as a way of sneaking state up to commit_delta, and the fact that we're having to do this makes me wonder if there isn't a better way, such as including this information in the delta itself.

But these things are mostly beyond the scope of this PR.

My sense is that a proper implementation of this feature is blocked on implementing priority. That will involve digging pretty deep into the inner workings of the crdt system, where frankly I've not really spent much time, and so would be of limited help. In any case, that work is still waiting for a fuller description of the problem.

So: I don't think we can come up with a fully satisfying implementation of this given the current state of the system. My suggestion is that we put this on hold and try to figure out what is involved in doing the priority stuff.

In any case, thanks for the work! The code is generally good, the feature is nice, and the fact that it doesn't work is in no way your fault. 🙇

cmyr · 2018-10-03T20:01:10Z

rust/core-lib/src/editor.rs

 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
+// distributed under the License is distributed on an "AS IS" BASIS,


I see someone has been testing move_line_down... 🤔

cmyr · 2018-10-29T21:15:20Z

@nangtrongvuon I was wondering if this might've been fixed by #941, and I played around a bit and it seemed better?

edit: okay, I spoke too soon, I'm still seeing these bugs. I'm going to investigate a bit more, though.

This implementation is meant to give some leeway for a future improvement, in which these functions can be extended to move lines further than 1 line at a time.

This commit also adds an InsertDrift exception, where the edit tries to stay outside the current selection even when the selection is a caret. This type is only used for MoveLineUp.

nangtrongvuon · 2018-11-20T07:42:08Z

@cmyr The PR has been updated to use the new InsertDrift type. Perhaps it's worth another brief look? Word wrap support is still not there, but I think in word wrap's current state maybe support for it can be deferred..?

nangtrongvuon mentioned this pull request Sep 30, 2018

Add frontend handling for move line xi-editor/xi-mac#278

Open

nangtrongvuon force-pushed the line-move branch from 6ff7bf5 to 86e77bc Compare September 30, 2018 08:12

ryanbloom reviewed Oct 1, 2018

View reviewed changes

nangtrongvuon force-pushed the line-move branch 3 times, most recently from 601bd20 to f0750a8 Compare October 2, 2018 02:24

cmyr reviewed Oct 3, 2018

View reviewed changes

cmyr added the on hold label Oct 9, 2018

nangtrongvuon mentioned this pull request Oct 18, 2018

Implement edit priority #933

Open

nangtrongvuon added 3 commits November 19, 2018 10:33

Add initial impl of line move

c370245

Preserve cursor state on move line

6d30d2a

Refined line move behavior

b0e1c5d

This implementation is meant to give some leeway for a future improvement, in which these functions can be extended to move lines further than 1 line at a time.

nangtrongvuon force-pushed the line-move branch from f0750a8 to e1dd90b Compare November 19, 2018 03:35

Rebase branch onto latest master

762bb82

nangtrongvuon force-pushed the line-move branch from e1dd90b to 762bb82 Compare November 19, 2018 04:24

nangtrongvuon added 3 commits November 19, 2018 12:05

Use InsertDrift for move line methods

80b3d09

This commit also adds an InsertDrift exception, where the edit tries to stay outside the current selection even when the selection is a caret. This type is only used for MoveLineUp.

Added simple tests for move lines

98f6ad0

Disable move line up when last line is empty

f9068b6

Fix failing multi line move tests

abe6d52

cmyr mentioned this pull request Nov 21, 2018

Restore selections on undo/redo #1006

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for moving lines #849

Add support for moving lines #849

nangtrongvuon commented Sep 30, 2018 •

edited

Loading

ryanbloom left a comment

ryanbloom Sep 30, 2018

ryanbloom Sep 30, 2018

ryanbloom Sep 30, 2018

cmyr commented Oct 3, 2018 •

edited

Loading

cmyr left a comment

cmyr Oct 3, 2018

cmyr commented Oct 29, 2018 •

edited

Loading

nangtrongvuon commented Nov 20, 2018

Add support for moving lines #849

Are you sure you want to change the base?

Add support for moving lines #849

Conversation

nangtrongvuon commented Sep 30, 2018 • edited Loading

ryanbloom left a comment

Choose a reason for hiding this comment

ryanbloom Sep 30, 2018

Choose a reason for hiding this comment

ryanbloom Sep 30, 2018

Choose a reason for hiding this comment

ryanbloom Sep 30, 2018

Choose a reason for hiding this comment

cmyr commented Oct 3, 2018 • edited Loading

cmyr left a comment

Choose a reason for hiding this comment

cmyr Oct 3, 2018

Choose a reason for hiding this comment

cmyr commented Oct 29, 2018 • edited Loading

nangtrongvuon commented Nov 20, 2018

nangtrongvuon commented Sep 30, 2018 •

edited

Loading

cmyr commented Oct 3, 2018 •

edited

Loading

cmyr commented Oct 29, 2018 •

edited

Loading