IME: add surrounding text reporting to text view#1028
IME: add surrounding text reporting to text view#1028dcz-self wants to merge 3 commits intolapce:mainfrom
Conversation
dcz-self
force-pushed
the
actions
branch
2 times, most recently
from
436522f to
d4e5d12
Compare
|
I'm not sure what to do with Should I reimplement it as a trait? A function? |
dcz-self
force-pushed
the
actions
branch
2 times, most recently
from
186f1dd to
340e762
Compare
jrmoulton
left a comment
jrmoulton
left a comment
There was a problem hiding this comment.
The ime code generally looks good to me. The biggest thing here is I don't want to be duplicating the widget gallery for this ime example. any change (which I can't tell what they are if there are any) can just be made to the widget gallery directly. @ArthurCose if you have time could you look this over?
Cargo.toml
Outdated
|
|
||
| [patch.crates-io] | ||
| #sctk = { git = "https://github.com/dcz-self/client-toolkit.git", branch = "sync2", package = "smithay-client-toolkit", version = "0.20.0" } | ||
| smithay-client-toolkit = { git = "https://github.com/Smithay/client-toolkit" } No newline at end of file |
There was a problem hiding this comment.
this patch needs removed
|
I'm fine bumping the rust version |
|
Oops, I pushed my experimental branch rather than the backported patch. I'll sort it out by tomorrow. |
dcz-self
force-pushed
the
actions
branch
2 times, most recently
from
bbe123e to
22ceda9
Compare
|
Done, and bumped the version, but clippy is unhappy now. Should I apply the suggestions in a separate MR? |
Needed for UTF code point navigation in lapce#1028
Needed for UTF code point navigation in lapce#1028
Needed for UTF code point navigation in lapce#1028
|
Well, I did it anyway in #1031 |
Needed for UTF code point navigation in #1028 Co-authored-by: dcz <gilapfco.dcz@porcupinefactory.org>
jrmoulton
left a comment
jrmoulton
left a comment
There was a problem hiding this comment.
Overall this looks good to me but I think there is a bug on line 305
| (cursor_end.saturating_sub(maxlen), cursor_end) | ||
| } else { | ||
| let cursor_end = cursor.saturating_sub(SURROUNDING_BYTES); | ||
| (cursor_end, cmp::max(cursor_end + maxlen, text.len())) |
There was a problem hiding this comment.
| (cursor_end, cmp::max(cursor_end + maxlen, text.len())) | |
| (cursor_end, cmp::min(cursor_end + maxlen, text.len())) |
There was a problem hiding this comment.
I think this should be min right?
There was a problem hiding this comment.
Yep, nice catch. Maybe I should add some unit tests before I resubmit.
There was a problem hiding this comment.
that would be great. and some docs (even if pretty minimal describing surrounding text would be great)
2 participants
I didn't touch the editor widget because I already failed to work with it once. Help appreciated.
Because the editor still doesn't support setting surrounding text, I had to add the capability conditionally when requesting IME, so
set_ime_allowednow needs more ceremony to set up, and so I move it toView::update.In the end it's for the best, because without sending all state updates at once, the input method experience is a blinkfest. So it's better to set up everything when it's ready.