[ozone/wayland] Text-input-v3: Deletion of surrounding text [chromium/src : main]
Orko Garai (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Kramer Ge
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Orko Garai added 1 comment![]( "Open in Gerrit")
Noticing an issue when testing with the on-screen keyboard. Will fix and then mark it ready for review again.
Related details
- Kramer Ge
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Orko Garai added 1 comment![]( "Open in Gerrit")
https://crrev.com/c/6542426 revealed a bug in gnome OSK: https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/8426
So had to test on gnome by manually adding another enable+commit to trigger it locally.
Related details
- Kramer Ge
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 4 comments![]( "Open in Gerrit")
struct SendData {
bool operator==(const SendData&) const = default;
std::string text;
int32_t cursor = 0;
int32_t anchor = 0;
};IMO not necessary to wrap this in a struct, we can just comment separate the unsent fields like `commit_count`;
// passed via ZwpTextInputV3::SetSurroundingText.Add a comment that it includes `preedit`?
if (index < 0) {I feel that this chain of if's can be simplified. like using `= ( )? : ` and `DVLOG_IF()`.
surrounding_text->delete_around_range =should this include the `preedit`, or incoming `preedit` instead of (index, 0)?
Related details
- Orko Garai
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Orko Garai added 4 comments![]( "Open in Gerrit")
struct SendData {
bool operator==(const SendData&) const = default;
std::string text;
int32_t cursor = 0;
int32_t anchor = 0;
};IMO not necessary to wrap this in a struct, we can just comment separate the unsent fields like `commit_count`;
Done.
Add a comment that it includes `preedit`?
Done
I feel that this chain of if's can be simplified. like using `= ( )? : ` and `DVLOG_IF()`.
Done
should this include the `preedit`, or incoming `preedit` instead of (index, 0)?
Great catch!
I think it shouldn't include it when there is no incoming preedit, as the preedit part will be removed in that case.
But when there is an incoming preedit, you're right it makes sense to include that. I didn't do that initially as I thought the double deletion is probably very unlikely anyway with the most likely scenario being `delete, done, delete, done, set_surrounding_text`. But you're right in theory there could be something like `preedit, delete, done, delete, done, set_surrounding_text`. So I've addressed that now. Thanks!
Related details
- Kramer Ge
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge voted and added 3 comments![]( "Open in Gerrit")
Votes added by Kramer Ge
| Code-Review | +1 |
3 comments
const auto preedit_data = self->pending_input_events_.preedit;nit: should this also be `const auto&`?
surrounding_text->delete_around_range =Orko Garaishould this include the `preedit`, or incoming `preedit` instead of (index, 0)?
Great catch!
I think it shouldn't include it when there is no incoming preedit, as the preedit part will be removed in that case.
But when there is an incoming preedit, you're right it makes sense to include that. I didn't do that initially as I thought the double deletion is probably very unlikely anyway with the most likely scenario being `delete, done, delete, done, set_surrounding_text`. But you're right in theory there could be something like `preedit, delete, done, delete, done, set_surrounding_text`. So I've addressed that now. Thanks!
To be frank I also think a sane compositor would perform 2 `done`s for each of their operations. Thanks for fixing.
Related details
- Orko Garai
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Orko Garai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Orko Garai
| Commit-Queue | +2 |
1 comment
const auto preedit_data = self->pending_input_events_.preedit;nit: should this also be `const auto&`?
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ui/ozone/platform/wayland/host/zwp_text_input_v3.cc
Insertions: 1, Deletions: 1.
@@ -407,7 +407,7 @@
}
auto& surrounding_text = self->committed_ime_data_.surrounding_text;
const auto& commit_string = self->pending_input_events_.commit;
- const auto preedit_data = self->pending_input_events_.preedit;
+ const auto& preedit_data = self->pending_input_events_.preedit;
const auto& delete_surrounding_text =
self->pending_input_events_.delete_surrounding_text;
if (surrounding_text && delete_surrounding_text &&
```
Change information
[ozone/wayland] Text-input-v3: Deletion of surrounding text
Implement text-input-v3 `delete_surrounding_text` event.
- M ui/ozone/platform/wayland/host/wayland_input_method_context.cc
- M ui/ozone/platform/wayland/host/zwp_text_input_v1.h
- M ui/ozone/platform/wayland/host/zwp_text_input_v3.cc
- M ui/ozone/platform/wayland/host/zwp_text_input_v3.h
- M ui/ozone/platform/wayland/host/zwp_text_input_v3_unittest.cc
- M ui/ozone/platform/wayland/test/mock_zwp_text_input_v3_client.h
Code-Review: +1 by Kramer Ge
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |