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

[RFC 4] Text Selection Overview #89

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

MrGVSV
Copy link
Collaborator

@MrGVSV MrGVSV commented Mar 4, 2022

Objective

Rendered

This RFC is meant to be an overview of text selection. It focuses less on implementation and more on the general concepts. This way, we have a single source of truth as we implement (big changes to the design or specs after finalizing this document might require an addendum document to make note of the changes, a la #72).

Why?

This RFC should set the groundwork for implementing things like text selection, displaying the text insertion cursor (caret), copying/cutting/pasting, etc.

Not all of those will be immediately available to implement, but will likely use features from this RFC as part of their design.

Feedback

Discussion and feedback are very important in making sure the features proposed are generally accepted, not missing important details, and make the lives of implementors easier.

If you have any thoughts, comments, suggestions, or critiques, please leave a comment or review!

MrGVSV added 6 commits March 1, 2022 23:54
Removed sections regarding methods for storing text.

These solutions were a bit too complicated and created lots of edge
cases we would have to handle.
@MrGVSV MrGVSV added the rfc A "request for comments" used to discuss large changes to the API label Mar 4, 2022
@MrGVSV MrGVSV self-assigned this Mar 4, 2022
@MrGVSV MrGVSV requested a review from StarArawn March 4, 2022 21:20
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

So, I've read about 2/3 of it and this is pretty good. My main issue though is that it seems that's going much further than the stated goal of having a cursor and the ability to select text in a TextBox. I agree that all of these concerns will be valid at some point, but I feel like there could be a much smaller RFC that only focuses on the cursor in a single focused TextBox. Considering the main use of kayak right now is to make UI for games with bevy, I don't think we need something as complete as this. There's already enough complexity with this limitation, so I'm not sure we also need to be thinking about an even bigger scope.

It's possible I simply misunderstood the purpose of this RFC and that the focus multi-node selection isn't intended to be the main focus. It's just that it was the first part so it caught my eyes.

@MrGVSV
Copy link
Collaborator Author

MrGVSV commented Mar 8, 2022

My main issue though is that it seems that's going much further than the stated goal of having a cursor and the ability to select text in a TextBox. I agree that all of these concerns will be valid at some point, but I feel like there could be a much smaller RFC that only focuses on the cursor in a single focused TextBox.

Yeah this was one of my concerns when drafting this up. I think I really wanted this to be more of an overview of a lot of the selection-based features. Since there's a lot to consider and many parts interacting with one another, I thought it would be a good idea to go over everything all at once, then create smaller addendum RFCs that really explore one feature. But maybe that's not the best idea if it just causes confusion and is too messy. Does the "Implementation Guide" at the bottom help or would you say it definitely needs to be split up?

Considering the main use of kayak right now is to make UI for games with bevy, I don't think we need something as complete as this. There's already enough complexity with this limitation, so I'm not sure we also need to be thinking about an even bigger scope.

Yeah this is something I keep struggling with. I keep wanting to keep things general or akin to other UI standards, but that might not be something that needs to be focused on right now. We should maybe prioritize the games/bevy side of things (will have to hear @StarArawn's thoughts on it, though).

It's possible I simply misunderstood the purpose of this RFC and that the focus multi-node selection isn't intended to be the main focus. It's just that it was the first part so it caught my eyes.

Hey, if you misunderstood something, that's on me haha. But I think what you said makes sense and I should probably hone in the scope. For example, we could maybe stick to just allowing selection within the bounds of a single node, reduce the number of supported styles, etc. Maybe even selection itself and just focus on the caret itself.

The big question is should we keep this document as the general overview still (and create more focused sub-RFCs) or just edit it down to a small set of features? 🤔

@IceSentry
Copy link
Contributor

Now that I had a good night of sleep to think about it, I think it's fine if we keep this as the final goal, but I think we should either have a separate smaller RFC or just make it clear that this RFC will be done in multiple steps and that the multi node selection stuff is the end goal not the first goal. I think the issue was that the RFC pretty much started with that and it's why I felt like it was too much.

I would also rework the implementation guide to take this into consideration.

Also, adding a table of contents might help with that, because I didn't even realize there was a multi step implementation detail at the bottom.

Copy link
Collaborator Author

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

@IceSentry I clarified that this RFC is meant to be more of an outline.

Another point of this RFC was to cover these topics in a general sense to make it easier for people to contribute to various areas if they'd like to work on part of it. Should I include that as part of it?

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

I really like the latest changes. I think the first paragraph does enough to explain that this can be done by multiple people over many PRs.


Additionally, when it comes to text selection, we generally want the [*grapheme cluster*](https://unicode.org/reports/tr29/#Grapheme_Cluster_Boundaries). This is what—to a user— is typically viewed as a "character".

> Rust's built-in [`chars()`](https://doc.rust-lang.org/stable/std/primitive.str.html#method.chars) method breaks strings down into *Unicode Scalar Values*. For segmentation along grapheme clusters, an external crate like [`unicode-segmentation`](https://crates.io/crates/unicode-segmentation) will likely need to be used.
Copy link
Owner

Choose a reason for hiding this comment

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

There a few different crates for managing this. It might be worth while looking into using some of those.

@MrGVSV MrGVSV mentioned this pull request Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc A "request for comments" used to discuss large changes to the API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants