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
38 changes: 38 additions & 0 deletions parley/src/layout/cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,44 @@ impl Cursor {
pub fn is_trailing(&self) -> bool {
self.text_end == self.insert_point
}

/// Given the layout that generated this cursor, return a new cursor
/// for the corresponding position on the next line.
///
/// If `h_pos` is provided, then it will be used as the horizontal offset
/// for computing the position on the next line.
///
/// Returns `None` if the cursor should remain in its current position.
pub fn next_line<B: Brush>(&self, layout: &Layout<B>, h_pos: Option<f32>) -> Option<Cursor> {
move_to_line(layout, self, h_pos, self.path.line_index.checked_add(1)?)
}

/// Given the layout that generated this cursor, return a new cursor
/// for the corresponding position on the previous line.
///
/// If `h_pos` is provided, then it will be used as the horizontal offset
/// for computing the position on the previous line.
///
/// Returns `None` if the cursor should remain in its current position.
pub fn prev_line<B: Brush>(&self, layout: &Layout<B>, h_pos: Option<f32>) -> Option<Cursor> {
move_to_line(layout, self, h_pos, self.path.line_index.checked_sub(1)?)
}
}

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

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.

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.

}

/// Index based path to a cluster.
Expand Down