-
Notifications
You must be signed in to change notification settings - Fork 27
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
Changes from 2 commits
60b99d1
54d08a0
272ca76
e8ab0eb
1d2402e
0938434
ee7f063
596a2af
f380d80
5aceaaa
abc9da8
d72da0d
8e4b199
c352505
0639357
66979c5
d02765a
3beb203
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> { | ||
let line = layout.get(line_index)?; | ||
let metrics = line.metrics(); | ||
let y = metrics.baseline - metrics.line_height * 0.5; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Point taken, we could factor the horizontal search out of |
||
} | ||
|
||
/// Index based path to a cluster. | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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