Clean up IME handling code #599
Conversation
|
I just realized Textbox never actually calls register_as_text_input. Oops. This is yet one more thing I'd like to test, but expressing it in a test is kinda awkward. |
|
I'll review when that is fixed. |
|
Should be fixed, but I haven't tested it locally. |
PoignardAzur
force-pushed
the
ime_stuff
branch
from
2cf7294 to
e7dc81c
Compare
masonry/src/render_root.rs
Outdated
| @@ -598,6 +593,21 @@ impl RenderRoot { | |||
| } | |||
| } | |||
|
|
|||
| impl RenderRootSignal { | |||
| pub(crate) fn ime_moved(area: Rect) -> Self { | |||
There was a problem hiding this comment.
Can we rename this to from_ime_area or similar?
ime_moved reads like a variant, but the capitalisation is wrong. Alternatively, could we just make ImeMoved take a Rect.
masonry/src/passes/update.rs
Outdated
| let was_ime_active = root.state.is_ime_active; | ||
| let is_ime_active = if let Some(id) = next_focused { | ||
| root.widget_arena.get_state(id).item.is_text_input | ||
| } else { | ||
| false | ||
| }; | ||
| root.state.is_ime_active = is_ime_active; |
There was a problem hiding this comment.
Why is this outside the below check?
| pub(crate) is_portal: bool, | ||
|
|
||
| /// Tracks whether widget is eligible for IME events. | ||
| /// Should be immutable after `WidgetAdded` event. |
There was a problem hiding this comment.
Why? I mean I realise that there is no way to unset this, but are there scenarios where we'd want this to change?
There was a problem hiding this comment.
Basically I'd like to be able to write our logic with the assumption that things like "is this focusable", "is this text-editable", etc, are immutable properties of a widget and don't need to be re-checked after creation.
`WidgetState::merge_up` no longer actually mutates `child_state` since linebender#488 and linebender#599, but it may do so again in the future. See also this Zulip thread: https://xi.zulipchat.com/#narrow/stream/317477-masonry/topic/WidgetState.3A.3Amerge_up.20no.20longer.20needs.20mutable.20child.20state
`WidgetState::merge_up` no longer actually mutates `child_state` since #488 and #599, but it may do so again in the future. See also this Zulip thread: https://xi.zulipchat.com/#narrow/stream/317477-masonry/topic/WidgetState.3A.3Amerge_up.20no.20longer.20needs.20mutable.20child.20state
Should fix the way repaints were requested every paint on some platforms.
This PR unfortunately comes with a lack of tests. TestHarness currently doesn't have good support for testing signals, but even if it did, this is an inherently visual feature that we don't really have a way to test visually.
I don't know how to trigger IME on my machine. (I'm using PopOS, a Debian variant.) If someone wants to either test this or help me get to speed, I'd appreciate it.