[ozone/wayland] text-input-v3 Part 9: Set surrounding text [chromium/src : main]
0 views
Skip to first unread message
Orko Garai (Gerrit)
Jul 16, 2024, 11:24:15 AM (11 days ago) Jul 16
to Hidehiko Abe, Kramer Ge, Nick Yamane, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Hidehiko Abe and Kramer Ge
New activity on the change![]( "Open in Gerrit")
Related details
Attention is currently required from:
- Hidehiko Abe
- Kramer Ge
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Jul 23, 2024, 3:50:35 AM (4 days ago) Jul 23
to Orko Garai, Kramer Ge, Nick Yamane, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge and Orko Garai
Hidehiko Abe added 5 comments![]( "Open in Gerrit")
File ui/ozone/platform/wayland/host/zwp_text_input_wrapper_v3.cc
Line 238, Patchset 5 (Latest):
// Selection overlaps with preedit. Simply discard selection in that
// case and put cursor at preedit position.Hidehiko Abe . unresolved
hmm... could you elaborate why this is reasonable? should't we just trim the overlapping part?
Line 230, Patchset 5 (Latest):
if (selection_range.IsValid()) {
if (selection_range.end() <= preedit_range.start()) {
// Selection is unaffected because it is to the left of preedit.
} else if (selection_range.start() >= preedit_range.end()) {
// Selection is after preedit, so move selection back
data.cursor -= preedit_range.length();
data.anchor -= preedit_range.length();
} else {
// Selection overlaps with preedit. Simply discard selection in that
// case and put cursor at preedit position.
data.cursor = preedit_range.start();
data.anchor = preedit_range.start();
}
} else {
// Invalid selection range. Put cursor at preedit position.
data.cursor = preedit_range.start();
data.anchor = preedit_range.start();
}Hidehiko Abe . unresolved
could you not assume start <= end ?
File ui/ozone/platform/wayland/host/zwp_text_input_wrapper_v3_unittest.cc
Line 291, Patchset 5 (Latest):
constexpr gfx::Range kPreeditRange(11, 38);
constexpr gfx::Range kSelectionRange = {38, 38};Hidehiko Abe . unresolved
style: coudl you keep using consistent style?
Related details
Attention is currently required from:
- Kramer Ge
- Orko Garai
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Jul 23, 2024, 10:35:57 AM (4 days ago) Jul 23
to Hidehiko Abe, Kramer Ge, Nick Yamane, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Hidehiko Abe and Kramer Ge
Orko Garai added 3 comments![]( "Open in Gerrit")
File ui/ozone/platform/wayland/host/zwp_text_input_wrapper_v3.cc
Line 238, Patchset 5 (Latest):
// Selection overlaps with preedit. Simply discard selection in that
// case and put cursor at preedit position.Hidehiko Abe . unresolved
hmm... could you elaborate why this is reasonable? should't we just trim the overlapping part?
Orko Garai
We could do that as well if that is preferred. Was not sure if this was indeed something that could actually occur and so defaulted to this type of fallback.
Line 240, Patchset 5 (Latest):
data.cursor = preedit_range.start();Hidehiko Abe . unresolved
end
Orko Garai
Can't be end because the preedit is not included in surrounding text that is sent. It's as if the entire preedit is collapsed at the cursor position.
File ui/ozone/platform/wayland/host/zwp_text_input_wrapper_v3_unittest.cc
Line 291, Patchset 5 (Latest):
constexpr gfx::Range kPreeditRange(11, 38);
constexpr gfx::Range kSelectionRange = {38, 38};Hidehiko Abe . unresolved
style: coudl you keep using consistent style?
Orko Garai
yup, missed this after copying from another test and renaming from regular variable to constexpr for this test
Related details
Attention is currently required from:
- Hidehiko Abe
- Kramer Ge
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Jul 24, 2024, 3:57:22 PM (3 days ago) Jul 24
to Hidehiko Abe, Kramer Ge, Nick Yamane, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Hidehiko Abe and Kramer Ge
Orko Garai added 4 comments![]( "Open in Gerrit")
File ui/ozone/platform/wayland/host/zwp_text_input_wrapper_v3.cc
Line 238, Patchset 5:
// Selection overlaps with preedit. Simply discard selection in that
// case and put cursor at preedit position.Hidehiko Abe . resolved
Orko Garaihmm... could you elaborate why this is reasonable? should't we just trim the overlapping part?
We could do that as well if that is preferred. Was not sure if this was indeed something that could actually occur and so defaulted to this type of fallback.
Orko Garai
Done
Line 240, Patchset 5:
data.cursor = preedit_range.start();Hidehiko Abe . unresolved
Orko Garaiend
Can't be end because the preedit is not included in surrounding text that is sent. It's as if the entire preedit is collapsed at the cursor position.
Orko Garai
please see latest patch with additional comments.
Line 230, Patchset 5:
if (selection_range.IsValid()) {
if (selection_range.end() <= preedit_range.start()) {
// Selection is unaffected because it is to the left of preedit.
} else if (selection_range.start() >= preedit_range.end()) {
// Selection is after preedit, so move selection back
data.cursor -= preedit_range.length();
data.anchor -= preedit_range.length();
} else {
// Selection overlaps with preedit. Simply discard selection in that
// case and put cursor at preedit position.
data.cursor = preedit_range.start();
data.anchor = preedit_range.start();
}
} else {
// Invalid selection range. Put cursor at preedit position.
data.cursor = preedit_range.start();
data.anchor = preedit_range.start();
}Hidehiko Abe . unresolved
Orko Garaicould you not assume start <= end ?
see latest patch. For selection start > end is possible.
File ui/ozone/platform/wayland/host/zwp_text_input_wrapper_v3_unittest.cc
Line 291, Patchset 5:
constexpr gfx::Range kPreeditRange(11, 38);
constexpr gfx::Range kSelectionRange = {38, 38};Hidehiko Abe . resolved
Orko Garaistyle: coudl you keep using consistent style?
yup, missed this after copying from another test and renaming from regular variable to constexpr for this test
Orko Garai
Done
Related details
Attention is currently required from:
- Hidehiko Abe
- Kramer Ge
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Jul 26, 2024, 11:31:58 AM (21 hours ago) Jul 26
to Hidehiko Abe, Kramer Ge, Nick Yamane, chromium...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Hidehiko Abe and Kramer Ge
Orko Garai added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Hidehiko Abe
- Kramer Ge
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages