IME Selected String #2056

kumattau · 2022-05-30T15:35:03Z

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.

wez · 2022-05-30T16:20:46Z

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.

kumattau · 2022-05-31T13:14:22Z

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.

kumattau · 2022-06-05T15:08:47Z

@wez
I finished to implement it. This PR is ready for review.

Note:
In this PR, kumattau/xcb-imdkit-rs@9ba1522 has been used yet.

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?

H-M-H · 2022-06-08T02:05:54Z

@kumattau I've merged wez's and your changes now, thanks!

kumattau · 2022-06-08T03:59:59Z

@H-M-H Thank you for merge.
@wez I will update the Cargo.toml to a new rust-xcb release after released.

wez

Thank you for this!
Sorry it has taken me a while to get around to reviewing it!
I have some comments and suggestions!

wez · 2022-06-16T20:50:09Z

window/src/os/windows/window.rs

-                )
-            };
-            OsString::from_wide(&wide_buf).into_string()
+    pub fn get_raw<T: Clone>(&self, which: DWORD) -> Result<Vec<T>, i32> {


The generics here slow down compilation, and AFAICT, we only ever use u16 anyway, so can we simplify this?

Suggested change

pub fn get_raw<T: Clone>(&self, which: DWORD) -> Result<Vec<T>, i32> {

pub fn get_raw(&self, which: DWORD) -> Result<Vec<u16>, i32> {

@wez

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 ?

https://github.com/kumattau/wezterm/blob/9e64e4d194b27300e071c090dd495582bb372b05/window/src/os/windows/window.rs#L1853-L1855

@wez
Can you reply to my above comment?

in that case, add a comment that indicates that T is u8 or u16 and that will help clarify. Thanks!

wez · 2022-06-16T20:51:23Z

window/src/os/x11/connection.rs

+    }
+
+    let mut char_indices = text.char_indices();
+    let s_start = std::cmp::min(char_indices.nth(a_start).unwrap().0, max);


I generally prefer to use the .min() or .max() methods as they result in less code to read:

Suggested change

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);

wez · 2022-06-16T20:54:12Z

window/src/lib.rs

@@ -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>>),


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:

Suggested change

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)

wez · 2022-06-16T20:55:40Z

wezterm-gui/src/termwindow/render.rs

+        let mut selected_start = 0;
+        let mut selected_width = 0;


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.

wez · 2022-06-16T21:00:41Z

window/src/os/macos/window.rs

@@ -2870,3 +2880,48 @@ fn resolve_geom(geometry: RequestedWindowGeometry) -> ResolvedGeometry {

    ResolvedGeometry { pos, width, height }
 }
+
+fn calc_str_selected_range(


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.

wez · 2022-06-16T21:04:21Z

window/src/os/x11/connection.rs

 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;


is this something that should ultimately end up in xcb-imdkit?
Could you put a comment here to indicate that?

I send PR to xcb-imdkit to define the constant.

H-M-H/xcb-imdkit-rs#3

kumattau · 2022-06-26T15:42:11Z

@wez thank you for the review and sorry for late reply.
My updates will take some times because I'm rust beginner.

swnakamura · 2023-07-22T16:54:01Z

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.

wez · 2024-01-23T13:36:03Z

@swnakamura would you care to rebase and re-submit this PR?

wez · 2024-01-23T13:36:46Z

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

wez · 2024-01-24T00:54:11Z

I recall that this was blocked on H-M-H/xcb-imdkit-rs#3
I've forked that repo (it's not been actively maintained in some time) and merged that PR into my fork:
https://github.com/wez/xcb-imdkit-rs

so hopefully this PR can now be fairly simply and easily rebased to bring it up to date.

swnakamura · 2024-01-24T04:35:27Z

OK! I'll take a look into the code base later.
But please be noted that what I did is just an ad hoc patch to make it work for macOS and I know little about xcb thing.
@kumattau If you're still willing to work on this, I'd be happy to wait for you. I'm also happy to test if your patch works on macOS!

kumattau · 2024-01-24T17:28:47Z

Sorry for the late.
I will begin this work in February.

wez · 2024-01-24T17:40:04Z

@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 main.

swnakamura · 2024-04-26T03:04:21Z

Hi @kumattau, how is it going?
If you are busy with other stuff, I'm happy to help by writing drafts for rebasing and the patch for macOS!

kumattau · 2024-04-27T05:36:08Z

@swnakamura

Sorry for the wait.
I am currently in the process of rebasing to main.
I only have the X11 Linux environment ready, but I will check the X11 Linux environment and then push the new code.

Maybe I will ask you to implement and test on macos.

kumattau · 2024-05-06T23:51:49Z

TO: @wez CC: @swnakamura

I have finished to update. Could you check again and merge it if okay ?

NOTE:
This does not support wayland because I cannot find the selected range in text input protocol API.

swnakamura · 2024-05-07T02:12:14Z

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.
The implementation uses selection_bg for the color of IME composing text. However, this color is not always well configured and barely visible for some color schemes like rose-pine-dawn.

This can be fixed by manually setting selection_bg in .wezterm_lua.
For example, I fix this by doing below.
https://github.com/swnakamura/dotfiles/blob/c62be1671c9ff7624125914924fb6927353a9e62/.wezterm.lua#L18-L28

However, as English is my secondary language, I may not be the best person to write the explanation for this fix in the documentation.
If @wez or somebody else could write some notes about this issue on the documentation, newcomers to wezterm would appreciate that. I'm happy to write a draft for the note if you ask.

kumattau · 2024-05-07T13:34:02Z

The implementation uses selection_bg for the color of IME composing text. However, this color is not always well configured and barely visible for some color schemes like rose-pine-dawn.

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 ?

swnakamura · 2024-05-07T14:46:12Z

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.
The correct solution would be to fix the color scheme so that the color of selection_bg is distinct. However, I guess that there are other builtin color schemes that have the same problem, and it would be so much of work to fix ALL of them and out of the scope 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 selection_bg.
I presume colors & appearances section would be suitable. Like this documentation below:

wezterm/docs/config/appearance.md

Lines 109 to 113 in 0b50725

    
           -- Since: 20220319-142410-0fcdea07 
        
           -- When the IME, a dead key or a leader key are being processed and are effectively 
        
           -- holding input pending the result of input composition, change the cursor 
        
           -- to this color to give a visual cue about the compose state. 
        
           compose_cursor = 'orange',

kumattau · 2024-05-07T16:57:03Z

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.

swnakamura · 2024-05-08T00:03:48Z

About the color of selected composing text, I think new configuration (e.g. compose_selected_cursor) should be added same as compose_cursor

I agree with you. Having a separate highlight group for the "selected" text in the composing text (in addition to the selection_bg or compose_cursor group we currently have) would be the best option. compose_selected_cursor sounds to be a good name.

kumattau · 2024-05-08T13:31:23Z

@wez

I have finished to update. Could you check again and merge it if okay ?

Sorry, I found some small issues. And I will add new configuration for IME selected string. So I back PR to draft.

kumattau force-pushed the ime-selected-string branch 4 times, most recently from 13eef3d to 9e64e4d Compare June 5, 2022 14:15

kumattau marked this pull request as ready for review June 5, 2022 15:02

wez requested changes Jun 16, 2022

View reviewed changes

kumattau marked this pull request as draft October 9, 2022 16:06

swnakamura added a commit to swnakamura/wezterm that referenced this pull request Jul 22, 2023

Rebase wez#2056 to main

dae8ef1

swnakamura added a commit to swnakamura/wezterm that referenced this pull request Jul 23, 2023

Rebase wez#2056 to main

81bb42f

kumattau force-pushed the ime-selected-string branch 3 times, most recently from caad1d6 to af312b6 Compare April 28, 2024 04:13

kumattau force-pushed the ime-selected-string branch 2 times, most recently from 964d640 to 00edfee Compare May 6, 2024 18:32

Render IME selected string in different color from other preedit string

1163d17

kumattau force-pushed the ime-selected-string branch from 00edfee to 1163d17 Compare May 6, 2024 18:42

kumattau marked this pull request as ready for review May 6, 2024 23:38

kumattau force-pushed the ime-selected-string branch 2 times, most recently from 8c18c67 to 58b2fa6 Compare May 7, 2024 23:46

Reduce unicode_column_width call

07ec769

kumattau force-pushed the ime-selected-string branch from 58b2fa6 to 07ec769 Compare May 7, 2024 23:53

kumattau marked this pull request as draft May 8, 2024 13:31

kumattau mentioned this pull request May 11, 2024

Fix panic if feedback_array.size == 0 wez/xcb-imdkit-rs#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IME Selected String #2056

IME Selected String #2056

kumattau commented May 30, 2022 •

edited

Loading

wez commented May 30, 2022

kumattau commented May 31, 2022

kumattau commented Jun 5, 2022

H-M-H commented Jun 8, 2022

kumattau commented Jun 8, 2022

wez left a comment

wez Jun 16, 2022

kumattau Oct 3, 2022 •

edited

Loading

kumattau Oct 19, 2022

wez Oct 22, 2022

wez Jun 16, 2022

wez Jun 16, 2022

wez Jun 16, 2022

wez Jun 16, 2022

wez Jun 16, 2022

kumattau Oct 3, 2022

kumattau commented Jun 26, 2022

swnakamura commented Jul 22, 2023 •

edited

Loading

wez commented Jan 23, 2024

wez commented Jan 23, 2024

wez commented Jan 24, 2024

swnakamura commented Jan 24, 2024

kumattau commented Jan 24, 2024

wez commented Jan 24, 2024

swnakamura commented Apr 26, 2024

kumattau commented Apr 27, 2024 •

edited

Loading

kumattau commented May 6, 2024

swnakamura commented May 7, 2024 •

edited

Loading

kumattau commented May 7, 2024

swnakamura commented May 7, 2024 •

edited

Loading

kumattau commented May 7, 2024 •

edited

Loading

swnakamura commented May 8, 2024

kumattau commented May 8, 2024

	pub fn get_raw<T: Clone>(&self, which: DWORD) -> Result<Vec<T>, i32> {
	pub fn get_raw(&self, which: DWORD) -> Result<Vec<u16>, i32> {

	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);

-    Composing(String, Option<Range<usize>>),
+    Composing {
+       composition: String,
+       selection: Option<Range<usize>>
+    },

IME Selected String #2056

Are you sure you want to change the base?

IME Selected String #2056

Conversation

kumattau commented May 30, 2022 • edited Loading

wez commented May 30, 2022

kumattau commented May 31, 2022

kumattau commented Jun 5, 2022

H-M-H commented Jun 8, 2022

kumattau commented Jun 8, 2022

wez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumattau Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumattau commented Jun 26, 2022

swnakamura commented Jul 22, 2023 • edited Loading

wez commented Jan 23, 2024

wez commented Jan 23, 2024

wez commented Jan 24, 2024

swnakamura commented Jan 24, 2024

kumattau commented Jan 24, 2024

wez commented Jan 24, 2024

swnakamura commented Apr 26, 2024

kumattau commented Apr 27, 2024 • edited Loading

kumattau commented May 6, 2024

swnakamura commented May 7, 2024 • edited Loading

kumattau commented May 7, 2024

swnakamura commented May 7, 2024 • edited Loading

kumattau commented May 7, 2024 • edited Loading

swnakamura commented May 8, 2024

kumattau commented May 8, 2024

kumattau commented May 30, 2022 •

edited

Loading

kumattau Oct 3, 2022 •

edited

Loading

swnakamura commented Jul 22, 2023 •

edited

Loading

kumattau commented Apr 27, 2024 •

edited

Loading

swnakamura commented May 7, 2024 •

edited

Loading

swnakamura commented May 7, 2024 •

edited

Loading

kumattau commented May 7, 2024 •

edited

Loading