(#2777) Tests for input-method-v1 #302
Conversation
There was a problem hiding this comment.
This looks pretty good all up.
The two pervasive issues are cross-client synchronisation (which should probably use dispatch_until on the receiving end), and serial handling for update_state. The latter is important because a compositor could reasonably discard any update_state requests with an incorrect serial - after all, they're attempting to update no-longer-valid state.
| struct TextInputV2WithInputMethodV1Test : wlcs::StartedInProcessServer | ||
| { | ||
| TextInputV2WithInputMethodV1Test() | ||
| : StartedInProcessServer{}, |
There was a problem hiding this comment.
We have traditionally not explicitly called the superclass constructor unless it's necessary (to pass parameters down). It's not wrong, though.
| the_server().move_surface_to(app_surface.value(), 0, 0); | ||
| pointer.move_to(10, 10); | ||
| pointer.left_click(); |
There was a problem hiding this comment.
We really need to make this an explicit interface of the WLCS integration 😉
(That's not something to be here, to be clear, just pointing this out to remind everyone including myself)
| { | ||
| create_focused_surface(); | ||
| zwp_text_input_v2_enable(text_input, app_surface->wl_surface()); | ||
| zwp_text_input_v2_update_state(text_input, 0, 0); |
There was a problem hiding this comment.
It would be nice to use the symbolic update_state reason here.
Also, I think this is wrong? As far as I can tell:
- You don't need to send an
update_statein order forenableto be processed - the protocol is unclear, but it doesn't sayenableis double-buffered state committed byupdate_state, and it's not important that it be associated with a particular batch of changes - The serial here should be from an
enter(orinput_method_changed) event. We should have got one of those as a part of the surface being focused.
There was a problem hiding this comment.
- In our
text-input-v2implementation, that request is left "pending" until anupdate_statecomes through, which would make it such that theinput-method-v1never gets activated. Perhaps our implementation is wrong? It could be... - Agreed!
| zwp_text_input_v2_enable(text_input, app_surface->wl_surface()); | ||
| zwp_text_input_v2_update_state(text_input, 0, 0); | ||
| app_client.roundtrip(); | ||
| input_client.roundtrip(); |
There was a problem hiding this comment.
These cross-client synchronisation requirements are hard; it's possible this is racy (I don't think it is in Mir, but I don't believe the protocol guarantees it isn't).
Probably you want to do app_client.roundtrip() and then input_client.dispatch_until(...) for the input client.
There was a problem hiding this comment.
I think I did this correctly now 🤔 Let me know!
| app_client.roundtrip(); | ||
| input_client.roundtrip(); | ||
| zwp_text_input_v2_disable(text_input, app_surface->wl_surface()); | ||
| zwp_text_input_v2_update_state(text_input, 1, 0); |
There was a problem hiding this comment.
Again, I'm reasonably confident that enable/disable does not require committing with update_state.
| const size_t data_length = 33; | ||
| wl_array map; | ||
| wl_array_init(&map); | ||
| const char** data = static_cast<const char**>(wl_array_add(&map, sizeof(char) * data_length)); |
There was a problem hiding this comment.
Style nit: You can probably declare this as auto data, as you're explicitly casting on the rhs?
input-method-v1
What's new?