Add an edit_ops.rs file #1263

TDecking · 2020-04-27T11:31:10Z

Summary

This PR does the following:

It moves central editing operation away from editor.rs, closing Move editing functions out of editor.rs #1199.
Said operations no longer require a View to operate, but take a &[SelRegion] instead.
- This will help with Move selection modification out of view.rs #1198, as someone working with raw selections no
  longer has to construct a View to use them for these functions.
- The operations used the View not just to get selections, but also for functions
  like offset_of_line to get the lines. In case of View these are visual lines, not logical ones.
  Activating word wrap thus changed their behaviour, with special mention to
  duplicate_line, which was extremly broken. By making them think in logical lines,
  many previously undiscovered bugs have been fixed.
It also applies the changes demanded by clippy 0.0.212.

I have responded to reviews and made changes where appropriate.
I have tested the code with cargo test --all / ./rust/run_all_checks.
I have updated comments / documentation related to the changes I made.
I have rebased my PR branch onto xi-editor/master.

cmyr · 2020-04-28T22:15:18Z

sorry I didn't notice this and fixed the various lints separately, want to rebase and I'll take a look?

in editor.rs in terms of edit_ops.rs. Moved a missed function.

TDecking · 2020-04-29T12:47:15Z

Rebased.

cmyr

Okay I have a few preliminary comments, let me know what you think and I'll take another look.

cmyr · 2020-04-29T14:00:52Z

