XIM improvement (non-latin support + connection recovery) #30274

New Issue

Shinsuke Irie · 2012-02-20T14:47:36+01:00

Shinsuke Irie commented

2012-02-20 14:47:36 +01:00

%%%I implemented two features:

allow us to input non-latin languages such as Japanese/Chinese
recover XIM connection and its input contexts when XIM server restarted

Currently it supports only "root window" style input, while most people (and I) want "over the spot" or "on the spot" style one. Probably the implementation of "over the spot" or "on the spot" style becomes much complicated, because XIM server requires the coordinates of current cursor location relative to the screen in order to show the candidate window in appropriate position.

Please check the patch.
%%%

%%%I implemented two features: - allow us to input non-latin languages such as Japanese/Chinese - recover XIM connection and its input contexts when XIM server restarted Currently it supports only "root window" style input, while most people (and I) want "over the spot" or "on the spot" style one. Probably the implementation of "over the spot" or "on the spot" style becomes much complicated, because XIM server requires the coordinates of current cursor location relative to the screen in order to show the candidate window in appropriate position. Please check the patch. %%%

Shinsuke Irie commented

2012-02-20 14:47:36 +01:00

Changed status to: 'Open'

Shinsuke Irie commented

2012-02-22 17:34:26 +01:00

%%%Tiny fix. New patch is XIM_non-latin_V2.patch.

In my setting of IBus, control+m is assigned to "commit string", but this key stroke didn't work in the text editor. For example, if I'm going to input a utf8 character "あ", typing control+m makes a pseudo key event あ, but the control key remains pressed, so this event will be recognized as control+あ unwantedly. I modified the initial patch so that the modifiers in utf8 character events are ignored.

Although it works fine now, utf8 string still cannot be displayed in the text editor :p
%%%

%%%Tiny fix. New patch is XIM_non-latin_V2.patch. In my setting of IBus, control+m is assigned to "commit string", but this key stroke didn't work in the text editor. For example, if I'm going to input a utf8 character "あ", typing control+m makes a pseudo key event あ, but the control key remains pressed, so this event will be recognized as control+あ unwantedly. I modified the initial patch so that the modifiers in utf8 character events are ignored. Although it works fine now, utf8 string still cannot be displayed in the text editor :p %%%

Campbell Barton commented

2012-02-28 06:09:33 +01:00

%%%Hi, quick review of the patch, though I'm not exert in this area there are a few Q's I have about this patch.

setlocale() call is suspicious. I'm not sure of why this is needed but worry it would conflict with our translation support, or different users configurations. Can't the existing locale be used?

see ./source/blender/blenfont/intern/blf_lang.c's use of setlocale()

utf8_buf[256 * 6] --- this size seems arbitrary (and very large), is there some define that can be used instead?

%%%

%%%Hi, quick review of the patch, though I'm not exert in this area there are a few Q's I have about this patch. * setlocale() call is suspicious. I'm not sure of why this is needed but worry it would conflict with our translation support, or different users configurations. Can't the existing locale be used? see ./source/blender/blenfont/intern/blf_lang.c's use of setlocale() * utf8_buf[256 * 6] --- this size seems arbitrary (and very large), is there some define that can be used instead? %%%

Shinsuke Irie commented

2012-03-02 18:23:12 +01:00

%%%Hi Campbell, sorry for late reply.

setlocale() call is suspicious. I'm not sure of why this is needed but worry it would conflict with our translation support, or different users configurations. Can't the existing locale be used?

setlocale() is essential here. If removing this, no utf8 character will be inserted. In my patch, setlocale() uses system locale, but the other locales should be OK if it's available on the system. So, probably BLF_lang_set() can be used instead of setlocale(), but I couldn't use it here because I don't know if GHOST is permitted to include BLF.

utf8_buf[256 * 6] --- this size seems arbitrary (and very large), is there some define that can be used instead?

I think (256 * 6) is sufficiently large value to input one sentence in any languages. If using small array and the string exceeds this length, Xutf8LookupString() sets the status "BufferOverflow". In such case, we still can receive the string by calling Xutf8LookupString() again after expanding the array, but the code becomes somewhat complicated.
%%%

%%%Hi Campbell, sorry for late reply. > * setlocale() call is suspicious. I'm not sure of why this is needed but worry it would conflict with our translation support, or different users configurations. Can't the existing locale be used? setlocale() is essential here. If removing this, no utf8 character will be inserted. In my patch, setlocale() uses system locale, but the other locales should be OK if it's available on the system. So, probably BLF_lang_set() can be used instead of setlocale(), but I couldn't use it here because I don't know if GHOST is permitted to include BLF. > * utf8_buf[256 * 6] --- this size seems arbitrary (and very large), is there some define that can be used instead? I think (256 * 6) is sufficiently large value to input one sentence in any languages. If using small array and the string exceeds this length, Xutf8LookupString() sets the status "BufferOverflow". In such case, we still can receive the string by calling Xutf8LookupString() again after expanding the array, but the code becomes somewhat complicated. %%%

Shinsuke Irie commented

2012-03-02 21:16:49 +01:00

%%%Updated. (XIM_non-latin_V3.patch)

Changed the buffer size to 16 * 6 = 96 bytes. If the length of the input string is larger than 96 bytes, allocate another memory area and call Xutf8LookupString() again.

The other apps use very different sizes. For example, Wine and Emacs use 24 bytes and 513 bytes, respectively.

Wine: http://source.winehq.org/source/dlls/winex11.drv/keyboard.c#L1321
Emacs: https://github.com/emacsmirror/emacs/blob/master/src/xterm.c#L6372

If WITH_X11_XINPUT=OFF, use 1 byte array to store one null character.
%%%

%%%Updated. (XIM_non-latin_V3.patch) Changed the buffer size to 16 * 6 = 96 bytes. If the length of the input string is larger than 96 bytes, allocate another memory area and call Xutf8LookupString() again. The other apps use very different sizes. For example, Wine and Emacs use 24 bytes and 513 bytes, respectively. Wine: http://source.winehq.org/source/dlls/winex11.drv/keyboard.c#L1321 Emacs: https://github.com/emacsmirror/emacs/blob/master/src/xterm.c#L6372 If WITH_X11_XINPUT=OFF, use 1 byte array to store one null character. %%%

Shinsuke Irie commented

2012-03-02 22:10:08 +01:00

%%%Oops, updated again... sorry. XIM_non-latin_V4.patch.

The buffer size must be 6 bytes or larger even if WITH_X11_XINPUT=OFF.
%%%

%%%Oops, updated again... sorry. XIM_non-latin_V4.patch. The buffer size must be 6 bytes or larger even if WITH_X11_XINPUT=OFF. %%%

Shinsuke Irie commented

2012-03-03 05:25:36 +01:00

%%%XIM_non-latin_V5.patch
Changed to set utf8_buf NULL if WITH_X11_XINPUT=OFF. NULL pointer means not an utf8 character.

BTW, the documentation of GHOST_EventKey's constructor needs updating because the parameter btf8_buf is not described.
%%%

%%%XIM_non-latin_V5.patch Changed to set utf8_buf NULL if WITH_X11_XINPUT=OFF. NULL pointer means not an utf8 character. BTW, the documentation of GHOST_EventKey's constructor needs updating because the parameter btf8_buf is not described. %%%

Shinsuke Irie commented

2012-03-03 07:47:15 +01:00

%%%XIM_non-latin_V6.patch

Removed setlocale() in GHOST_SystemX11.cpp. Instead, moved BLF_lang_init() call in WM_init() so that it will be called before wm_ghost_init(), and modified BLF_lang_init() to call setlocale() even if U.transopts&USER_DOTRANSLATE is zero.
%%%

%%%XIM_non-latin_V6.patch Removed setlocale() in GHOST_SystemX11.cpp. Instead, moved BLF_lang_init() call in WM_init() so that it will be called before wm_ghost_init(), and modified BLF_lang_init() to call setlocale() even if U.transopts&USER_DOTRANSLATE is zero. %%%

Shinsuke Irie commented

2012-03-03 20:20:10 +01:00

%%%XIM_non-latin_V7.patch

Enlarged utf8_buf- [ ] by adding extra 5 bytes. The last 5 bytes of the buffer won't be used to obtain the utf8 data but should be useful to avoid the segfault, though such segfault have never happened on my system. Not using the extra memory area at the end of the buffer is potentially dangerous, because the constructor of GHOST_EventKey reads 6 bytes unconditionally even if the actual data is only 2-3 bytes.
%%%

%%%XIM_non-latin_V7.patch Enlarged utf8_buf- [ ] by adding extra 5 bytes. The last 5 bytes of the buffer won't be used to obtain the utf8 data but should be useful to avoid the segfault, though such segfault have never happened on my system. Not using the extra memory area at the end of the buffer is potentially dangerous, because the constructor of GHOST_EventKey reads 6 bytes unconditionally even if the actual data is only 2-3 bytes. %%%

Shinsuke Irie commented

2012-03-06 12:20:21 +01:00

%%%XIM_non-latin_V8.patch

Reverted the changes of the patch V6 because UI translation became not working.

blf_lang_set() cannot be used instead of setlocale() because GHOST_CreateSystemPaths() has not been executed yet. Don't worry, setlocale() in GHOST_SystemX11.cpp should not conflict with the other locale settings in Blender, because this setlocale() is executed earlier than all of them so its locale simply will be overridden by them, and XIM can work after locale is changed.
%%%

%%%XIM_non-latin_V8.patch Reverted the changes of the patch V6 because UI translation became not working. blf_lang_set() cannot be used instead of setlocale() because GHOST_CreateSystemPaths() has not been executed yet. Don't worry, setlocale() in GHOST_SystemX11.cpp should not conflict with the other locale settings in Blender, because this setlocale() is executed earlier than all of them so its locale simply will be overridden by them, and XIM can work after locale is changed. %%%

Shinsuke Irie commented

2012-03-19 11:49:46 +01:00

%%%XIM_non-latin_V9.patch

Refreshed. (only compensate for offset, no functional change)
%%%

%%%XIM_non-latin_V9.patch Refreshed. (only compensate for offset, no functional change) %%%

Dalai Felinto commented

2012-03-20 14:45:55 +01:00

%%%This patch fixes the problem of inputing ' " and accented letters in the Text Editor and in the UI elements
It still doesn't work for the 'Console' though.

+1

