Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[parley] new selection logic and an editor example #106

Merged
merged 18 commits into from
Aug 22, 2024
Merged

[parley] new selection logic and an editor example #106

merged 18 commits into from
Aug 22, 2024

Conversation

dfrg
Copy link
Collaborator

@dfrg dfrg commented Aug 15, 2024

Adds a new Selection type for dealing with cursor movement and selection.

Also includes a vello_editor example that serves as a testbed for checking the logic. This supports mouse selections, arrow keys (+ shift to extend selections), backspace, delete and basic text input.

Marked as draft because the code is a mess but making this public so people can play with it and report bugs.

dfrg added 2 commits August 14, 2024 21:32
Shows how to implement the next/previous line behaviors for cursors.

Note that this is untested.
Copy link
Contributor

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to add that h_pos ought to be remembered such that the cursor will return back to it's original h pos when moving into a long enough line after moving through short (or empty) lines.

Comment on lines 191 to 195
Some(Cursor::from_point(
layout,
h_pos.unwrap_or(cursor.offset),
y,
))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit silly to be working out the y pos from the line and then Cursor::from_point presumably turning that back into a line, but it'll certainly work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. In theory, I'd rather see it use the data we already know will just be re-found to be the index we have available. But this is fine as-is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. This stuff is really complex (the current code in masonry gets much of it wrong and will be worse with mixed direction text). Once you have fast and robust index/point primitives, you want to use them as much as possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line movement is fundamentally a hit test operation anyway, given the expectation of maintaining the visual horizontal position.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean sure. It would be an optimisation to skip hit testing in the y direction because you already know which line you will hit (right?). If you think it's better not to do that to keep the code simpler then that seems fine. It's not something I feel particularly strongly about (and would also be something that would be easy to change later (perhaps when the codebase is more mature and better tested))

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point taken, we could factor the horizontal search out of from_position and reuse it for this.

@DJMcNab
Copy link
Member

DJMcNab commented Aug 15, 2024

Isn't remembering the h_pos the job of a higher-level API?

