Implement vertical movement (for text editing)#1280
Conversation
cmyr
force-pushed
the
text-editor
branch
2 times, most recently
from
8163550 to
10160ed
Compare
cmyr
force-pushed
the
vertical-movement
branch
from
dbef385 to
2671d48
Compare
justinmoon
left a comment
justinmoon
left a comment
There was a problem hiding this comment.
Besides the one variable naming nit, this works great for me on Windows
| pub end: usize, | ||
|
|
||
| /// The saved horizontal position, during vertical movement. | ||
| pub h_pos: Option<f64>, |
There was a problem hiding this comment.
It's a little weird how this variable is called h_pos here and x_pos on 105
cmyr
force-pushed
the
vertical-movement
branch
from
2671d48 to
cc6f946
Compare
cmyr
added
S-needs-review
cmyr
force-pushed
the
vertical-movement
branch
from
cc6f946 to
704fa2f
Compare
raphlinus
left a comment
raphlinus
left a comment
There was a problem hiding this comment.
Generally this looks good, very small nits inline.
It's fun to see the capabilities slowly grow :)
| } else { | ||
| let lm = layout.line_metric(cur_pos.line).unwrap(); | ||
| let h_pos = s.h_pos.unwrap_or(cur_pos.point.x); | ||
| // may not work correctly for point sizes below 1.0 |
There was a problem hiding this comment.
What I did in xi2 is add half the line height. I think that's most robust but I'm not sure it matters that much - point sizes below 1.0 are not really a thing.
There was a problem hiding this comment.
This comment was intended to be somewhat tongue in cheek 😅.
Wouldn't adding half the line-height would fail if our current line-height is more than 2x the height of the preceding line, which doesn't seem impossible?
This seemed like the most robust approach, but there may be issues I'm not considering.
druid/src/text/movement.rs
Outdated
| /// | ||
| /// returns a new selection representing the state after the movement. | ||
| /// | ||
| /// If `modify` is true, only the leading edge (the 'end') of the selection |
There was a problem hiding this comment.
Is "leading" the right word here? It seems to me "trailing".
There was a problem hiding this comment.
fair, I guess "active" is the better word.
druid/src/text/movement.rs
Outdated
| Movement::Up => { | ||
| let cur_pos = layout.hit_test_text_position(s.end); | ||
| if cur_pos.line == 0 { | ||
| (0, Some(cur_pos.point.x)) |
There was a problem hiding this comment.
I think this is the behavior of our existing code, but I think Some(s.h_pos.unwrap_or(cur_pos.point.x)) is slightly more precise. And that might lead to logic simplification because I think it's the same in both branches.
So I'm testing different editors and not finding a really authoritative answer. TextEdit on mac is consistent with the suggestion I posted, as is Sublime (tested on mac). VS Code (tested on Windows) is consistent with the code you have. WordPad (Windows) doesn't move the cursor in this case. So I think any of these behaviors are reasonable, and if you want to go with what you have, I won't block on it.
Note that the code as you have it matches xi2, so I certainly can't complain :)
There was a problem hiding this comment.
Good call, I think I prefer preserving the existing h_pos if possible.
cmyr
force-pushed
the
vertical-movement
branch
from
704fa2f to
b74b209
Compare
4 participants
Based off of #1274
This implements vertical motion for text editing.
I think the traits we use here may change over time, but this gets something working for now.
(this is slightly buggy because it relies on a pending piet release)