Improve IME area reporting and set IME area in TextArea
#785
Conversation
This also adds the required context methods for (re)setting the IME area. These are added to the mutable context types, which I believe is the right place. Note Masonry currently reports new IME cursor areas only when the widget itself moves or the focus changes to a new widget, but Parley's `PlainEditor` determines the IME cursor area based on the selection and preedit state of the editor, meaning the area can change based on events as well. Other widgets may choose to have different behavior still. (It appears Masonry currently doesn't account for the IME area potentially changing based on relayout, which may be a bug.) This replaces the old IME area reporting with a check at the end of the rewrite pass to see if IME is active, and if it is, whether the IME area has moved. If it has, the new area is reported to the platform.
| if self.global_state.is_ime_active { | ||
| let widget = self | ||
| .global_state | ||
| .focused_widget | ||
| .expect("IME is active without a focused widget"); | ||
| let ime_area = self.widget_arena.get_state(widget).item.get_ime_area(); | ||
| if self.global_state.last_sent_ime_area != ime_area { | ||
| self.global_state.last_sent_ime_area = ime_area; | ||
| self.global_state | ||
| .emit_signal(RenderRootSignal::new_ime_moved_signal(ime_area)); | ||
| } | ||
| } |
There was a problem hiding this comment.
My understanding is that these rewrite passes are intended to make the state consistent, so Masonry will know the correct IME area once this has been done. Some other signals are emitted to the platform here as well. While I believe it makes sense to add this here, I can certainly be convinced otherwise.
| /// Get the IME area from the editor, accounting for padding. | ||
| /// | ||
| /// This should only be called when the editor layout is available. | ||
| fn ime_area(&self) -> Rect { |
There was a problem hiding this comment.
This being repeated so often to me suggests that it should be a method on Widget, so that there is only one source of truth needed. That would be a bit of a bigger change to wire up, and I don't have all the specifics.
That is, this is a non-blocking conern
| if self.global_state.is_ime_active { | ||
| let widget = self | ||
| .global_state | ||
| .focused_widget | ||
| .expect("IME is active without a focused widget"); | ||
| let ime_area = self.widget_arena.get_state(widget).item.get_ime_area(); | ||
| if self.global_state.last_sent_ime_area != ime_area { | ||
| self.global_state.last_sent_ime_area = ime_area; | ||
| self.global_state | ||
| .emit_signal(RenderRootSignal::new_ime_moved_signal(ime_area)); | ||
| } | ||
| } |
| .global_state | ||
| .focused_widget | ||
| .expect("IME is active without a focused widget"); | ||
| let ime_area = self.widget_arena.get_state(widget).item.get_ime_area(); |
There was a problem hiding this comment.
It's conceivable that you have enough context to call a Widget method with a QueryCtx here.
There was a problem hiding this comment.
Yes, that probably is possible. Hmm, my main concern with both using QueryCtx and removing the repeated calls to set the IME area that you mentioned on TextArea::ime_area, would be that the results aren't cached. In general, calculating the area can be a somewhat expensive operation – and certainly in the case of PlainEditor it is somewhat expensive.
I believe the question is then whether WidgetState stores the IME area, which is what Masonry currently does, or whether that field should be removed in favor of a query method on Widget, which moves the responsibility of caching when necessary to the widget. This doesn't solve the issue of the repeated TextArea::ime_area calls.
There was a problem hiding this comment.
Hmm, that's true, and it's probably too messy. Without having a new invalidation method, it doesn't really work.
--------- Co-authored-by: Daniel McNab <[email protected]>
tomcur
changed the title
TextArea
|
I've gone ahead and merged, but am happy to revisit your comments about the source of truth. |
This adds the required context methods for (re)setting the IME area. These are added to the mutable context types, which I believe is the right place.
TextAreanow usesPlainEditor::ime_cursor_areato report its IME area.Note Masonry currently reports new IME cursor areas only when the widget itself moves or the focus changes to a new widget, but Parley's
PlainEditordetermines the IME cursor area based on the selection and preedit state of the editor, meaning the area can change based on events as well. Other widgets may choose to have different behavior still.(It appears Masonry currently doesn't account for the IME area potentially changing based on relayout, which may be a bug.)
This replaces the old IME area reporting with a check at the end of the rewrite pass to see if IME is active, and if it is, whether the IME area has moved. If it has, the new area is reported to the platform.