rust/core-lib/src/line_offset.rs

 /// A trait from which lines and columns in a document can be calculated
 /// into offsets inside a rope an vice versa.
 pub trait LineOffset {
    // use own breaks if present, or text if not (no line wrapping)

-    /// Returns the byte offset corresponding to the given visual line.
+    /// Returns the byte offset corresponding to the given line.


visual is an important distinction, here; it indicates a distinction from 'logical' lines, which are just lines that end in a newline character. a 'visual' line may not end in a line break, and may be the result of word wrap.

I guess this whole comment is slightly out of date, having been moved from view.rs?

LineOffset deals with both visual lines and logical lines, so this comment is indeed out of date.

I don't think that one trait coming with two behaviours is ideal in the long run,
but perhaps there is a way out of this.

Right, so this was an intentional choice, to the best of my recollection; the idea is that in the absence of line breaks, the hard breaks are the visual lines. This means that all edit ops can work with just a buffer, but things 'just work' when you add a view?

It may be that this isn't helpful, but that was the original thinking.

rust/core-lib/src/tabs.rs

cmyr · 2020-04-29T14:02:14Z

rust/rope/src/delta.rs

@@ -16,6 +16,8 @@
 //! It's useful to explicitly represent these operations so they can be
 //! shared across multiple subsystems.

+#![allow(clippy::many_single_char_names)]


I would prefer this to be on a specific function of that is possible?

rust/core-lib/src/line_offset.rs

cmyr · 2020-04-29T14:06:07Z

rust/core-lib/src/backspace.rs

    if !region.is_caret() {
        region.min()
    } else {
        // backspace deletes max(1, tab_size) contiguous spaces
-        let (_, c) = view.offset_to_line_col(&text, region.start);
+        let (_, c) = DefaultLineOffset.offset_to_line_col(&text, region.start);


this seems to be changing the behaviour? by not using the view we may get different results here when word wrap is enabled. I'm not sure if this is a problem in practice, but it feels fishy.

view.line_of_offset calls lines.visual_line_of_offset, so different behaviour on word wrap was the default and the change a bugfix (see PR summary).
I've renamed DefaultLineOffset to LogicalLines, which should make the change more obvious (and DefaultLineOffset more descriptive).

cmyr · 2020-04-29T14:12:29Z

rust/core-lib/src/editor.rs

-                    builder.replace(interval, Rope::from(capitalized_text));
+            Transpose => {
+                let delta = edit_ops::transpose(&self.text, regions);
+                if !delta.is_identity() {


we need to set EditType::Transpose here. It is probably worth doing another close read and make sure the edit type is correct in other places as well?

Fixed. There was indeed another case, but I belive that got them all.

(Are we testing undo and redo behaviour?)

There are a few tests around recording that exercise undo/redo, but overall not really. 😒

cmyr · 2020-04-29T14:15:33Z

rust/core-lib/src/editor.rs

-            if start > 0 && self.text.byte_at(start - 1) == (b'-') {
-                start -= 1;
+        match cmd {
+            Delete { movement, kill } => {


reading through this patch, I think my personal inclination would be to keep using the separate do_$thing methods, and then just in those methods call out to edit_ops; a major advantage of this approach is that it makes this diff easier to read and verify.

I've reversed this.
I didn't want multiple related functions with the same name doing different things when I made this. For now, the functions in editor.rs that call edit_ops.rs functions have the do_-prefix, while the functions in edit_ops.rs do not have that prefix.

great yea this was the intended pattern originally; do_ meant that "this is the entry point for some command".

codecov-io · 2020-04-30T11:49:19Z

Codecov Report

Merging #1263 into master will increase coverage by 0.29%.
The diff coverage is 78.53%.

@@            Coverage Diff             @@
##           master    #1263      +/-   ##
==========================================
+ Coverage   71.09%   71.39%   +0.29%     
==========================================
  Files          81       83       +2     
  Lines       14961    14812     -149     
==========================================
- Hits        10637    10575      -62     
+ Misses       4324     4237      -87

Impacted Files	Coverage Δ
rust/core-lib/src/annotations.rs	`70.43% <ø> (-1.47%)`	⬇️
rust/core-lib/src/core.rs	`42.62% <ø> (-1.64%)`	⬇️
rust/core-lib/src/event_context.rs	`83.39% <ø> (+0.13%)`	⬆️
rust/core-lib/src/find.rs	`82.21% <0.00%> (+2.54%)`	⬆️
rust/core-lib/src/plugins/catalog.rs	`25.39% <0.00%> (-0.41%)`	⬇️
rust/core-lib/src/plugins/mod.rs	`2.73% <ø> (ø)`
rust/core-lib/src/rpc.rs	`73.80% <ø> (-3.47%)`	⬇️
rust/core-lib/src/width_cache.rs	`84.09% <ø> (-2.28%)`	⬇️
rust/core-lib/tests/rpc.rs	`98.93% <ø> (-0.11%)`	⬇️
rust/experimental/lang/src/language/plaintext.rs	`0.00% <0.00%> (ø)`
... and 98 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65911d9...d3395c8. Read the comment docs.

cmyr

Great, this looks good to me, and is a helpful bit of cleanup. I'm currently working on text stuff in druid, and may at some point soon want to figure out a way to use some core xi stuff (edit ops, selections, movement) there. I'm not sure if this will mean moving that stuff to its own crate, or adding a default feature without which we only include some minimum set of things.

I'll leave this up for a minute in case you have further tweaks to make, and you can ping me when you'd like it merged.

TDecking · 2020-04-30T15:07:00Z

I don't have any further tweaks to make. Please go ahead and merge.

cmyr · 2020-04-30T22:10:35Z

Thanks!

TDecking added 13 commits April 29, 2020 01:05

edit_ops beginning

f157784

recreate core edit operation in edit_ops.rs

4e50484

rewrite of operations

45405bb

in editor.rs in terms of edit_ops.rs. Moved a missed function.

fix lint warnings

89ddfde

fix lint erors

79902b6

everything needed to satidfy lint

c0cd051

make tests pass

a1e2b66

lint

55fc256

smooth out the api a bit

52481b9

remove some remnants.

0643318

more remnants

8571ff8

format

5ed8991

clippy

5d4da02

TDecking force-pushed the edit_ops branch from e6ff766 to 5d4da02 Compare April 29, 2020 11:23

TDecking added 2 commits April 29, 2020 14:17

rebase fixes

c5a53f4

Merge remote-tracking branch 'upstream/master' into edit_ops

c282ae1

cmyr reviewed Apr 29, 2020

View reviewed changes

TDecking added 4 commits April 29, 2020 18:23

review changes part 1

a110159

review changes part 2

c6f69b4

rustfmt + clippy

3ae2532

Merge remote-tracking branch 'upstream/master' into edit_ops

d3395c8

cmyr approved these changes Apr 30, 2020

View reviewed changes

cmyr merged commit add9e3e into xi-editor:master Apr 30, 2020

This was referenced Jun 21, 2020

clippy nits #1279

Merged

Fix panics in do_insert_tab #1281

Merged

TDecking mentioned this pull request Jun 26, 2020

Fixing regressions in do_insert_tab #1285

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an edit_ops.rs file #1263

Add an edit_ops.rs file #1263

TDecking commented Apr 27, 2020 •

edited

cmyr commented Apr 28, 2020

TDecking commented Apr 29, 2020

cmyr left a comment

cmyr Apr 29, 2020

TDecking Apr 29, 2020

cmyr Apr 30, 2020

cmyr Apr 29, 2020

TDecking Apr 29, 2020

cmyr Apr 29, 2020

TDecking Apr 29, 2020

cmyr Apr 29, 2020

TDecking Apr 29, 2020

cmyr Apr 30, 2020

cmyr Apr 29, 2020

TDecking Apr 29, 2020

cmyr Apr 30, 2020

codecov-io commented Apr 30, 2020

cmyr left a comment

TDecking commented Apr 30, 2020

cmyr commented Apr 30, 2020

Add an edit_ops.rs file #1263

Add an edit_ops.rs file #1263

Conversation

TDecking commented Apr 27, 2020 • edited

Summary

cmyr commented Apr 28, 2020

TDecking commented Apr 29, 2020

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 30, 2020

Codecov Report

cmyr left a comment

Choose a reason for hiding this comment

TDecking commented Apr 30, 2020

cmyr commented Apr 30, 2020

TDecking commented Apr 27, 2020 •

edited