DJMcNab
DJMcNab previously approved these changes Aug 15, 2024
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're not planning on doing testing for this, we can just land it.

) -> Option<Cursor> {
let line = layout.get(line_index)?;
let metrics = line.metrics();
let y = metrics.baseline - metrics.line_height * 0.5;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this subtraction needed? I can see that it could make things more robust when finding the lines?
That is, does the actual y within the range (metrics.baseline - metrics.line_height)..metrics.baseline have any significance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it’s just for robustness to ensure we choose a y position within the target line.

Comment on lines 191 to 195
Some(Cursor::from_point(
layout,
h_pos.unwrap_or(cursor.offset),
y,
))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. In theory, I'd rather see it use the data we already know will just be re-found to be the index we have available. But this is fine as-is.

@dfrg
Copy link
Collaborator Author

dfrg commented Aug 15, 2024

If you're not planning on doing testing for this, we can just land it.

I’m going to update this to include a selection primitive with the basic operations: next, prev, next_line, prev_line and selection rects which I’ve now tested to work properly. Stay tuned

commit 6b11f21
Author: Chad Brokaw <[email protected]>
Date:   Fri Aug 16 12:31:34 2024 -0400

    selection and editing

commit 951fc96
Author: Chad Brokaw <[email protected]>
Date:   Fri Aug 16 10:13:13 2024 -0400

    checkpoint

commit ecd232d
Author: Chad Brokaw <[email protected]>
Date:   Thu Aug 15 21:30:27 2024 -0400

    checkpoint
@dfrg dfrg dismissed DJMcNab’s stale review August 16, 2024 16:33

code is substantially changed

@dfrg dfrg marked this pull request as draft August 16, 2024 16:33
@dfrg dfrg changed the title Add next_line/prev_line methods to Cursor [parley] new selection logic and an editor example Aug 16, 2024
.vscode/launch.json Outdated Show resolved Hide resolved
KeyCode::Backspace => {
let start = if self.selection.is_collapsed() {
let end = self.selection.focus().text_start;
if let Some((start, _)) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if let Some((start, _)) =
// TODO: Handle Emoji, etc.
if let Some((start, _)) =

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted but I think this behavior should be an option. I believe some editors (mostly on desktop?) allow you to “decompose” emojis with backspace by removing modifiers. Mobile usually just deletes the whole thing for simplicity.

Parley tracks emoji clusters so adding this here should be significantly simpler than the current code in masonry that tries to parse them manually.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Handy that we can use the existing data for this.

Decomposing also needs some care with the zero width joiners, of course.
I presumed that the decomposing behaviour I was seeing was a bug in vscode, but it does seem to be intentional.

Configuring would be good. Of course, it would also be possible to do even better here, but our job isn't to come up with new UX for emoji input/editing :)

Copy link
Member

@xorgy xorgy Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decomposing emoji is highly dependent on specific editor features that I've never seen implemented in a useful way in a real application. On Android this could conceivably be handled by input methods (through corrections), but on desktop there is no standard UI for it.

I think this is something to prototype in Masonry land, it'd be cool to have something like a ‘variants menu’, but also there's a question about keyboard and screen reader interpretation of that.

parley/src/layout/cursor.rs Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably remove this - consider then adding it to a local gitignore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, yes, I missed that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why GitHub decided to hide the first time I made this comment from me...

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another overall thought - we have written a lot of docs for very similar code before (e.g. the Selection struct), so I would really like to see some of that at least brought across (where licenses are compatible)

But this is looking really promising - using Cursors for the cursor position is compelling, and I'm annoyed I didn't realise I should have been doing that...

@DJMcNab
Copy link
Member

DJMcNab commented Aug 19, 2024

Backlink to #52

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really liking the behaviour I see running this. Not sure what the expectation normally is with arrow-keys in bidi text - this matches vscode (but not firefox)

}

pub fn next_logical<B: Brush>(&self, layout: &Layout<B>, extend: bool) -> Self {
self.maybe_extend(Cursor::from_byte_index(layout, self.focus.text_end), extend)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't quite work for emergency breaks. Pressing right at:

aaaaaaaaaaaaaaaaaaaaaa<|>a[emergency break]
aaaaaa

should give:

aaaaaaaaaaaaaaaaaaaaaaa<|>[emergency break]
aaaaaa

but actually gives:

aaaaaaaaaaaaaaaaaaaaaaa[emergency break]
<|>aaaaaa

A similar thing happens with bidi text (and would also happen with differently styled rich text).

Copy link
Contributor

@PoignardAzur PoignardAzur Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that right now Parley doesn't keep track of cursor affinity. This is not unheard of (chrome doesn't have affinity either) but I also strongly prefer keeping affinity for soft breaks. It's also the behavior of most text editors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that this logic already correctly has state for affinity, as discussed in #106 (comment)

In the example I have given, end and home work as expected for me, the issue is with the left and right keys.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do have this working for soft line breaks but not emergency breaks (yet). This new behavior matches Firefox where moving right at the end of the line places the cursor at the beginning of the next (rather than skipping the first cluster). Note that this effectively requires two key presses to move between characters so affinity doesn’t handle this naturally.

Copy link
Contributor

@PoignardAzur PoignardAzur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this is good progress, and we could definitely use this in Masonry.

The biggest thing I see missing (beside first-class affinity) is word-by-word selection (eg what happens when you do Ctrl+Right). Everything else we can figure out later, but word selection is kinda essential for a first-class editor experience.

Comment on lines 119 to 125
// Resize the surface when the window is resized
WindowEvent::Resized(size) => {
self.context
.resize_surface(&mut render_state.surface, size.width, size.height);
render_state.window.request_redraw();
self.text.update_layout(size.width as _, 1.0);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also update the coordinates of the cursor/selection.

Right now the cursor position becomes stale when you resize the window.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a refresh method to both Cursor and Selection and inserted a call here.

Comment on lines 86 to 87
/// Creates a new cursor for the specified layout and text position.
pub fn from_position<B: Brush>(
layout: &Layout<B>,
mut position: usize,
is_leading: bool,
) -> Self {
pub fn from_byte_index<B: Brush>(layout: &Layout<B>, mut index: usize) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding from reading the code is that, if the byte index you give is in the middle of a grapheme, the cursor returned will basically store a range from the beginning to the end of that grapheme. This property is used by prev_logical where you request the grapheme that contains the byte before the beginning of the current grapheme.

I think that behavior should be described in the doc comment.

Comment on lines 191 to 196
fn move_to_line<B: Brush>(
layout: &Layout<B>,
cursor: &Cursor,
h_pos: Option<f32>,
line_index: usize,
) -> Option<Cursor> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like this method or one similar to be public. It would be helpful to implement PageUp and PageDown.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I’ll just expose a single line motion method with a signed delta

Cursor::from_point(layout, x, y).into()
}

pub fn from_byte_index<B: Brush>(layout: &Layout<B>, index: usize) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should take an affinity parameter or similar.

Copy link
Collaborator Author

@dfrg dfrg Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in as yet unpushed code

@dfrg
Copy link
Collaborator Author

dfrg commented Aug 21, 2024

I’ve been hacking away at this and have some rather large changes incoming.

Daniel: the new behavior matches either pango or firefox (cursor moves visually in bidi sections). The difference is how insertion is handled at boundaries. In pango apps, the visual cursor always represents the “strong” insertion point which is where text matching layout direction would appear. In firefox this depends on how you enter the boundary. (edit: this is controllable with an enum)

Olivier: word movement/selection with ctrl (+shift) is done and working including double click to select a word.

@dfrg
Copy link
Collaborator Author

dfrg commented Aug 21, 2024

I’ve got one tiny bug to fix and then I’ll push the new code for review and we can hopefully get it landed later today

@dfrg
Copy link
Collaborator Author

dfrg commented Aug 21, 2024

Updated with all of my changes now. There are a few debug prints that I'll remove before merging but I think this is in a pretty good state as far as functionality.

Basic rundown:

  • Visual cursor movement matches pango by default. I arrived at this decision after discussion with both Olivier and Behdad.
  • Allows both cursor and mouse navigation to the end of soft line breaks.
  • Word movement/selection is implemented. This works in logical order so will jump around for bidi text. I was unable to find any existing app that does visual movement here 🤷

There are a million little edge cases to handle and a lot of code changes here to address them. Happy to have suggestions for improvement during review but I'd like to defer substantial changes to both code and docs to future work.

edit: editor example also has copy/paste support

Please test and let me know how it works :)

@dfrg dfrg marked this pull request as ready for review August 21, 2024 15:07
@PoignardAzur
Copy link
Contributor

Tried to run it, got this error:

  = note: /usr/bin/ld: cannot find -lxcb-shape: No such file or directory
          /usr/bin/ld: cannot find -lxcb-xfixes: No such file or directory

@dfrg
Copy link
Collaborator Author

dfrg commented Aug 21, 2024

Nice, this is probably the old clipboard crate. I'll try updating to clipboard_rs.

@PoignardAzur
Copy link
Contributor

Amazing work!

Some semi-blocking problems:

  • Pressing up on the first line or down on the last line doesn't move the cursor to the beginning/end of the line.
  • Ctrl+Delete and Ctrl+Backspace aren't implemented.
  • Delete doesn't work correctly with non-collapsed selections (The text is correctly deleted, but the selection isn't collapsed.)

Some non-blocking nitpicks:

  • Double-click stops working if you drag the cursor around without releasing your click.
  • Double-click (select by word) is handled, but triple-click (select by line) isn't.
  • Ctrl+Home and Ctrl+End aren't implemented.

Despite all that, I think this is basically already usable in Masonry. I'll see if I can write an initial integration this evening.

@xorgy
Copy link
Member

xorgy commented Aug 22, 2024

[target.'cfg(target_os = "android")'.dependencies]
winit = { version = "0.30.3", features = ["android-native-activity"] }

To fix the issue with the examples building for Android. You need to select an Activity type from android-activity to build winit for Android.

@xorgy
Copy link
Member

xorgy commented Aug 22, 2024

You will also need to gate clipboard-rs because it doesn't build for Android.

@waywardmonkeys
Copy link
Contributor

For the failed typos check, you can use a variation on this block from the Xilem typos config:

extend-ignore-re = [
    # Matches lorem ipsum text.
    # In general, regexes are only matched until the end of a line by typos,
    # and the repeated matcher at the end of both of these also ensures that
    # matching ends at quotes or symbols commonly used to terminate comments.
    "Lorem ipsum [a-zA-Z .,]*",
    "Phasellus in viverra dolor [a-zA-Z .,]*",
]

@dfrg
Copy link
Collaborator Author

dfrg commented Aug 22, 2024

You will also need to gate clipboard-rs because it doesn't build for Android.

ah, good catch. Is there any crate that supports this or is it something we'll have to implement?

@dfrg
Copy link
Collaborator Author

dfrg commented Aug 22, 2024

For the failed typos check, you can use a variation on this block from the Xilem typos config:

Thanks! Will fix this up too

@DJMcNab
Copy link
Member

DJMcNab commented Aug 22, 2024

ah, good catch. Is there any crate that supports this or is it something we'll have to implement?

I don't think there's much advantage in trying to get it working, because most users on Android won't have a keyboard plugged in anyway.
Although I guess to use this demo, you need to have that plugged in anyway...

@xorgy
Copy link
Member

xorgy commented Aug 22, 2024

Handling for Ime events is one thing that would be good before we go integrating this into Masonry (particularly making sure we can support preedit behavior). I think this can be done as an iteration once this lands though.

- support ctrl+home/end
- fix delete with non-collapsed selections
- move to start/end of line when maxing out up/down respectively
- fix panic on empty text buffer
- proper cursor placement when using left/right to collapse selection
- fix weird cursor offset bug when line ends with RTL run containing only whitespace that is reordered
- triple click to select line
@dfrg
Copy link
Collaborator Author

dfrg commented Aug 22, 2024

Amazing work!

❤️

Some semi-blocking problems:

  • Pressing up on the first line or down on the last line doesn't move the cursor to the beginning/end of the line.

Fixed in Selection::move_lines.

  • Ctrl+Delete and Ctrl+Backspace aren't implemented.

This behavior is sorta weird when the selection is not collapsed so I'm going to defer it.

  • Delete doesn't work correctly with non-collapsed selections (The text is correctly deleted, but the selection isn't collapsed.)

