bevy_text::input module with no undo

[Zeophlite]

Objective

• bevy_text::input module #20336 without undo/redo

Testing

• CI

[alice-i-cecile]

I've spent some time documenting this PR and making very minor tweaks. After digging in deeper, I have some complaints that I would like to resolve.

I feel very mixed about shipping this as is, without even a UI example. On the one hand, this is a very nice piece of very useful functionality. On the other hand, users will get excited about "oh wow, text input! I should use this in a UI" and promptly run into a brick wall.

[viridia]

I feel very mixed about shipping this as is, without even a UI example.

What needs to happen is to extract the widgets and examples from the original pr: #20326. This has both single line and multi line widgets, as well as multiple examples. Looking at the diffs, I think there's only a handful of large source files which would need to be copied over.

This could be done as a separate PR.

I would also make several changes:

• Migrate any changes relative to the text input code in this PR.
• Rename the text input widgets to TextInput and TextArea to be consistent with web nomenclature.
• Move the widgets into the same crate as the other core widgets (presumably after the renaming of that crate: Consider renaming bevy_core_widgets to bevy_headless_widgets #20664).

Once all that is done, an additional follow-on PR could implement a feathers-branded text input widget. The main downside there is that any changes made to feathers increases the amount of BSN migration technical debt.

[tigregalis]

Sorry for posting this as a comment rather than a proper review, looking at this on my phone. Wanted to get my feedback out there though.

In summary, I think Bevy needs a text input desperately, so although this is not exactly the approach I'd take, I think it's a step in the right direction, though I would like to see some things revisited to unify and deduplicate the text pipeline and rendering even if that does result in inevitable breaking changes (though I think required components would limit the blast radius). One exception is that I strongly believe TextEdit::Submit doesn't belong on the TextEdit enum.

In detail:

I don't understand space_advance - I assume it's working around some bug or limitation: what's the issue it's solving?

I wasn't aware you could just do Editor<'static>. That's cool. I guess this means you have to duplicate much of the text backend logic, since you're accessing the Editor's buffer instead of a normal buffer, that seems unfortunate. Ideally I'd want to be able to just add an Editor component to some existing top level Text entity: in my experiments I just extracted the two fields from Editor into an EditorState struct, then "paused" the editing, and "resumed" the editing as required. Although it having its own buffer means a lot of the existing Text pipeline work is duplicated for text inputs, the nice thing about this approach (having the text input be special-cased) is it gets around the lossiness of cosmic-text's editing abstraction, so you don't have to worry about synchronisation with text entities. But note one advantage of not having special-cased text is having selectable (rich) text without editing. Maybe we can generalise the rendering of text; for example, I really want a Text Gizmo.

In a follow-up I'd like for a Cursor to be a related entity, rather than defined by components and resources. This will make it easier for someone to implement a code editor with multiple cursors for example; maybe that's out of scope but I think it wouldn't add too much complexity.

Text is always joined via a "\n" but for correctness you might use the BufferLine's LineEnding (e.g. on windows it might be \r\n if coming from a file or a paste), but I'm not 100% sure on this.

Another thing you might consider is a get_text_into(target_buffer, ..) just to allow a user to avoid allocation/reallocation of a new string, and to reuse an existing allocation, particularly if fetched for temporary reasons. e.g. this could be used for the password mask.

20.0 is used as a magic constant in a few places, can we make it a named constant(s)?

As a default, line height of 1.2x font size is used generally across web, and looks good generally. For example I think it's in firefox's user agent stylesheet if I'm not wrong.

Comment on Paste should note it replaces the current selection, not inserting at the cursor. More broadly, all insertions replace the current selection, sometimes the selection just has a width of 0.

TextEdit::Submit feels like it belongs in the realm of TextInput being a full widget, maybe even as part of a Form, so its inclusion is odd when there's not yet a full widget. It's the only variant that is special-cased in the apply_text_edits handler system. It's also special-cased in the apply_text_edit function. I would be inclined to remove it and revisit it as a higher level action with the text input widget, and not tied at all to text editing.

For the PasswordMask, it's pretty niche especially without having a full widget yet, but I can understand why you'd want it. Implementation of having two editors with two buffers does feel odd to me; the EditorState idea could help here too. One thing I'd suggest here is that, because the length of the text will be the same, you can basically just replace the hidden editor's Cursor and Selection with the PasswordMask's Cursor and Selection, then you don't need to worry about weird pixel-level mismatches (the recommendation about fixed width fonts won't be relevant), e.g. when the password is

wwwwwwww

but it is rendered as

********

It may be necessary to separate text change from text selection actions to make this work.

For text selections, get the selection and cursor of the mask editor, apply that selection and cursor directly to the hidden editor. i.e. text selections only directly affect the mask editor, and the state of the hidden editor is derived.

For text changes, apply the TextEdits to the hidden editor, get the text from the hidden editor and replace each char with the mask char, and finally replace the text of the mask editor. i.e. text changes only directly affect the hidden editor, and the state of the mask editor is derived.

The PasswordMask I think is very similar conceptually to a syntax-highlighted code editor, a visual representation the user can interact with, and an underlying representation the user's edits actually affect, so there may be a more general solution here. Perhaps related entities?

Should ScrollingLayoutRunIter be upstreamed if it's a bug with cosmic-text?

[Zeophlite]

Hey @tigregalis , I think the below addresses your feedback for this release, and the rest can be deferred for another PR?

I don't understand space_advance - I assume it's working around some bug or limitation: what's the issue it's solving?

I'm not super sure either, @ickshonpe @viridia can you chime in?

Text is always joined via a "\n" but for correctness you might use the BufferLine's LineEnding (e.g. on windows it might be \r\n if coming from a file or a paste), but I'm not 100% sure on this.

I'm not sure on this, we just call editor.action(Action::Enter); and cosmic_text handles that

20.0 is used as a magic constant in a few places, can we make it a named constant(s)?

Done

As a default, line height of 1.2x font size is used generally across web, and looks good generally. For example I think it's in firefox's user agent stylesheet if I'm not wrong.

Added a comment to reflect this

Comment on Paste should note it replaces the current selection, not inserting at the cursor. More broadly, all insertions replace the current selection, sometimes the selection just has a width of 0.

Added a comment

TextEdit::Submit feels like it belongs in the realm of TextInput being a full widget, maybe even as part of a Form, so its inclusion is odd when there's not yet a full widget. It's the only variant that is special-cased in the apply_text_edits handler system. It's also special-cased in the apply_text_edit function. I would be inclined to remove it and revisit it as a higher level action with the text input widget, and not tied at all to text editing.

I agree, I've separated the "submission" into the example, this could be merged back later, depending on how "forms" look

[viridia]

I'm not sure what space_advance is. However, what I can tell you is that this is an area in which desktop UIs vary.

Early terminal displays did not support I-beam style cursors, and could only blink a fixed-width character on and off; when the cursor was at the end of the line, it would blink an empty space.

In the original MacOS UI guidelines, and for many years after, the standard behavior for most GUIs was that when you selected the invisible line-break character at the end of a wrapped line, the selection highlight would be extended all the way to the right of the text input box, that is, the left edge of the rectangle would start just after the last character on the line, and the right edge of the rectangle would be the inner right padding of the containing box.

However, some modern apps (like Chrome and VSCode) do not do this: instead, selecting the wrap character produces a cursor of some fixed width. When there is no wrap character (that is, either the cursor is at the end of the text, or it broke the line in the middle of a word) it shows a normal I-beam style cursor.

[tigregalis]

Hey @tigregalis , I think the below addresses your feedback for this release, and the rest can be deferred for another PR?

Yeah, sounds good.

I am a little hesitant about the password mask and the input filter being part of the same PR, but if those are the features people are looking for ultimately, maybe it doesn't make sense to hold them back for the sake of code history/evolution clarity.

[atlv24]

i am concerned about how much code is in the example. additionally, it seems this is just for 2d, can it be used in a ui in 3d or etc easily?

[Zeophlite]

i am concerned about how much code is in the example. additionally, it seems this is just for 2d, can it be used in a ui in 3d or etc easily?

There's a lot in the example as this is the "low level" api - the follow-up PR has the "mid level" api for widgets, etc. Have a look at the examples/ in https://github.com/bevyengine/bevy/pull/20326/files .

[mockersf]

I've played a bit with this PR and how to use it, and I think it introduces too many things at once, and some that doesn't interact well together.

I would prefer a simpler PR that would introduce a component that holds a cosmic buffer, and how to interact with it. Making it smaller would also improve the code organisation.

I don't think this should be merged in this state.

[Zeophlite]

and some that doesn't interact well together

@mockersf Could you expand on this point?