refactor change code, add TextRange and TextSize types #638

oberblastmeister · 2021-08-22T14:51:40Z

I have put the new Change in a separate file and haven't updated everything yet.

kirawi · 2021-08-22T15:42:29Z

I'm confused, was there prior discussion about this on Matrix? I don't quite understand the reason for this change 😅

cessen · 2021-08-22T15:57:37Z

I really appreciate your enthusiasm to contribute to Helix. However, it's not at all clear to me what the benefit of this refactor is, nor the new types you're adding. It feels like abstraction for abstraction's sake, and to my eye at least actually makes things less straight-forward/obvious.

Maybe I missed where this was discussed, but I'd really recommend opening an issue to discuss sweeping changes like this and get some consensus before actually coding them up. That way you don't spend a lot of effort that just gets rejected. (Again, maybe I just missed where that discussion happened, in which case nevermind.)

oberblastmeister · 2021-08-22T15:59:52Z

I did talk on matrix about something similar but that was just for me to understand things. There was no prior discussion matrix about this refactor, The reason for this change is that this greatly simplifies the code. A single Change can now be taken in isolation. There are no unnecessary conversions between changes and Operation and then back. Changing to Operation doesn't even help because it makes processing even harder as it cannot be taken in isolation which means processing them must repeat the same mutable code. This code also enforces stronger invariants. It makes sure that every Change is disjoint and that they are sorted. Not doing this can cause issues. In addition, this pr adds TextSize and TextRange which are more ergonomic types. They inforce more invariants than representing them as tuples and have multiple useful functions that I have seen repeated in code.

oberblastmeister · 2021-08-22T16:07:41Z

Thanks for appreciating my enthusiasm! I should definitely post an issue next time, sorry. I think I definitely am not overabstracting though. I removed the Operation abstraction, which I thought was unnecessary for the reasons above.TextSize and TextRange are nice abstractions, especially when we are in an editor that uses these sort of things all the time. The stuff in the one module I am not sure about yet. I think they are nice because they allow types to be 'correct by construction'.

oberblastmeister · 2021-08-22T17:28:16Z

@cessen Thanks for your suggestions. I have simplified the code a lot, it is now really nice. I think I did overabstract the Change struct, so now it is much better. Please give this another chance!

kirawi

It's still not overtly clear to me what benefits this has over the existing code, but I'm not really familiar with this area of the codebase either. (I would also suggest marking this PR as a draft since I noticed a lot of commented code; there should be an option on the right-hand sidebar of this page)

kirawi · 2021-08-22T17:37:51Z

helix-core/src/lib.rs

 pub use smallvec::SmallVec;
 pub use syntax::Syntax;

 pub use diagnostic::Diagnostic;
 pub use state::State;

 pub use line_ending::{LineEnding, DEFAULT_LINE_ENDING};
-pub use transaction::{Assoc, Change, ChangeSet, Operation, Transaction};
+pub use transaction::{Assoc, Change, ChangeSet, Operation, Transaction};


Missing newline here.

helix-core/src/text_size/offset.rs

kirawi · 2021-08-22T17:42:14Z

helix-core/src/text_size/range.rs

+///
+/// It is a logic error for `start` to be greater than `end`.
+#[derive(Default, Copy, Clone, Eq, PartialEq, Hash)]
+pub struct TextRange {


Isn't this basically just SelectionRange?

SelectionRange can be flipped

kirawi · 2021-08-22T17:43:28Z

helix-core/src/text_size/traits.rs

@@ -0,0 +1,25 @@
+use super::size::TextSize;


I'm also confused here, I feel like this is also already a covered case w/ RopeGraphemes (I might be wrong though).

helix-core/src/transaction.rs

kirawi · 2021-08-22T17:45:13Z

helix-core/src/text_size/traits.rs

+    fn text_len(&self) -> TextSize {
+        *self 
+    }
+}


Missing newline.

kirawi · 2021-08-22T17:45:20Z

helix-core/src/text_size/size.rs

+    fn sum<I: Iterator<Item = A>>(iter: I) -> TextSize {
+        iter.fold(0.into(), Add::add)
+    }
+}


Missing newline.

helix-core/src/transaction.rs

oberblastmeister · 2021-08-22T17:55:43Z

Could you explain what you still don't think has benefits over the existing code? I think I can keep TextRange and remove TextSize and TextOffset as those don't seem to be very useful. I think we can just use a type alias instead for TextSize.

kirawi · 2021-08-22T18:06:27Z

From the cursory look I gave, it seems to me that you can achieve the same benefit for less abstraction/work by improving upon the already-existing code in transaction.rs rather than pulling in all this external code. If you are able to flatten the Change processing through this PR, I think that would be helpful. Again, I'm not really familiar with this area of the codebase (I only worked with it for helix-core/src/diff.rs).

oberblastmeister · 2021-08-22T18:24:40Z