(ps I had to apply the patch with patch -p1 < XIM....patch. next time it's better to create the patch from inside the blender folder)%%%

%%%This patch fixes the problem of inputing ' " and accented letters in the Text Editor and in the UI elements It still doesn't work for the 'Console' though. +1 (ps I had to apply the patch with `patch -p1 < XIM....patch`. next time it's better to create the patch from inside the blender folder)%%%

Shinsuke Irie commented

2012-03-21 16:09:12 +01:00

%%%@dfelinto
Thanks for the testing.

It still doesn't work for the 'Console' though.

This patch solves only X11 specific problems. I think the console issue should be fixed by another patch.

(ps I had to apply the patch with patch -p1 < XIM....patch. next time it's better to create the patch from inside the blender folder)

Sorry, this is a Debian package style patch because I'm testing it in PPA in Launchpad. (https://launchpad.net/~irie/+archive/blender)
%%%

%%%@dfelinto Thanks for the testing. > It still doesn't work for the 'Console' though. This patch solves only X11 specific problems. I think the console issue should be fixed by another patch. > (ps I had to apply the patch with `patch -p1 < XIM....patch`. next time it's better to create the patch from inside the blender folder) Sorry, this is a Debian package style patch because I'm testing it in PPA in Launchpad. (https://launchpad.net/~irie/+archive/blender) %%%

Shinsuke Irie commented

2012-03-25 18:31:41 +02:00

%%%XIM_non-latin_V10.patch

Updated for the style cleanup in r45127 and r45130.
No functional change.
%%%

%%%XIM_non-latin_V10.patch Updated for the style cleanup in r45127 and r45130. No functional change. %%%

Sergey Sharybin commented

2012-03-26 12:05:12 +02:00

%%%Two things:

it's ok to use printf for error reporting in ghost but for diagnostics messages like "XIM input context destroyed" better to use GHOST_PRINT which will print messages only if blender is run with -d flag.
It's still looks a bit hackish things happening with setlocale. You're initializing XIM with one locale which would be changed later. Not sure if it'll always work fine. Maky XIM input might be initialized after loading startup.blend and setting locale used by the interface?%%%

%%%Two things: - it's ok to use printf for error reporting in ghost but for diagnostics messages like "XIM input context destroyed" better to use GHOST_PRINT which will print messages only if blender is run with -d flag. - It's still looks a bit hackish things happening with setlocale. You're initializing XIM with one locale which would be changed later. Not sure if it'll always work fine. Maky XIM input might be initialized after loading startup.blend and setting locale used by the interface?%%%

Shinsuke Irie commented

2012-03-27 15:24:58 +02:00

%%%XIM_non-latin_V11.patch

Don't use setlocale(), and don't open connection to XIM server on startup. The connection will be opened when the first KeyPress event is received after entering the event loop.
Use GHOST_PRINT() instead of printf() to indicate that the XIM connection is opened or XIC is created.

@nazgul
Thanks for the review.

diagnostics messages like "XIM input context destroyed"

"XIM input context destroyed" is an error message that will be displayed only when the XIM server unexpectedly shuts down.

GHOST_PRINT which will print messages only if blender is run with -d flag.

GHOST_PRINT won't be enabled by -d flag but GHOST_DEBUG macro should enable it. (Currently GHOST_DEBUG seems to be broken.)
%%%

%%%XIM_non-latin_V11.patch 1. Don't use setlocale(), and don't open connection to XIM server on startup. The connection will be opened when the first KeyPress event is received after entering the event loop. 2. Use GHOST_PRINT() instead of printf() to indicate that the XIM connection is opened or XIC is created. @nazgul Thanks for the review. > diagnostics messages like "XIM input context destroyed" "XIM input context destroyed" is an error message that will be displayed only when the XIM server unexpectedly shuts down. > GHOST_PRINT which will print messages only if blender is run with -d flag. GHOST_PRINT won't be enabled by -d flag but GHOST_DEBUG macro should enable it. (Currently GHOST_DEBUG seems to be broken.) %%%

Shinsuke Irie commented

2012-03-27 18:23:30 +02:00

%%%XIM_non-latin_V12.patch

Not only KeyPress event, but also FocusIn event opens the XIM connection and creates the input context. Typically FocusIn event occurs earlier than KeyPress event, so checking the KeyPress event works as a fallback.
%%%

%%%XIM_non-latin_V12.patch Not only KeyPress event, but also FocusIn event opens the XIM connection and creates the input context. Typically FocusIn event occurs earlier than KeyPress event, so checking the KeyPress event works as a fallback. %%%

Shinsuke Irie commented

2012-05-09 05:59:06 +02:00

%%%XIM_non-latin_V13.patch

Refreshed. (no functional change)
%%%

%%%XIM_non-latin_V13.patch Refreshed. (no functional change) %%%

Shinsuke Irie commented

2012-05-19 07:01:55 +02:00

%%%XIM_non-latin_V14.patch

Updated for style cleanup r46787.
Can anyone review this patch again or merge into trunk?
%%%

%%%XIM_non-latin_V14.patch Updated for style cleanup r46787. Can anyone review this patch again or merge into trunk? %%%

Sv. Lockal commented

2012-05-21 18:13:14 +02:00

%%%I have tested this patch and confirm that it works as expected with IBus and Ubuntu 12.04. It does not interfere with non-XIM input method for non-latin languages such as russian, and does not change visible behavior when WITH_X11_XINPUT is disabled. The only thing XIM user have to remember is to switch off IBus with ctrl+space after finishing non-latin text input. I hope it's not a big deal for XIM users.%%%

Leon Cheung commented

2012-05-23 04:51:35 +02:00

%%%Nice work! I think it would be really great to tell Ton to let it in the coming release. :)

http://www.blendernation.com/2012/05/22/developer-meeting-notes-may-20-2012%%%

%%%Nice work! I think it would be really great to tell Ton to let it in the coming release. :) http://www.blendernation.com/2012/05/22/developer-meeting-notes-may-20-2012%%%

Shinsuke Irie commented

2012-07-11 01:50:26 +02:00

%%%XIM_non-latin_V15.patch

Refreshed. (no functional change)
%%%

%%%XIM_non-latin_V15.patch Refreshed. (no functional change) %%%

Campbell Barton commented

2012-07-11 09:51:57 +02:00

%%%attached edits to XIM_non-latin_V15.patch, called XIM_non-latin_V15_cleanup.patch

only functional change was to replace printf with GHOST_PRINT%%%

%%%attached edits to XIM_non-latin_V15.patch, called XIM_non-latin_V15_cleanup.patch only functional change was to replace printf with GHOST_PRINT%%%

Campbell Barton commented

2012-07-11 10:32:51 +02:00