-
-
Notifications
You must be signed in to change notification settings - Fork 770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IME Selected String #2056
base: main
Are you sure you want to change the base?
IME Selected String #2056
Conversation
my fork was pending some changes to xcb that have subsequently been landed, so we should be able to get H-M-H/xcb-imdkit-rs#2 merged now. I would suggest making your imdkit PR against its master branch to get that in-progress, then your PR can point to your fork of my imdkit + your change once you're ready for review. We can reconcile and re-point to the appropriate upstreams as they land. |
Thank you for advice. I will send the PR kumattau/xcb-imdkit-rs@9ba1522 to upstream when H-M-H/xcb-imdkit-rs#2 is merged and this PR #2056 is ready for merge. |
13eef3d
to
9e64e4d
Compare
@wez Note: H-M-H/xcb-imdkit-rs#2 has not been merged yet, so I can also send kumattau/xcb-imdkit-rs@9ba1522 to the https://github.com/wez/xcb-imdkit-rs. What should I do? |
@kumattau I've merged wez's and your changes now, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this!
Sorry it has taken me a while to get around to reviewing it!
I have some comments and suggestions!
) | ||
}; | ||
OsString::from_wide(&wide_buf).into_string() | ||
pub fn get_raw<T: Clone>(&self, which: DWORD) -> Result<Vec<T>, i32> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The generics here slow down compilation, and AFAICT, we only ever use u16
anyway, so can we simplify this?
pub fn get_raw<T: Clone>(&self, which: DWORD) -> Result<Vec<T>, i32> { | |
pub fn get_raw(&self, which: DWORD) -> Result<Vec<u16>, i32> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The get_raw is used as get_raw<u16>
and get_raw<u8>
. Should we implement 2 special (non generic) functions like as get_raw_u8 and get_raw_u16 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wez
Can you reply to my above comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that case, add a comment that indicates that T
is u8
or u16
and that will help clarify. Thanks!
window/src/os/x11/connection.rs
Outdated
} | ||
|
||
let mut char_indices = text.char_indices(); | ||
let s_start = std::cmp::min(char_indices.nth(a_start).unwrap().0, max); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally prefer to use the .min()
or .max()
methods as they result in less code to read:
let s_start = std::cmp::min(char_indices.nth(a_start).unwrap().0, max); | |
let s_start = char_indices.nth(a_start).unwrap().0.min(max); |
window/src/lib.rs
Outdated
@@ -131,7 +132,7 @@ pub enum DeadKeyStatus { | |||
None, | |||
/// Holding until composition is done; the string is the uncommitted | |||
/// composition text to show as a placeholder | |||
Composing(String), | |||
Composing(String, Option<Range<usize>>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tuple structs can make it hard to intuit the purpose of the different elements, especially for optional elements where we may tend to end up with None
.
Please change this to a struct so that the code where these are used becomes more self-descriptive:
Composing(String, Option<Range<usize>>), | |
Composing { | |
composition: String, | |
selection: Option<Range<usize>> | |
}, |
If it makes some things easier, it would also be fine to break that struct out as a separate DeadKeyComposition
struct and then this variant could be Composing(DeadKeyComposition)
wezterm-gui/src/termwindow/render.rs
Outdated
let mut selected_start = 0; | ||
let mut selected_width = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be turned into a Range? Would that make the rest of the code in this function clearer and easier to follow?
Rationale: having two separate variables feels more prone to having one be mutated without considering the other, and thus may be more prone to bugs in the future.
window/src/os/macos/window.rs
Outdated
@@ -2870,3 +2880,48 @@ fn resolve_geom(geometry: RequestedWindowGeometry) -> ResolvedGeometry { | |||
|
|||
ResolvedGeometry { pos, width, height } | |||
} | |||
|
|||
fn calc_str_selected_range( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variations on this function are not easy to grasp on a quick scan of the code.
Could you do something to help readability?
Having a doc comment block to explain its purpose and the parameters would help.
I think max
as a variable name may not be descriptive enough, as it is not immediately clear what it is the maximum of.
window/src/os/x11/connection.rs
Outdated
use std::os::unix::io::AsRawFd; | ||
use std::rc::Rc; | ||
use std::sync::{Arc, Mutex}; | ||
use xcb::x::Atom; | ||
|
||
#[allow(non_upper_case_globals)] | ||
const XIMReverse: u32 = 0x1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this something that should ultimately end up in xcb-imdkit?
Could you put a comment here to indicate that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I send PR to xcb-imdkit to define the constant.
@wez thank you for the review and sorry for late reply. |
Wow, this IME coloring feature is what I've yearned for! I am able to enjoy comfortable Japanese input in macOS by copying&pasting this and rebasing to the latest main. I'd be happy if this PR is merged. |
@swnakamura would you care to rebase and re-submit this PR? |
or @kumattau of course! :) |
My branch includes my own fixes, as well as this PR: refs: H-M-H/xcb-imdkit-rs#3 which should enable this PR that's been blocked on it for a while: refs: #2056
I recall that this was blocked on H-M-H/xcb-imdkit-rs#3 so hopefully this PR can now be fairly simply and easily rebased to bring it up to date. |
OK! I'll take a look into the code base later. |
Sorry for the late. |
@kumattau please take your time; there's no time pressure for you to do this work in any time frame, or even do it at all. I was just curious to see if you were still interested in finishing it because I think everyone involved got busy for a long time and it slipped out of mind for everyone. Hopefully the remaining work is now much simpler because the imdkit changes are more easily accessible now in |
Hi @kumattau, how is it going? |
Sorry for the wait. Maybe I will ask you to implement and test on macos. |
caad1d6
to
af312b6
Compare
964d640
to
00edfee
Compare
00edfee
to
1163d17
Compare
TO: @wez CC: @swnakamura I have finished to update. Could you check again and merge it if okay ? NOTE: |
I haven't checked the implementation, but it works on macOS perfectly. I'm OK to merge this. Thank you @kumattau very very much!! I think it would be better to have documentation to change the highlight color for the IME composing text. This can be fixed by manually setting However, as English is my secondary language, I may not be the best person to write the explanation for this fix in the documentation. |
I tried "rose-pine-moon" without "selection_bg = 'silver'" but the selected region by pointer was not visible either same as selected composing text. Is this builtin scheme problem ? |
Yes, I think you are right. This is rather a problem of the builtin color scheme than the problem of the implementation of this PR. Thus it remains possible that somebody wants to use one of such color schemes and wants to change the composing text string color. For that case, it would be worth noting somewhere in the doc that the compose text color can be changed through wezterm/docs/config/appearance.md Lines 109 to 113 in 0b50725
|
About the color of selected composing text, I think new configuration (e.g. compose_selected_cursor) should be added same as compose_cursor, and for correcting the color appropriately, resolving the color should be integrated to the resolver of other colors such as cursor_border_color and cursor_border_color_alt. But currently I don't know how defficult it would be to implement this feature. I added a commit to reduce unicode_column_width call. |
8c18c67
to
58b2fa6
Compare
58b2fa6
to
07ec769
Compare
I agree with you. Having a separate highlight group for the "selected" text in the composing text (in addition to the |
Sorry, I found some small issues. And I will add new configuration for IME selected string. So I back PR to draft. |
Hi, @wez
Japanese IME needs that the current converting string has different visual from the whole preedit string because Japanese IME may convert a part of whole preedit string.
For example, the current converting string is the purple part in the following screenshot, which is the result of my converting string implementation work:
I am implementing it for Windows, macOS and X11, but the implementation for X11 requires the exposure of more PreeditInfo's feedback_array member in xcb-imdkit-rs like as follows:
kumattau/xcb-imdkit-rs@9ba1522
The current wezterm seems to use self-hosted xcb-imdkit-rs.
Where is it better I send the request, wezterm or upstream ?
This PR is draft. The code reviews has not been needed yet.