add support of grapheme clusters by jmuk · Pull Request #520 · xi-editor/xi-editor

jmuk · 2018-02-13T21:37:25Z

now next/prev_grapheme_offset works correctly rather than
falling back to codepoints.

fixes #298

now next/prev_grapheme_offset works correctly rather than falling back to codepoints. fixes xi-editor#298

cmyr

Cool, thanks for this! A few little nits but otherwise looks good.

cmyr · 2018-02-14T01:38:25Z

rust/rope/src/rope.rs

+    }
+
+    fn is_boundary(s: &String, offset: usize) -> bool {
+        if let Ok(r) = GraphemeCursor::new(offset, s.len(), true).is_boundary(s, 0) {


you can call .ok() on any Result<T> to convert it to an Option<T>, discarding the error.

cmyr · 2018-02-14T01:39:06Z

rust/rope/src/rope.rs

+        cursor.is_boundary::<GraphemeMetric>()
+    }
+
    // graphemes should probably be developed as a cursor-based interface


We can get rid of this comment now. :)

cmyr · 2018-02-14T01:47:02Z

rust/rope/src/rope.rs

 }

+#[derive(Clone, Copy)]
+pub struct GraphemeMetric(());


You can declare a unit struct without any braces: pub struct GraphemeMetric;.

raphlinus · 2018-02-14T03:13:10Z

I'm thrilled to see progress on this, and hate to throw cold water on the idea, but unfortunately trying to treat grapheme clusters as a metric doesn't quite work. The underlying reason is that grapheme cluster counting is fundamentally not a monoid homomorphism, as awesome as it would be if that were the case. Here's how to write a test case that will fail:

Create a string of RIS code points, let's say \u{1F1E6} that doesn't fit in a single leaf node (ie > 1kb). Now test whether the offset at len(first leaf) + 4 bytes is a boundary. The GraphemeMetric will always say no, but that would be wrong if the number of code points in the first leaf is odd. If the number of code points in the first leaf is even, edit the rope (usually taking the substring from offset 4 will work, unless the first leaf is small), so that one or the other would fail.

Because the size of graphemes is not bounded, and because boundaries can in the worst case depend on unbounded amounts of context, the boundary detection must be able to handle bringing in an arbitrary number of leaves as pre-context.

Bottom line, this needs to be implemented directly in {prev,next}_grapheme_offset with some attention to leaf boundaries. It is, sadly, not trivial to test - this is one reason I'd like to see some infrastructure for property testing developed.

Thanks for the attempt, and I'm happy to provide guidance to see it through.

mqzry · 2018-03-13T06:52:27Z

I am taking a stab at this. Did I understand it correctly that what is needed is:

Instantiate a GraphemeCursor
create a chunk iterators that walk in reverse and normal order to handle GraphemeIncomplete::PreContext or GraphemeIncomplete::NextChunk results
Call the right function on the grapheme cursor for example is_boundary
Handle any Err(GraphemeIncomplete) return value properly

jmuk · 2018-03-13T17:41:05Z

Yup, actually I was working on that. Some prototype is working on my local machine although it needs some refinements (sorry for my slow progress).

However, I've found a few issues on unicode-segmentation side, which cause panic. I'll send a PR to them first. Stay tuned.

jmuk · 2018-03-13T17:42:01Z

unicode-rs/unicode-segmentation#37 here is the PR.

Currently this points to my local github repository since some fixes are necessary.

jmuk · 2018-03-17T03:04:10Z

So I pushed my version of the code. Please take a look, if that's okay. I'm fine to hold this until the problem I've seen in unicode-segmentation are fixed.

jmuk · 2018-03-20T00:18:27Z

This PR actually relies on unicode-rs/unicode-segmentation#40 too. I can create workarounds if that turned out unwanted.

mqzry · 2018-04-29T14:51:05Z

What is the status for this PR? All relevant PRs seem to be merged.

raphlinus · 2018-04-29T15:33:50Z

Good question. I think unicode-segmentation needs to be published to crates.io. @Manishearth?

Manishearth · 2018-04-29T22:03:51Z

done!

…

-Manish Goregaokar

On Sun, Apr 29, 2018 at 8:33 AM, Raph Levien ***@***.***> wrote: Good question. I think unicode-segmentation needs to be published to crates.io. @Manishearth <https://github.com/Manishearth>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#520 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABivSD2dQ5KPhx_VeUrRGBpLK2WnKMY5ks5ttd1mgaJpZM4SEcDT> .

raphlinus · 2018-04-29T22:40:23Z

Thanks @Manishearth! Would you like to resolve the conflicts, @jmuk? If not, we can take care of it.

Incidentally, this will help a lot in dealing with CRLF line endings, right now the cursor can get between the two which causes all kinds of unexpected behavior.

jmuk · 2018-04-30T18:38:28Z

@raphlinus done!

raphlinus

Looks great, thanks, and thanks for your patience.

