Skip to content

Conversation

valadaptive
Copy link
Contributor

Other text layout libraries in Rust--namely, Parley and Cosmic Text--have one canonical text cursor type (Parley's is a byte index, Cosmic Text's also stores the line index). To prepare for migrating egui to one of those libraries, it should also have only one text cursor type. I also think simplifying the API is a good idea in and of itself--having three different cursor types that you have to convert between (and a Cursor struct which contains all three at once) is confusing.

After a bit of experimentation, I found that the best cursor type to coalesce around is CCursor. In the few places where we need a paragraph index or row/column position, we can calculate them as necessary.

I've removed CursorRange and PCursorRange (the latter appears to have never been used), merging the functionality with CCursorRange. To preserve the cursor position when navigating row-by-row, CCursorRange now stores the previous horizontal position of the cursor.

I've also removed PCursor, and renamed RowCursor to LayoutCursor (since it includes not only the row but the column). I have not renamed either CCursorRange or CCursor as those names are used in a lot of places, and I don't want to clutter this PR with a bunch of renames. I'll leave it for a later PR.

Finally, I've removed the deprecated methods from TextEditState--it made the refactoring easier, and it should be pretty easy to migrate to the equivalent TextCursorState methods.

I'm not sure how many breaking changes people will actually encounter. A lot of these APIs were technically public, but I don't think many were useful. The TextBuffer trait now takes &CCursorRange instead of &CursorRange in a couple of methods, and I renamed CCursorRange::sorted to CCursorRange::sorted_cursors to match CursorRange.

I did encounter a couple of apparent minor bugs when testing out text cursor behavior, but I checked them against the current version of egui and they're all pre-existing.

Copy link

Preview available at https://egui-pr-preview.github.io/pr/5785-cursor-refactor
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

@lucasmerlin lucasmerlin added feature New feature or request egui labels Mar 13, 2025
@lucasmerlin
Copy link
Collaborator

Awesome! This looks great, and I also couldn't find anything unusal when testing.

@lucasmerlin
Copy link
Collaborator

lucasmerlin commented Mar 18, 2025

Found a small bug in the markdown editor (just pressing up/down, starting at the lower line):

Screen.Recording.2025-03-18.at.11.06.42.mov

The eventual goal is to have a single cursor type, which matches how
newer text layout libraries do it. This should also simplify the API,
meaning we don't have to juggle 3 different cursor types. To start,
convert everything to CCursorRange.
We are now left with CCursor, for recording character positions, and
RCursor, for recording layout positions.
I'll leave renaming CCursor to an entirely different PR, since that name
is used *everywhere*.
These checks were here for the purpose of preserving the horizontal
position of the cursor (via its column number) if it extended past the
end of the current row. Since we now store the explicit horizontal
position, they're no longer necessary.
@valadaptive
Copy link
Contributor Author

There were some old checks remaining from the previous "cursor up/down one row" code that existed to preserve the cursor's horizontal position, but don't make sense now that it's stored explicitly. Removing those seems to have fixed that issue.

Copy link
Collaborator

@lucasmerlin lucasmerlin left a comment

Choose a reason for hiding this comment

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

Awesome, great work!

@emilk emilk added refactor and removed feature New feature or request labels Mar 20, 2025
@emilk emilk merged commit 2674859 into emilk:master Mar 20, 2025
25 checks passed
@valadaptive
Copy link
Contributor Author

A note if this makes it into a release before the Parley changes do: this won't be the last change to the text cursor API, since the Parley API reworks will change the text cursors to be byte-index-based instead of character-index-based.

It's probably going to be a bit annoying for users of the text cursor APIs to adjust to two reworks in a row, but giving them a heads-up about it should make things easier.

(The cursors being character-index-based only really provide an advantage in an ab_glyph world where one character == one glyph. Once we move away from that, byte indices will be a lot simpler.)

emilk pushed a commit that referenced this pull request Jun 15, 2025
Closes #7077.

This fixes the problem shown in #7077 where clearing a `TextEdit`
wouldn't reset its cursor position. I've fixed that by adding back the
`TextCursorState::range` method, which clamps the selection range to
that of the passed `Galley`, and calling it in the same places where it
was called before #5785.

(/cc @juancampa)

* [x] I have followed the instructions in the PR template
darkwater pushed a commit to darkwater/egui that referenced this pull request Aug 24, 2025
<!--
Please read the "Making a PR" section of
[`CONTRIBUTING.md`](https://github.com/emilk/egui/blob/master/CONTRIBUTING.md)
before opening a Pull Request!

* Keep your PR:s small and focused.
* The PR title is what ends up in the changelog, so make it descriptive!
* If applicable, add a screenshot or gif.
* If it is a non-trivial addition, consider adding a demo for it to
`egui_demo_lib`, or a new example.
* Do NOT open PR:s from your `master` branch, as that makes it hard for
maintainers to test and add commits to your PR.
* Remember to run `cargo fmt` and `cargo clippy`.
* Open the PR as a draft until you have self-reviewed it and run
`./scripts/check.sh`.
* When you have addressed a PR comment, mark it as resolved.

Please be patient! I will review your PR, but my time is limited!
-->

* Closes N/A, but this is part of
emilk#3378
* [x] I have followed the instructions in the PR template

Other text layout libraries in Rust--namely, Parley and Cosmic
Text--have one canonical text cursor type (Parley's is a byte index,
Cosmic Text's also stores the line index). To prepare for migrating egui
to one of those libraries, it should also have only one text cursor
type. I also think simplifying the API is a good idea in and of
itself--having three different cursor types that you have to convert
between (and a `Cursor` struct which contains all three at once) is
confusing.

After a bit of experimentation, I found that the best cursor type to
coalesce around is `CCursor`. In the few places where we need a
paragraph index or row/column position, we can calculate them as
necessary.

I've removed `CursorRange` and `PCursorRange` (the latter appears to
have never been used), merging the functionality with `CCursorRange`. To
preserve the cursor position when navigating row-by-row, `CCursorRange`
now stores the previous horizontal position of the cursor.

I've also removed `PCursor`, and renamed `RowCursor` to `LayoutCursor`
(since it includes not only the row but the column). I have not renamed
either `CCursorRange` or `CCursor` as those names are used in a lot of
places, and I don't want to clutter this PR with a bunch of renames.
I'll leave it for a later PR.

Finally, I've removed the deprecated methods from `TextEditState`--it
made the refactoring easier, and it should be pretty easy to migrate to
the equivalent `TextCursorState` methods.

I'm not sure how many breaking changes people will actually encounter. A
lot of these APIs were technically public, but I don't think many were
useful. The `TextBuffer` trait now takes `&CCursorRange` instead of
`&CursorRange` in a couple of methods, and I renamed
`CCursorRange::sorted` to `CCursorRange::sorted_cursors` to match
`CursorRange`.

I did encounter a couple of apparent minor bugs when testing out text
cursor behavior, but I checked them against the current version of egui
and they're all pre-existing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants