After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 698183 - Text input is too slow and some keys are broken after changing input source
Text input is too slow and some keys are broken after changing input source
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Quartz
3.6.x
Other Mac OS
: Normal normal
: ---
Assigned To: gtk-quartz maintainers
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2013-04-17 09:55 UTC by Takuro Ashie
Modified: 2013-06-20 15:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Stack trace with "--g-fatal-warnings" (1.46 KB, text/plain)
2013-04-17 09:58 UTC, Takuro Ashie
  Details
Proposed patch v1 (2.32 KB, patch)
2013-04-17 10:22 UTC, Takuro Ashie
committed Details | Review
Compares InputSourceID in maybe_update_keymap(). (1.97 KB, patch)
2013-05-21 04:28 UTC, Tsuyoshi Horo
rejected Details | Review
A patch against GTK+2 to observe input source (2.24 KB, patch)
2013-05-30 15:52 UTC, Takuro Ashie
committed Details | Review

Description Takuro Ashie 2013-04-17 09:55:44 UTC
Overview:
After I switch input sources by Mac OS X's input menu, text input becomes too slow and some keys
such as "delete" are disabled. In addition to it, many warning messages are output to console when
I press a key.

Steps to reproduce:
1. Open Mac OS X's "System Preferences" window.
2. Open "International" -> "Input source" page.
3. Enable "Vietnamese" -> "Simple Telex"
4. Enable "Show input menu in menu bar"
5. Run gtk3-demo from a console
6. Open a text entry from "Entry" -> "Entry Buffer"
7. Switch input source to "Simple Telex" from input menu of Mac OS X
8. Press normal keys to enter text into the entry
9. Press "delete" to delete some characters

Actual results:
* Text input becomes too slow. It's hard to enter text even one charater.
* Some keys such as "delete" doesn't work
* Many warning messages are output to console:
(gtk3-demo:37734): GLib-CRITICAL **: g_hash_table_insert_internal: assertion `hash_table != NULL' failed

(gtk3-demo:37734): GLib-CRITICAL **: g_hash_table_lookup: assertion `hash_table != NULL' failed

(gtk3-demo:37734): GLib-CRITICAL **: g_hash_table_insert_internal: assertion `hash_table != NULL' failed

(gtk3-demo:37734): GLib-CRITICAL **: g_hash_table_lookup: assertion `hash_table != NULL' failed

....

Expected results:
* Text input shound't become slow
* All keys should work correctly
* Runtime warning messages shouldn't be output

Additional information:
* I confirmed the bug on Japanese environment of Mac OS X.
  I'm not sure it is also reproduced on English environment.
* gtk+-2.24.16 has same issue.
Comment 1 Takuro Ashie 2013-04-17 09:58:43 UTC
Created attachment 241720 [details]
Stack trace with "--g-fatal-warnings"
Comment 2 Takuro Ashie 2013-04-17 10:22:30 UTC
Created attachment 241723 [details] [review]
Proposed patch v1

I think main cause of this is that GdkQuartz tries to update keymap on every key event.
It's too expesive and risky.

This fix uses CFNotificationCenter to observe keymap switching so that GdkQuartz tries to
update keymap only when it's really needed.
Comment 3 Takuro Ashie 2013-04-17 10:37:22 UTC
BTW I found this bug while I was developing an immodule for Mac OS X:

https://github.com/ashie/gtkimcocoa
Bug 694273
Comment 4 John Ralls 2013-04-29 19:53:39 UTC
I don't dispute your diagnosis, and the change does seem more efficient, but I'm not able to replicate the problem even if I switch localization settings to Japanese & Japan.

Are you perhaps not using a US keyboard?
Comment 5 Tsuyoshi Horo 2013-05-20 04:39:09 UTC
I think the main cause of this problem is this comparison.
https://github.com/GNOME/gtk/blob/d0af25f12c2c99ef0959ce19b1385a9f375ac19c/gdk/quartz/gdkkeys-quartz.c#L285

In maybe_update_keymap() function it is checked whether the keyboard layout has been changed or not.
But the way of checking it have a problem.
The return value of TISCopyCurrentKeyboardLayoutInputSource() is used to check it.
But the value can differ even if the keyboard layout is not changed.

I think InputSourceID should be compared which can be acquired with TISGetInputSourceProperty.

(CFStringRef*) TISGetInputSourceProperty(new_layout, kTISPropertyInputSourceID)
Comment 6 Tsuyoshi Horo 2013-05-20 19:55:39 UTC
Sorry, CFStringRef is correct.

(CFStringRef) TISGetInputSourceProperty(new_layout, kTISPropertyInputSourceID)
Comment 7 Tsuyoshi Horo 2013-05-21 04:28:03 UTC
Created attachment 244894 [details] [review]
Compares InputSourceID in maybe_update_keymap().
Comment 8 Tsuyoshi Horo 2013-05-21 04:28:35 UTC
I created a patch.
 0001-Compares-InputSourceID-in-maybe_update_keymap.patch

I think this patch and Ashie-san's patch should be applied.
Comment 9 Takuro Ashie 2013-05-29 04:27:33 UTC
Sorry for late response.