Good catch. This definitely used to work but it appears I broke it at some point. Fixed.

Some non-blocking nitpicks:

  • Double-click stops working if you drag the cursor around without releasing your click.

Will also defer this one. Selection::extend_to_point should probably take some sort of granularity.

  • Double-click (select by word) is handled, but triple-click (select by line) isn't.

Added this.

  • Ctrl+Home and Ctrl+End aren't implemented.

Added this as well.

Despite all that, I think this is basically already usable in Masonry. I'll see if I can write an initial integration this evening.

🎉

@dfrg
Copy link
Collaborator Author

dfrg commented Aug 22, 2024

Handling for Ime events is one thing that would be good before we go integrating this into Masonry (particularly making sure we can support preedit behavior). I think this can be done as an iteration once this lands though.

Adding this to the editor example would be valuable. I'll leave that to others :)

@dfrg dfrg added this pull request to the merge queue Aug 22, 2024
Merged via the queue into main with commit 14070d5 Aug 22, 2024
20 checks passed
@dfrg dfrg deleted the move-line branch August 22, 2024 17:38
waywardmonkeys added a commit to waywardmonkeys/parley that referenced this pull request Sep 12, 2024
This line may have been mistakenly removed in linebender#106
(14070d5).

Fix linebender#114.
github-merge-queue bot pushed a commit that referenced this pull request Sep 12, 2024
This line may have been mistakenly removed in #106
(14070d5).

Fix #114.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants