Cursor rework #170

dfrg · 2024-11-11T02:26:15Z

This is a draft because some functionality (mostly around word/line selection) is still missing. I also commented out the fix from #163 because the skip_mandatory_break flag is now gone and I haven't dug into the logic from that PR.

Submitting this now so that people can take a look and see if it fixes some of the known bugs, particularly around newline handling.

note: #166 should probably land first

nicoburns · 2024-11-11T02:42:07Z

I also commented out the fix from #163 because the skip_mandatory_break flag is now gone and I haven't dug into the logic from that PR.

Looks like you're now appending the newline cluster to the previous line rather than waiting to append it to the next line. We'll need to test, but I suspect that means that the bug fixed in #163 won't be present. The key thing not being which line the newline cluster ends up in, but that the cluster is "committed" eagerly rather than deferred to the next iteration of the loop (the fix in #163 was handling that deferred cluster commit handling the case that the next iteration of the loop was an inline box (and thus went down a different code path)).

If you wanted to keep the "newline is at the start of the following line" behaviour then think you could probably just reverse the append_cluster_to_line and try_commit_line statements (although IMO it's more intuitive if the newline cluster is part of the line it ends).

nicoburns

I've left some review comments on the layout code that I'm familiar with. I haven't reviewed the selection code which I don't fully understand yet.

parley/src/layout/line/greedy.rs

nicoburns · 2024-11-11T02:46:59Z

parley/src/layout/line/greedy.rs

+                        let whitespace = cluster.info().whitespace();
+                        let is_newline = whitespace == Whitespace::Newline;
+                        let is_space = whitespace.is_space_or_nbsp();
                        let boundary = cluster.info().boundary();


Can you explain why you have switched from Boundary::Mandatory to Whitespace::Newline here? What (if anything) is the difference?

I'm assuming the reason you (can safely) check Boundary::Line before checking is_newline is because a newline will never be a Boundary::Line (it will always be a Boundary::Mandatory)? If this is not the case then I think you need to invert the order in which you check those conditions.

The full explanation is at #132 (comment)

tldr; Unicode LBA specifies a break after the newline and a trailing newline doesn’t have a following cluster so we lose that information.

Re check order: good question. I’m going to assume that a newline sequence is not marked as a break opportunity because the code appears to work as is :)

parley/src/shape.rs

parley/src/layout/line/greedy.rs

spirali · 2024-11-11T11:07:42Z

I did some quick manual testing with 220f1bc and it seems that problem from #163 is gone. The logic behind #163 was to solve the problem that newline cluster is part of the next line and it broke placing inline boxes.

dfrg · 2024-11-11T14:28:36Z

I did some quick manual testing with 220f1bc and it seems that problem from #163 is gone. The logic behind #163 was to solve the problem that newline cluster is part of the next line and it broke placing inline boxes.

Thanks for confirming.

dfrg · 2024-11-11T14:32:16Z

If you wanted to keep the "newline is at the start of the following line" behaviour then think you could probably just reverse the append_cluster_to_line and try_commit_line statements (although IMO it's more intuitive if the newline cluster is part of the line it ends).

This was initially done to force a line to be created at the end of the layout when the last character was a newline. This didn’t really work anyway and I agree that it’s more intuitive (and correct!) to keep the newline attached to the line it ends.

parley/src/shape.rs

parley/src/layout/cursor.rs

mwcampbell · 2024-11-21T13:49:49Z

Cursor::to_access_position returns None when the cursor is at the end of the text (e.g. after pressing Ctrl+End in vello_editor). This is incorrect, as it means we can't report an accessible text selection when the cursor is at the end.

dfrg · 2024-11-21T14:09:56Z

Ah, I see. We won’t have a downstream cluster at the end of the text. I’ll push a fix for this in a bit.

- correct condition for empty text in shaping - compute actual layout height based on line max coordinates - try to handle end of text conditions in AccessKit methods

mwcampbell · 2024-11-21T15:48:36Z

Just tried the latest push with a screen reader. If I go to the end of the default text in vello_editor with Ctrl+End, then the accessible selection is where it should be. But if I then press Enter to start a new blank line at the end, then the accessible selection is on the previous line, after the hard line break, rather than on the new, blank line.

- use from_byte_index in from_accesskit instead of setting index directly to capture correct affinity for trailing newline

dfrg · 2024-11-21T16:09:02Z

This is probably a question of affinity. I pushed an update that might address it.

mwcampbell · 2024-11-21T16:19:31Z

No change to the accessibility behavior when inserting a new blank line at the end.

dfrg · 2024-11-21T16:31:37Z

I wonder if that problem is that there's no actual character on that line and AccessKit doesn't have a text run node associated with it?

mwcampbell · 2024-11-21T17:06:49Z

When inserting a blank line at the end of the text, is there a Parley run for that blank line at the end? It appears that there is now parley run, and an AccessKit text run node, for the one blank line if the text is empty.

dfrg · 2024-11-21T17:17:21Z

I think there's one if the text is empty but there's no run for a final blank line in non-empty text. I guess we need to add one somehow and make sure it's mapped properly in the to/from acessskit methods.

xorgy

Glad to see this coming along. :+ )

parley/src/layout/cursor.rs

xorgy · 2024-11-21T19:18:11Z

parley/src/layout/editor.rs

+            // let range = self
+            //     .selection
+            //     .focus()
+            //     .cluster_path()
+            //     .cluster(&self.layout)
+            //     .map(|c| c.text_range())
+            //     .unwrap_or_default();
+            ActiveText::FocusedCluster(self.selection.focus().affinity(), "")


👁️ 👁️ Needs reintroduced.

I have some new debugging code that is more useful so I'll come up with a way to fit that in.

I've had to add a hack to workaround a (new)? Parley bug where an empty initial text causes a crash Note that later setting the text to be empty does not reproduce the crash

DJMcNab

I've ported to this in linebender/xilem#755. Overall, this works well, except for the mentioned privacy concerns.

Additionally, there seems to be a new crash if the layout/buffer is exactly empty when layout is called, but only the first time.
That is, having an empty initial text crashes, but editing so that the text is empty does not. I have not dug into this very far.

parley/src/layout/editor.rs

DJMcNab · 2024-11-22T11:23:06Z

parley/src/layout/editor.rs

+                    // For newline sequences and emoji, delete the previous cluster
+                    range.start
+                } else {
+                    // Otherwise, delete the previous character


Suggested change

// Otherwise, delete the previous character

// Otherwise, delete the previous character (unicode scalar value)

DJMcNab · 2024-11-22T11:25:25Z

parley/src/layout/editor.rs

-                self.editor.cursor_mode,
-                false,
-            ));
+        self.editor.set_selection(


This is a pre-existing concern, but I'll raise it now; should these methods call refresh_layout? That would be necessary if specifically the text has changed since the last relayout, and I don't think anything protects against that.

DJMcNab · 2024-11-22T11:32:02Z

parley/src/layout/cursor.rs

+        }
+    }
+
+    pub(crate) fn maybe_extend(&self, focus: Cursor, extend: bool) -> Self {


A function of this name used to be public; it's used within editor.rs (with extend: true). Can we just expose an extend function?

mwcampbell · 2024-11-25T15:26:11Z

Is the fix for the problem with accessibility on the last line waiting on something from me?

dfrg · 2024-11-26T00:58:23Z

Is the fix for the problem with accessibility on the last line waiting on something from me?

No, I plan on adding an empty run to the last line to see if that helps. Just haven’t gotten to it yet. I’ll ping here when the PR is updated.

PoignardAzur · 2024-11-26T13:47:07Z

Pressing Delete while the cursor is a the beginning of a line puts the cursor in an invalid position.

... also various fixes from review feedback

DJMcNab · 2024-11-26T15:34:37Z

parley/src/layout/cluster.rs

@@ -394,10 +372,12 @@ impl<'a, B: Brush> Cluster<'a, B> {
 /// Determines how a cursor attaches to a cluster.
 #[derive(Copy, Clone, PartialEq, Eq, Default, Debug)]
 pub enum Affinity {
-    /// Left side for LTR clusters and right side for RTL clusters.
+    /// Cursor is attached to the character that is logically following in the
+    /// text stream.


Nice! This is so much better.

dfrg added 2 commits November 10, 2024 20:08

wip new cursor impl

dc64090

remove cursor2

220f1bc

nicoburns reviewed Nov 11, 2024

View reviewed changes

DJMcNab reviewed Nov 11, 2024

View reviewed changes

parley/src/shape.rs Show resolved Hide resolved

xorgy reviewed Nov 12, 2024

View reviewed changes

parley/src/layout/cursor.rs Show resolved Hide resolved

DJMcNab mentioned this pull request Nov 18, 2024

Update to Vello 0.3.0, Parley main, AccessKit 0.17 linebender/xilem#616

Merged

nicoburns added this to the 0.3 Release milestone Nov 18, 2024

dfrg added 10 commits November 18, 2024 20:24

Merge branch 'main' into cursor2

caddbca

checkpoint

21703db

commit newline cluster with accumulated advance

8a17b50

clean up shaping/line breaking changes

8f3a1d1

fmt

452c3dd

additional support for word selection

d55f713

Merge branch 'main' into cursor2

f651bae

license header

6b9319a

unbreak CI

9ea31fe

for now, drop support for secondary cursor

7cae451

dfrg mentioned this pull request Nov 21, 2024

PlainEditor: cursor disappears when all text is deleted #183

Open

various fixes

d227280

- correct condition for empty text in shaping - compute actual layout height based on line max coordinates - try to handle end of text conditions in AccessKit methods

docs and another attempted accesskit fix

3c60140

- use from_byte_index in from_accesskit instead of setting index directly to capture correct affinity for trailing newline

xorgy mentioned this pull request Nov 21, 2024

Implement IME primitives for PlainEditor. #136

Draft

DJMcNab mentioned this pull request Nov 21, 2024

Ctrl-End followed by Ctrl-Del makes the vello_editor example panic #179

Open

dfrg marked this pull request as ready for review November 21, 2024 17:17

dfrg changed the title ~~WIP cursor rework~~ Cursor rework Nov 21, 2024

xorgy requested changes Nov 21, 2024

View reviewed changes

DJMcNab reviewed Nov 22, 2024

View reviewed changes

PoignardAzur linked an issue Nov 26, 2024 that may be closed by this pull request

Ctrl-End followed by Ctrl-Del makes the vello_editor example panic #179

Open

handle word selection

a591d58

... also various fixes from review feedback

DJMcNab reviewed Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cursor rework #170

Cursor rework #170

dfrg commented Nov 11, 2024

nicoburns commented Nov 11, 2024

nicoburns left a comment

nicoburns Nov 11, 2024

dfrg Nov 11, 2024

spirali commented Nov 11, 2024

dfrg commented Nov 11, 2024

dfrg commented Nov 11, 2024

mwcampbell commented Nov 21, 2024

dfrg commented Nov 21, 2024

mwcampbell commented Nov 21, 2024

dfrg commented Nov 21, 2024

mwcampbell commented Nov 21, 2024

dfrg commented Nov 21, 2024

mwcampbell commented Nov 21, 2024

dfrg commented Nov 21, 2024

xorgy left a comment

xorgy Nov 21, 2024 •

edited

Loading

dfrg Nov 26, 2024

DJMcNab left a comment

DJMcNab Nov 22, 2024

DJMcNab Nov 22, 2024

DJMcNab Nov 22, 2024

mwcampbell commented Nov 25, 2024

dfrg commented Nov 26, 2024

PoignardAzur commented Nov 26, 2024

DJMcNab Nov 26, 2024

	// Otherwise, delete the previous character
	// Otherwise, delete the previous character (unicode scalar value)

Cursor rework #170

Are you sure you want to change the base?

Cursor rework #170

Conversation

dfrg commented Nov 11, 2024

nicoburns commented Nov 11, 2024

nicoburns left a comment

Choose a reason for hiding this comment

nicoburns Nov 11, 2024

Choose a reason for hiding this comment

dfrg Nov 11, 2024

Choose a reason for hiding this comment

spirali commented Nov 11, 2024

dfrg commented Nov 11, 2024

dfrg commented Nov 11, 2024

mwcampbell commented Nov 21, 2024

dfrg commented Nov 21, 2024

mwcampbell commented Nov 21, 2024

dfrg commented Nov 21, 2024

mwcampbell commented Nov 21, 2024

dfrg commented Nov 21, 2024

mwcampbell commented Nov 21, 2024

dfrg commented Nov 21, 2024

xorgy left a comment

Choose a reason for hiding this comment

xorgy Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

dfrg Nov 26, 2024

Choose a reason for hiding this comment

DJMcNab left a comment

Choose a reason for hiding this comment

DJMcNab Nov 22, 2024

Choose a reason for hiding this comment

DJMcNab Nov 22, 2024

Choose a reason for hiding this comment

DJMcNab Nov 22, 2024

Choose a reason for hiding this comment

mwcampbell commented Nov 25, 2024

dfrg commented Nov 26, 2024

PoignardAzur commented Nov 26, 2024

DJMcNab Nov 26, 2024

Choose a reason for hiding this comment

xorgy Nov 21, 2024 •

edited

Loading