raphlinus · 2018-03-19T18:42:33Z

rust/rope/src/rope.rs

@@ -363,13 +366,13 @@ impl Rope {

    // graphemes should probably be developed as a cursor-based interface


This comment can be removed, as you've actually done it :)

raphlinus · 2018-03-19T18:50:31Z

rust/rope/src/rope.rs

+        }
+        // GraphemeIncomplete::NextChunk error will not happen, since this cursor thinks the end of
+        // the current leaf as the end of the string. Can a cursor know the total length of
+        // the tree?


To answer this question, this should be available at cursor.root.len(). In my first reading, I'm not sure whether this would allow you to simplify the following logic.

Okay -- yet cursor.root isn't visible here in rope.rs. I added total_len() to tree.rs, and it helps to simplify the code; at least I think so. PTAL.

raphlinus · 2018-03-19T18:55:56Z

rust/Cargo.lock

+[[package]]
+name = "unicode-segmentation"
+version = "1.2.0"
+source = "git+https://github.com/jmuk/unicode-segmentation.git?branch=fix_for_xi#1d7d062b38e6e5299ed0e5750f6890230c6cd0fa"


I think I'd like to wait for unicode-segmentation to release the fixed version, as (among other things), this would prevent releasing xi-rope as a crate. I know it's been a long time since we've done that, but we're discussing doing a release before long.

raphlinus · 2018-03-19T18:59:45Z

rust/rope/src/rope.rs

+        let mut c = GraphemeCursor::new(pos, l.len() + leaf_offset, true);
+        let mut next_boundary = c.next_boundary(&l, leaf_offset);
+        while let Err(GraphemeIncomplete::PreContext(_)) = next_boundary {
+            let (pl, poffset) = self.prev_leaf()?;


We should update the minimum rust version in README, as ? for Option is not supported in 1.20. A quick test shows that 1.22 should still work.

It seems the README had been updated already to require Rust 1.22; as 3c53388

raphlinus · 2018-03-19T19:01:06Z

rust/rope/src/rope.rs

+        assert_eq!(None, a.next_grapheme_offset(17));
+    }
+
+    #[test]


Excellent, thanks!

cmyr

Played around with this, feels good.

raphlinus · 2018-05-01T19:00:29Z

Many thanks!

jmuk · 2018-05-01T19:56:05Z

Yay!

add support of grapheme clusters

6ed38b3

now next/prev_grapheme_offset works correctly rather than falling back to codepoints. fixes xi-editor#298

googlebot added the cla: yes label Feb 13, 2018

cmyr reviewed Feb 14, 2018

View reviewed changes

jmuk added 9 commits March 13, 2018 17:18

refactoring of next_grapheme

d7a5006

Merge branch 'master' into grapheme

4329d05

refine implementation of next/prev grapheme

1fb60f3

fix on editor.rs

5f07122

reform next_boundary

829132d

cleanup the code

4454260

update Cargo.toml

89f1fd2

Currently this points to my local github repository since some fixes are necessary.

allow next/prev grapheme offset from non codepoint boundaries

5e960db

fix spacing

76364bb

jmuk added 3 commits March 27, 2018 15:30

point to master of unicode-segmentation repository

84cd904

Merge branch 'master' into grapheme

80b90fa

re-update cargo.lock

4ac10ff

cmyr mentioned this pull request Apr 18, 2018

Extremely low perfomance and lags with emoji lists eyelash/xi-gtk#54

Open

jmuk added 2 commits April 30, 2018 10:54

update cargo.toml / lock files

aeb141b

Merge branch 'master' into grapheme

5611443

raphlinus approved these changes Apr 30, 2018

View reviewed changes

jmuk added 2 commits April 30, 2018 15:18

remove a comment

0c27cfc

introduce 'total_len' to cursor

b9bc510

cmyr approved these changes May 1, 2018

View reviewed changes

raphlinus merged commit 6fd9768 into xi-editor:master May 1, 2018

raphlinus mentioned this pull request May 12, 2018

Refine backspace behavior #655

Closed

		@@ -363,13 +366,13 @@ impl Rope {

		// graphemes should probably be developed as a cursor-based interface

Conversation

jmuk commented Feb 13, 2018

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raphlinus commented Feb 14, 2018

Uh oh!

mqzry commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmuk commented Mar 13, 2018

Uh oh!

jmuk commented Mar 13, 2018

Uh oh!

jmuk commented Mar 17, 2018

Uh oh!

jmuk commented Mar 20, 2018

Uh oh!

mqzry commented Apr 29, 2018

Uh oh!

raphlinus commented Apr 29, 2018

Uh oh!

Manishearth commented Apr 29, 2018 via email

Uh oh!

raphlinus commented Apr 29, 2018

Uh oh!

jmuk commented Apr 30, 2018

Uh oh!

raphlinus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

raphlinus commented May 1, 2018

Uh oh!

jmuk commented May 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mqzry commented Mar 13, 2018 •

edited

Loading