[PATCH v4 0/5] MR7812: win32u: Add more CompAttr, CompClause implementation using cursor_begin, cursor_end concept.
If CompAttr and CompClause are properly configured, Japanese input will be more comfortable.
Inspired by cursor_begin and cursor_end from Wayland zwp_text_input_v3:: preedit_string, I extended the cursor_pos concept as follows:
cursor_pos = MAKELONG( cursor_begin, cursor_end );
ime_to_tascii_ex() uses this to construct CompAttr, CompClause. MS Windows native CompAttr, CompClause is a bit more complicated than this, but the concept is useful enough.
It requires additional implementation in the Wine ime_ui_window proc and richedit control. However, it is useful for applications that inline ime composition string.
This can be tested with MS Office Word, Excel. LANG=ja_JP.UTF-8 wine EXCEL.EXE
Test key sequences: - “n-i-h-o-n-g-o-n-o-m-o-j-i-d-e-s-u-.-SPACE”. - And, RIGHT, LEFT, Shift+LEFT, Shift+RIGHT, ESC, SPACE, etc.
-- v4: winex11: Update only when caret pos changed in xic_preedit_caret. winex11: Extend cursor_pos using cursor_begin, cursor_end. winemac: Extend cursor_pos using cursor_begin, cursor_end. winewayland: Extend cursor_pos using cursor_begin, cursor_end. win32u: Add more CompAttr, CompClause implementation using cursor_begin, cursor_end concept.
From: Byeongsik Jeon bsjeon@hanmail.net
If CompAttr and CompClause are properly configured, Japanese input will be more comfortable.
Inspired by cursor_begin and cursor_end from Wayland zwp_text_input_v3:: preedit_string, I extended the cursor_pos concept as follows:
cursor_pos = MAKELONG( cursor_begin, cursor_end );
ime_to_tascii_ex() uses this to construct Compttr, CompClause. MS Windows native CompStrAttr, CompStrClause is a bit more complicated than this, but the concept is useful enough.
It requires additional implementation in the Wine ime_ui_window proc and richedit control. However, it is useful for applications that inline ime composition string.
This can be tested with MS Office Word, Excel. LANG=ja_JP.UTF-8 wine EXCEL.EXE
Test key sequences: - 'n-i-h-o-n-g-o-n-o-m-o-j-i-d-e-s-u-.-SPACE'. - And, RIGHT, LEFT, Shift+LEFT, Shift+RIGHT, ESC, SPACE, UP, DOWN, etc. --- dlls/win32u/imm.c | 70 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 61 insertions(+), 9 deletions(-)
diff --git a/dlls/win32u/imm.c b/dlls/win32u/imm.c index c0ed670e779..c413a43e5f2 100644 --- a/dlls/win32u/imm.c +++ b/dlls/win32u/imm.c @@ -445,8 +445,8 @@ static void post_ime_update( HWND hwnd, UINT cursor_pos, WCHAR *comp_str, WCHAR WCHAR *prev_result_str, *tmp; struct ime_update *update;
- TRACE( "hwnd %p, cursor_pos %u, comp_str %s, result_str %s\n", hwnd, cursor_pos, - debugstr_w(comp_str), debugstr_w(result_str) ); + TRACE( "hwnd %p, cursor_pos %u - %u, comp_str %s, result_str %s\n", hwnd, LOWORD(cursor_pos), + HIWORD(cursor_pos), debugstr_w(comp_str), debugstr_w(result_str) );
comp_len = comp_str ? wcslen( comp_str ) + 1 : 0; result_len = result_str ? wcslen( result_str ) + 1 : 0; @@ -495,6 +495,48 @@ static void post_ime_update( HWND hwnd, UINT cursor_pos, WCHAR *comp_str, WCHAR free( tmp ); }
+static UINT get_comp_clause_count( UINT comp_len, INT cursor_begin, INT cursor_end ) +{ + if (cursor_begin == cursor_end || (cursor_begin == 0 && cursor_end == comp_len)) + return 2; + else if (cursor_begin == 0 || cursor_end == comp_len) + return 3; + else + return 4; +} + +static void set_comp_clause( DWORD *comp_clause, UINT comp_clause_count, UINT comp_len, + INT cursor_begin, INT cursor_end ) +{ + comp_clause[0] = 0; + switch (comp_clause_count) + { + case 2: + comp_clause[1] = comp_len; + break; + case 3: + comp_clause[1] = cursor_begin == 0 ? cursor_end : cursor_begin; + comp_clause[2] = comp_len; + break; + case 4: + comp_clause[1] = cursor_begin; + comp_clause[2] = cursor_end; + comp_clause[3] = comp_len; + break; + } +} + +static void set_comp_attr( BYTE *comp_attr, UINT comp_attr_len, INT cursor_begin, INT cursor_end ) +{ + if (cursor_begin == cursor_end) + memset( comp_attr, ATTR_INPUT, comp_attr_len ); + else + { + memset( comp_attr, ATTR_CONVERTED, comp_attr_len ); + memset( comp_attr + cursor_begin, ATTR_TARGET_CONVERTED, cursor_end - cursor_begin ); + } +} + static struct ime_update *find_ime_update( WORD vkey, WORD scan ) { struct ime_update *update; @@ -508,7 +550,8 @@ static struct ime_update *find_ime_update( WORD vkey, WORD scan ) static UINT ime_to_tascii_ex( UINT vkey, UINT lparam, const BYTE *state, COMPOSITIONSTRING *compstr, BOOL *key_consumed, HIMC himc ) { - UINT needed = sizeof(COMPOSITIONSTRING), comp_len, result_len; + UINT needed = sizeof(COMPOSITIONSTRING), comp_len, result_len, comp_clause_count = 0; + INT cursor_begin = -1, cursor_end = -1; struct ime_update *update; void *dst;
@@ -528,9 +571,19 @@ static UINT ime_to_tascii_ex( UINT vkey, UINT lparam, const BYTE *state, COMPOSI else { comp_len = wcslen( update->comp_str ); + cursor_begin = LOWORD(update->cursor_pos); + cursor_end = HIWORD(update->cursor_pos); + + if (cursor_begin > comp_len) cursor_begin = comp_len; + if (cursor_end > comp_len) cursor_end = comp_len; + if (cursor_end < cursor_begin) cursor_end = cursor_begin; + if (cursor_begin < 0 || cursor_end < 0) cursor_begin = cursor_end = -1; /* no cursor */ + + comp_clause_count = get_comp_clause_count( comp_len, cursor_begin, cursor_end ); + needed += comp_len * sizeof(WCHAR); /* GCS_COMPSTR */ needed += comp_len; /* GCS_COMPATTR */ - needed += 2 * sizeof(DWORD); /* GCS_COMPCLAUSE */ + needed += comp_clause_count * sizeof(DWORD); /* GCS_COMPCLAUSE */ }
if (!update->result_str) result_len = 0; @@ -556,7 +609,7 @@ static UINT ime_to_tascii_ex( UINT vkey, UINT lparam, const BYTE *state, COMPOSI
if (update->comp_str) { - compstr->dwCursorPos = update->cursor_pos; + compstr->dwCursorPos = cursor_begin;
compstr->dwCompStrLen = comp_len; compstr->dwCompStrOffset = compstr->dwSize; @@ -564,17 +617,16 @@ static UINT ime_to_tascii_ex( UINT vkey, UINT lparam, const BYTE *state, COMPOSI memcpy( dst, update->comp_str, compstr->dwCompStrLen * sizeof(WCHAR) ); compstr->dwSize += compstr->dwCompStrLen * sizeof(WCHAR);
- compstr->dwCompClauseLen = 2 * sizeof(DWORD); + compstr->dwCompClauseLen = comp_clause_count * sizeof(DWORD); compstr->dwCompClauseOffset = compstr->dwSize; dst = (BYTE *)compstr + compstr->dwCompClauseOffset; - *((DWORD *)dst + 0) = 0; - *((DWORD *)dst + 1) = compstr->dwCompStrLen; + set_comp_clause( dst, comp_clause_count, comp_len, cursor_begin, cursor_end ); compstr->dwSize += compstr->dwCompClauseLen;
compstr->dwCompAttrLen = compstr->dwCompStrLen; compstr->dwCompAttrOffset = compstr->dwSize; dst = (BYTE *)compstr + compstr->dwCompAttrOffset; - memset( dst, ATTR_INPUT, compstr->dwCompAttrLen ); + set_comp_attr( dst, compstr->dwCompAttrLen, cursor_begin, cursor_end ); compstr->dwSize += compstr->dwCompAttrLen; }
From: Byeongsik Jeon bsjeon@hanmail.net
--- dlls/winewayland.drv/wayland_text_input.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/dlls/winewayland.drv/wayland_text_input.c b/dlls/winewayland.drv/wayland_text_input.c index f8df5fe09cb..d27d83d62c4 100644 --- a/dlls/winewayland.drv/wayland_text_input.c +++ b/dlls/winewayland.drv/wayland_text_input.c @@ -100,14 +100,17 @@ static void text_input_preedit_string(void *data, struct zwp_text_input_v3 *zwp_ const char *text, int32_t cursor_begin, int32_t cursor_end) { struct wayland_text_input *text_input = data; - TRACE("data %p, text_input %p, text %s, cursor_begin %d.\n", data, zwp_text_input_v3, - debugstr_a(text), cursor_begin); + TRACE("data %p, text_input %p, text %s, cursor %d - %d.\n", data, zwp_text_input_v3, + debugstr_a(text), cursor_begin, cursor_end);
pthread_mutex_lock(&text_input->mutex); - if ((text_input->preedit_string = strdupUtoW(text)) && cursor_begin > 0) + if ((text_input->preedit_string = strdupUtoW(text))) { - RtlUTF8ToUnicodeN(NULL, 0, &text_input->preedit_cursor_pos, text, cursor_begin); - text_input->preedit_cursor_pos /= sizeof(WCHAR); + DWORD begin = 0, end = 0; + + if (cursor_begin > 0) RtlUTF8ToUnicodeN(NULL, 0, &begin, text, cursor_begin); + if (cursor_end > 0) RtlUTF8ToUnicodeN(NULL, 0, &end, text, cursor_end); + text_input->preedit_cursor_pos = MAKELONG(begin / sizeof(WCHAR), end / sizeof(WCHAR)); } pthread_mutex_unlock(&text_input->mutex); }
From: Byeongsik Jeon bsjeon@hanmail.net
--- dlls/winemac.drv/cocoa_window.m | 3 ++- dlls/winemac.drv/event.c | 4 +++- dlls/winemac.drv/macdrv_cocoa.h | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/dlls/winemac.drv/cocoa_window.m b/dlls/winemac.drv/cocoa_window.m index 4ef9c305b61..71d5c96aa1a 100644 --- a/dlls/winemac.drv/cocoa_window.m +++ b/dlls/winemac.drv/cocoa_window.m @@ -867,7 +867,8 @@ - (void) setMarkedText:(id)string selectedRange:(NSRange)selectedRange replaceme event->im_set_text.himc = [window himc]; event->im_set_text.text = (CFStringRef)[[markedText string] copy]; event->im_set_text.complete = FALSE; - event->im_set_text.cursor_pos = markedTextSelection.location + markedTextSelection.length; + event->im_set_text.cursor_begin = markedTextSelection.location; + event->im_set_text.cursor_end = markedTextSelection.location + markedTextSelection.length;
[[window queue] postEvent:event];
diff --git a/dlls/winemac.drv/event.c b/dlls/winemac.drv/event.c index 20a791ac48a..82bf829d1a0 100644 --- a/dlls/winemac.drv/event.c +++ b/dlls/winemac.drv/event.c @@ -176,7 +176,9 @@ static void macdrv_im_set_text(const macdrv_event *event) }
if (event->im_set_text.complete) post_ime_update(hwnd, -1, NULL, text); - else post_ime_update(hwnd, event->im_set_text.cursor_pos, text, NULL); + else post_ime_update(hwnd, + MAKELONG(event->im_set_text.cursor_begin, event->im_set_text.cursor_end), + text, NULL);
free(text); } diff --git a/dlls/winemac.drv/macdrv_cocoa.h b/dlls/winemac.drv/macdrv_cocoa.h index 917c8103d41..9807d4e53fc 100644 --- a/dlls/winemac.drv/macdrv_cocoa.h +++ b/dlls/winemac.drv/macdrv_cocoa.h @@ -365,7 +365,8 @@ extern int macdrv_set_display_mode(const struct macdrv_display* display, struct { void *himc; CFStringRef text; /* new text or NULL if just completing existing text */ - unsigned int cursor_pos; + unsigned int cursor_begin; + unsigned int cursor_end; unsigned int complete; /* is completing text? */ } im_set_text; struct {
From: Byeongsik Jeon bsjeon@hanmail.net
When tested on ibus, fcitx5, and uim, the applicable bit is XIMReverse. Since these always have chg_first == 0, I referenced the libX11 documentation for applying chg_first. --- dlls/winex11.drv/xim.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/dlls/winex11.drv/xim.c b/dlls/winex11.drv/xim.c index acbe1c3aac1..1b2abb8e92b 100644 --- a/dlls/winex11.drv/xim.c +++ b/dlls/winex11.drv/xim.c @@ -168,6 +168,33 @@ static int xic_preedit_done( XIC xic, XPointer user, XPointer arg ) return 0; }
+static DWORD get_comp_cursor_pos( XIMPreeditDrawCallbackStruct *params ) +{ + int i, cursor_begin = -1, cursor_end = -1; + XIMText *text = params->text; + + if (text && text->feedback) + { + for (i = 0; i < text->length; i++) + { + if (text->feedback[i] & XIMReverse) + { + if (cursor_begin == -1) cursor_begin = i; + cursor_end = i + 1; + } + } + if (cursor_begin != -1) cursor_begin += params->chg_first; + if (cursor_end != -1) cursor_end += params->chg_first; + } + + if (cursor_begin == cursor_end) + cursor_begin = cursor_end = params->caret; /* ATTR_INPUT */ + + TRACE( "caret %d, cursor_begin %d, cursor_end %d\n", params->caret, cursor_begin, cursor_end ); + + return MAKELONG( cursor_begin, cursor_end ); +} + static int xic_preedit_draw( XIC xic, XPointer user, XPointer arg ) { XIMPreeditDrawCallbackStruct *params = (void *)arg; @@ -202,7 +229,7 @@ static int xic_preedit_draw( XIC xic, XPointer user, XPointer arg )
if (text && str != text->string.multi_byte) free( str );
- post_ime_update( hwnd, params->caret, ime_comp_buf, NULL ); + post_ime_update( hwnd, get_comp_cursor_pos( params ), ime_comp_buf, NULL );
return 0; } @@ -248,7 +275,7 @@ static int xic_preedit_caret( XIC xic, XPointer user, XPointer arg ) } params->position = xim_caret_pos = pos;
- post_ime_update( hwnd, pos, ime_comp_buf, NULL ); + post_ime_update( hwnd, MAKELONG( pos, pos ), ime_comp_buf, NULL );
return 0; }
From: Byeongsik Jeon bsjeon@hanmail.net
--- dlls/winex11.drv/xim.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/dlls/winex11.drv/xim.c b/dlls/winex11.drv/xim.c index 1b2abb8e92b..ef09356fbb0 100644 --- a/dlls/winex11.drv/xim.c +++ b/dlls/winex11.drv/xim.c @@ -45,6 +45,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(xim); #endif
static WCHAR *ime_comp_buf; +static DWORD ime_comp_cursor_pos = 0;
static XIMStyle input_style = 0; static XIMStyle input_style_req = XIMPreeditCallbacks | XIMStatusCallbacks; @@ -229,14 +230,14 @@ static int xic_preedit_draw( XIC xic, XPointer user, XPointer arg )
if (text && str != text->string.multi_byte) free( str );
- post_ime_update( hwnd, get_comp_cursor_pos( params ), ime_comp_buf, NULL ); + ime_comp_cursor_pos = get_comp_cursor_pos( params ); + post_ime_update( hwnd, ime_comp_cursor_pos, ime_comp_buf, NULL );
return 0; }
static int xic_preedit_caret( XIC xic, XPointer user, XPointer arg ) { - static int xim_caret_pos; XIMPreeditCaretCallbackStruct *params = (void *)arg; HWND hwnd = (HWND)user; int pos; @@ -245,7 +246,7 @@ static int xic_preedit_caret( XIC xic, XPointer user, XPointer arg )
if (!params) return 0;
- pos = xim_caret_pos; + pos = LOWORD( ime_comp_cursor_pos ); switch (params->direction) { case XIMForwardChar: @@ -273,9 +274,17 @@ static int xic_preedit_caret( XIC xic, XPointer user, XPointer arg ) FIXME( "Not implemented\n" ); break; } - params->position = xim_caret_pos = pos; + params->position = pos;
- post_ime_update( hwnd, MAKELONG( pos, pos ), ime_comp_buf, NULL ); + /* uim implements the preedit_caret callback. This callback is only + valid when the xim is in non-converted state (ATTR_INPUT). + */ + if (LOWORD( ime_comp_cursor_pos ) == HIWORD( ime_comp_cursor_pos ) && + LOWORD( ime_comp_cursor_pos ) != pos) + { + ime_comp_cursor_pos = MAKELONG( pos, pos ); + post_ime_update( hwnd, ime_comp_cursor_pos, ime_comp_buf, NULL ); + }
return 0; }
v4: Fix indention.
This issue can also be tested in the Mery text editor ime inline mode. There is a problem with Japanese completion in ime inline mode, but this CompAttrr, CompClause issue can be tested directly.
https://gitlab.winehq.org/wine/wine/-/merge_requests/7827#note_100929
Rémi Bernon (@rbernon) commented about dlls/win32u/imm.c:
comp_clause[1] = cursor_begin;comp_clause[2] = cursor_end;comp_clause[3] = comp_len;break;- }
+}
+static void set_comp_attr( BYTE *comp_attr, UINT comp_attr_len, INT cursor_begin, INT cursor_end ) +{
- if (cursor_begin == cursor_end)
memset( comp_attr, ATTR_INPUT, comp_attr_len );- else
- {
memset( comp_attr, ATTR_CONVERTED, comp_attr_len );memset( comp_attr + cursor_begin, ATTR_TARGET_CONVERTED, cursor_end - cursor_begin );- }
Do we need to keep ATTR_INPUT? It was being used from the early imm32 versions but it seems that ATTR_CONVERTED is overall a better fit?
Rémi Bernon (@rbernon) commented about dlls/win32u/imm.c:
else { comp_len = wcslen( update->comp_str );
cursor_begin = LOWORD(update->cursor_pos);cursor_end = HIWORD(update->cursor_pos);if (cursor_begin > comp_len) cursor_begin = comp_len;if (cursor_end > comp_len) cursor_end = comp_len;if (cursor_end < cursor_begin) cursor_end = cursor_begin;if (cursor_begin < 0 || cursor_end < 0) cursor_begin = cursor_end = -1; /* no cursor */
Can this ever happen? It doesn't look like the helper functions are handling negative cursor positions, maybe this can simply be dropped. Cursor variables could stay unsigned as well, the check with comp_len will enforce correct values.
Rémi Bernon (@rbernon) commented about dlls/win32u/imm.c:
static UINT ime_to_tascii_ex( UINT vkey, UINT lparam, const BYTE *state, COMPOSITIONSTRING *compstr, BOOL *key_consumed, HIMC himc ) {
- UINT needed = sizeof(COMPOSITIONSTRING), comp_len, result_len;
- UINT needed = sizeof(COMPOSITIONSTRING), comp_len, result_len, comp_clause_count = 0;
- INT cursor_begin = -1, cursor_end = -1;
```suggestion:-0+0 UINT cursor_begin, cursor_end; ```
Rémi Bernon (@rbernon) commented about dlls/win32u/imm.c:
{ comp_len = wcslen( update->comp_str );
cursor_begin = LOWORD(update->cursor_pos);cursor_end = HIWORD(update->cursor_pos);if (cursor_begin > comp_len) cursor_begin = comp_len;if (cursor_end > comp_len) cursor_end = comp_len;if (cursor_end < cursor_begin) cursor_end = cursor_begin;if (cursor_begin < 0 || cursor_end < 0) cursor_begin = cursor_end = -1; /* no cursor */comp_clause_count = get_comp_clause_count( comp_len, cursor_begin, cursor_end );needed += comp_len * sizeof(WCHAR); /* GCS_COMPSTR */ needed += comp_len; /* GCS_COMPATTR */
needed += 2 * sizeof(DWORD); /* GCS_COMPCLAUSE */
needed += comp_clause_count * sizeof(DWORD); /* GCS_COMPCLAUSE */
For simplification, would it be an issue to use 4 all the time and always set [0, cursor_begin, cursor_end, comp_len], regardless if some are equal?
On Wed Apr 16 08:46:42 2025 +0000, Rémi Bernon wrote:
Do we need to keep ATTR_INPUT? It was being used from the early imm32 versions but it seems that ATTR_CONVERTED is overall a better fit?
ATTR_INPUT is the state before pressing “SPACE” on the “nihongo” input. In Romanji, it looks converted, but on a Japanese keyboard, it is typed directly.
This is the state where we can add and delete character while moving the cursor.
On Wed Apr 16 08:46:43 2025 +0000, Rémi Bernon wrote:
For simplification, would it be an issue to use 4 all the time and always set [0, cursor_begin, cursor_end, comp_len], regardless if some are equal?
It might work, but I think it might be problematic for some applications. I'll investigate further to see if this is acceptable.
On Wed Apr 16 08:46:42 2025 +0000, Rémi Bernon wrote:
UINT cursor_begin, cursor_end;
I added it because the logic was fine, but I was getting a weird 'may be used uninitialised' compiler warning. I'll look into it to see if it can be avoided.
On Wed Apr 16 08:46:42 2025 +0000, Rémi Bernon wrote:
Can this ever happen? It doesn't look like the helper functions are handling negative cursor positions, maybe this can simply be dropped. Cursor variables could stay unsigned as well, the check with comp_len will enforce correct values.
OK, I'll let clean up.
On Wed Apr 16 10:36:38 2025 +0000, Byeongsik Jeon wrote:
I added it because the logic was fine, but I was getting a weird 'may be used uninitialised' compiler warning. I'll look into it to see if it can be avoided.
Sorry. I see.
-
Byeongsik Jeon -
Byeongsik Jeon (＠bsjeon)
-
Rémi Bernon (＠rbernon)