Like I explained above, the existing Change code is much larger with more abstractions and isn't as safe as this code. It has to convert from and to Operation which is a worse representation in my opinion. For example, look at the function invert, in the new code it is like 2 lines of code. The existing implementation takes 20 something lines. This is similar when comparing many functions. Additionaly, the new code has the option to ensure that everything is disjoint and sorts the changes. This guarantees that everything happens properly, it is even listed as a todo in the code by blaz. Thats why I created this pr, to refactor the code and make it simpler. As for the other types I added, I think we can remove TextSize and TextOffset because those don't make much sense. I like TextRange however because it makes stuff nicer to work with. It also ensures that you cannot subtract with overflow which is currently not prevented. However it is not absolutely necessary and I can remove it if people really want me to.

archseer · 2021-08-23T01:14:35Z

I agree with @cessen's sentiment: I appreciate the effort put into this, but a sweeping change that affects all open PRs needs a prior discussion (see #362 for an example). In particular, it's good to talk through why the code is currently the way it is, because there's often a reason behind it.

The reason for this change is that this greatly simplifies the code.

The diff is currently +1200 -500. It's also very noisy, most of the diff is renaming various existing structs, I'd prefer to keep those names as is.

There are no unnecessary conversions between changes and Operation and then back. Changing to Operation doesn't even help because it makes processing even harder as it cannot be taken in isolation which means processing them must repeat the same mutable code.

There are of course reasons why we have two types right now. The editing primitive is an Operation which models an operational transform and it's map and compose (map currently being unimplemented) allow for far greater flexibility in the future (async formatting after some user edits? map the formatting transaction over the edits and apply it. It's also the foundation for collaborative editing).

But Operations aren't very ergonomic, so the change/change_by_range functions expose an API that's easier to use. I did attempt to merge the two before, but it either made the OT operations more convoluted, or it made the API less easy to use. I would be open to such a change if we can come up with a proposal that's acceptable.

Additionaly, the new code has the option to ensure that everything is disjoint and sorts the changes. This guarantees that everything happens properly, it is even listed as a todo in the code by blaz.

Since the changeset builder is more low-level and we control all the methods I was thinking there's no need to do a check for ordering and this should actually be a debug_assert. Kakoune has built-in assertions in debug builds that verify a bunch of invariants before and after every command.

oberblastmeister added 8 commits August 21, 2021 16:52

add textrange

f1e9214

change name

6aa8a91

I like monoids

71704b9

ok

916274c

add offset

a4631be

added apply

b6f71bf

some tests

9e70716

evil hack

6551396

oberblastmeister added 2 commits August 22, 2021 12:20

remove stuff

db01b56

simplify

5dc054a

oberblastmeister changed the title ~~refactor change code, add TextRange and TextSize primitives~~ refactor change code, add TextRange and TextSize types Aug 22, 2021

oberblastmeister added 2 commits August 22, 2021 13:15

more stuff

91541ca

small change

8bf65ba

kirawi reviewed Aug 22, 2021

View reviewed changes

remove comment

ca8becd

oberblastmeister marked this pull request as draft August 22, 2021 17:58

remove offset and size

e2993fe

oberblastmeister closed this Aug 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor change code, add TextRange and TextSize types #638

refactor change code, add TextRange and TextSize types #638

oberblastmeister commented Aug 22, 2021

kirawi commented Aug 22, 2021 •

edited

Loading

cessen commented Aug 22, 2021

oberblastmeister commented Aug 22, 2021

oberblastmeister commented Aug 22, 2021

oberblastmeister commented Aug 22, 2021

kirawi left a comment •

edited

Loading

kirawi Aug 22, 2021

kirawi Aug 22, 2021

oberblastmeister Aug 22, 2021

kirawi Aug 22, 2021

kirawi Aug 22, 2021

kirawi Aug 22, 2021

oberblastmeister commented Aug 22, 2021

kirawi commented Aug 22, 2021 •

edited

Loading

oberblastmeister commented Aug 22, 2021

archseer commented Aug 23, 2021

refactor change code, add TextRange and TextSize types #638

refactor change code, add TextRange and TextSize types #638

Conversation

oberblastmeister commented Aug 22, 2021

kirawi commented Aug 22, 2021 • edited Loading

cessen commented Aug 22, 2021

oberblastmeister commented Aug 22, 2021

oberblastmeister commented Aug 22, 2021

oberblastmeister commented Aug 22, 2021

kirawi left a comment • edited Loading

Choose a reason for hiding this comment

kirawi Aug 22, 2021

Choose a reason for hiding this comment

kirawi Aug 22, 2021

Choose a reason for hiding this comment

oberblastmeister Aug 22, 2021

Choose a reason for hiding this comment

kirawi Aug 22, 2021

Choose a reason for hiding this comment

kirawi Aug 22, 2021

Choose a reason for hiding this comment

kirawi Aug 22, 2021

Choose a reason for hiding this comment

oberblastmeister commented Aug 22, 2021

kirawi commented Aug 22, 2021 • edited Loading

oberblastmeister commented Aug 22, 2021

archseer commented Aug 23, 2021

kirawi commented Aug 22, 2021 •

edited

Loading

kirawi left a comment •

edited

Loading

kirawi commented Aug 22, 2021 •

edited

Loading