(In reply to comment #4)
> Are you perhaps not using a US keyboard?

Yes, I'm using Japanese keyboard.
Comment 10 Takuro Ashie 2013-05-29 04:37:23 UTC
(In reply to comment #5)
> I think the main cause of this problem is this comparison.
> https://github.com/GNOME/gtk/blob/d0af25f12c2c99ef0959ce19b1385a9f375ac19c/gdk/quartz/gdkkeys-quartz.c#L285

Thank you for following the bug. It seems fine.

I also suspected the comparison but I couldn't confirm & fix it by myself
because I'm not familiar with Mac OS X.
Comment 11 John Ralls 2013-05-30 03:30:52 UTC
Hmm. Does this manifest only with GtkIMCocoa? Or only with a non-US keyboard?
I'm not able to replicate the problem.

Since running update_keyboard is controlled by the notification, shouldn't all
of the conditional stuff that made it maybe_update_keyboard be removed rather than tweaking which identifier is used to notice that the layout is changed?
Comment 12 Tsuyoshi Horo 2013-05-30 07:43:59 UTC
> Hmm. Does this manifest only with GtkIMCocoa? Or only with a non-US keyboard?
> I'm not able to replicate the problem.

I don't know why, but the return value of TISCopyCurrentKeyboardLayoutInputSource differ only when we are using third party input method software.
Strange to say, it does not always occur even if we are using third party input method software.
I think this is depending on OS's internal state.

You can build and try Apple's sample code of input method as the following steps.

curl -o NumberInput_IMKit_Sample.zip https://developer.apple.com/library/mac/samplecode/NumberInput_IMKit_Sample/NumberInput_IMKit_Sample.zip
xcodebuild -project "NumberInput_IMKit_Sample/trunk/NumberInput 4/NumberInput.xcodeproj" -configuration "Release"
sudo cp -rf "NumberInput_IMKit_Sample/trunk/NumberInput 4/build/Release/NumberInput.app" "/Library/Input Methods/"

Logout
Login

Open "System Preferences"
Open "Language & Text"
Select "Input Sources"
Turn on "NumberInput"

Select "Decimal" in input menu in menu bar.
Type "123467" + Space -> "1,234,567" will be shown.


> Since running update_keyboard is controlled by the notification, shouldn't all
> of the conditional stuff that made it maybe_update_keyboard be removed rather
> than tweaking which identifier is used to notice that the layout is changed?

Oh yes.
Removing this conditional stuff is good enough.


By the way, this problem occurs while using GIMP with third party input method software.
I think GIMP is using GTK2.
It this fix will be applied to GTK2?
Comment 13 Tsuyoshi Horo 2013-05-30 07:48:29 UTC
fix typo:
Will this fix be applied to GTK2?
Comment 14 Takuro Ashie 2013-05-30 09:52:45 UTC
(In reply to comment #11)
> Hmm. Does this manifest only with GtkIMCocoa? Or only with a non-US keyboard?
> I'm not able to replicate the problem.

I confirmed this issue without any immodules.
I didn't install any third party immodules to reproduce it.
It's still reproduced even if I disable all bundled immodules.
Comment 15 Takuro Ashie 2013-05-30 09:56:37 UTC
One of posibillity is target architecture?
I confirmed it with x86_64 binary.
Comment 16 Takuro Ashie 2013-05-30 10:24:04 UTC
(In reply to comment #12)
> I don't know why, but the return value of
> TISCopyCurrentKeyboardLayoutInputSource differ only when we are using third
> party input method software.

I also confirmed it using third party input method (GoogleIME) at first.

Since I ensured to simplify steps to reproduce, I looked for a system IME
which causes the issue on my environment.
Comment 17 Takuro Ashie 2013-05-30 15:52:25 UTC
Created attachment 245658 [details] [review]
A patch against GTK+2 to observe input source

(In reply to comment #13)
> Will this fix be applied to GTK2?

Sorry, I forgot to attach the patch against GTK2 although I already prepared it.
Comment 18 Tsuyoshi Horo 2013-06-05 12:52:06 UTC
> Sorry, I forgot to attach the patch against GTK2 although I already prepared
it.

Thank you very much!
Comment 19 Michael Natterer 2013-06-20 08:39:52 UTC
I think that we should do two things:

- update the keymap in the notification callback
- remove the check from the callback, as said in comment 11

Even without having 3rd party input methods or japanese keyboards,
both changes seem completely logical to me.
Comment 20 Michael Natterer 2013-06-20 09:50:49 UTC
If there are no objections I will do exactly that, in separate patches
because the check removal will cherry-pick without any conflict between
the branches.
Comment 21 John Ralls 2013-06-20 12:43:47 UTC
Sounds good to me.
Comment 22 Takuro Ashie 2013-06-20 13:58:25 UTC
I also agree with you.
Comment 23 Michael Natterer 2013-06-20 15:31:21 UTC
Fixed in master, gtk-3-8 and gtk-2-24:

commit f04fe99deb92e425acf1da95a0d8a3455bdc14b4
Author: Michael Natterer <mitch@gimp.org>
Date:   Thu Jun 20 17:21:25 2013 +0200

    quartz: remove check for keymap changes from update_keymap()
    
    The function is now only called when the keymap has actually
    changed. bug #698183.
    (cherry picked from commit e5e17cf3612191c0497882a6a6b3206dc0ca96cd)

 gdk/quartz/gdkkeys-quartz.c | 425 ++++++++++++++++++++++++++----------------------------
 1 file changed, 207 insertions(+), 218 deletions(-)

commit 7b08ca94cc5c9c7798b6f7541f409f3c7dd09c52
Author: Michael Natterer <mitch@gimp.org>
Date:   Thu Jun 20 17:09:07 2013 +0200

    quartz: update the keymap only if the input method changed
    
    and not on each keystroke, which for some IMs apparently caused a full
    update on each keystroke, not just a check for changes. Patch from
    Takuro Ashie, bug #698183.
    
    (cherry picked from commit bbe3554fa9baefc875dcc3d62cd1a4bbbaf18b53)

 gdk/quartz/gdkkeys-quartz.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)