masonry: adopt ui-events text input events#1700
masonry: adopt ui-events text input events#1700waywardmonkeys wants to merge 1 commit intolinebender:mainfrom
Conversation
|
This is a set of (in progress) changes that correspond to endoli/ui-events#114 There will probably be a PR for Parley as well to improve This doesn't use the new |
Switch Masonry to the `ui-events` text input model and remove the local IME event copy. * carry `TextInputEvent` through `masonry_core` and `masonry_winit` * translate `TextArea` to the new text input/event model * synthesize composition end on Masonry-side IME teardown * add tests for replacement, selection, composition, actions, and current unsupported cases
waywardmonkeys
force-pushed
the
ui-events-update-text-edit
branch
from
dd2a4c8 to
65b1bae
Compare
|
There are changes to Parley in flight in linebender/parley#595 ... if that were to land, then some of the code here can be cleaned up and improved. |
PoignardAzur
left a comment
PoignardAzur
left a comment
There was a problem hiding this comment.
I'm not familiar enough with text_area.rs to notice if some egregious mistake was made, but other than that everything seems fine.
| #[test] | ||
| fn set_composing_region_marks_existing_document_range() { | ||
| let area = NewWidget::new(TextArea::new_editable("hello")); | ||
| let mut harness = TestHarness::create(test_property_set(), area); | ||
| let text_id = harness.root_id(); | ||
|
|
||
| harness.focus_on(Some(text_id)); | ||
| assert_eq!( | ||
| harness.process_text_event(TextEvent::TextInput(TextInputEvent::SetComposingRegion( | ||
| TextTargetRange::utf8_bytes(1, 4), | ||
| ))), | ||
| Handled::Yes | ||
| ); | ||
|
|
||
| let area = harness.root_widget(); | ||
| assert_eq!(area.editor.raw_text(), "hello"); | ||
| assert_eq!(area.editor.raw_compose().clone(), Some(1..4)); | ||
| assert_eq!(area.text().to_string(), "ho"); | ||
| } |
There was a problem hiding this comment.
I was a bit confused by this:
- What I'm reading is that the
SetComposingRegionevent takes text "out" of the "canonical" buffer and into the "composing" text, i.e. the temporary text that represents what IME editors suggest to you when you're typing something. Therefore,SetComposingRegionchanges whatTextArea::text()will return. Is that the intended behavior? What's the practical use case? - It took me a while to get why the final string is
"ho"and not"h". I initially assumed thatutf8_bytestook an offset and a length. I think this could be made clearer by either havingutf8_bytestake aRangeor, in this specific case, indexing into a longer string (so that the range would be something like11..14).
| impl RenderRootState { | ||
| pub(crate) fn reset_ime_area_tracking(&mut self) { | ||
| self.last_sent_ime_area = INVALID_IME_AREA; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
There's already an impl RenderRootState block in the file. I'd rather keep type declarations and impl blocks separate to be consistent with the rest of the codebase.
| /// An IME event. | ||
| Ime(Ime), | ||
| /// A transport-agnostic text-input event. | ||
| TextInput(TextInputEvent), |
There was a problem hiding this comment.
Is this still expected to mostly be used for IME-adjacent stuff?
In other words, if a user on an ASCII keyboard presses their "A" key, we still expect not to send this event, right?
I get that "transport-agnostic text-input event "is technically correct, but to most user it will be very uninformative.
I'd rather have a description like "A text-input event emitted for IME actions, Android text events and similar non-keyboard text inputs" that gives users a mental model of "when am I going to get this".
| pub fn preedit(text: String) -> Self { | ||
| Self::Ime(Ime::Preedit(text, None)) | ||
| Self::TextInput(TextInputEvent::CompositionUpdate(CompositionState::new( | ||
| text, | ||
| ))) | ||
| } |
There was a problem hiding this comment.
At this point we should probably rename the method as well.
| /// # Panics | ||
| /// | ||
| /// If **selected** isn't a substring of **text**. | ||
| pub fn preedit_with_cursor(text: String, selected: String) -> Self { |
There was a problem hiding this comment.
Same here. Not sure what the best name would be, because composition_update_with_selection is too long.
2 participants
Switch Masonry to the
ui-eventstext input model and remove the local IME event copy.TextInputEventthroughmasonry_coreandmasonry_winitTextAreato the new text input/event model