GNOME Bugzilla – Bug 694273
Patch to support NSTextInputClient in text widgets
Last modified: 2013-07-27 21:51:43 UTC
Created attachment 236935 [details] Patch against gtk+-2.24.13 Here is a patch to support NSTextInputClient with gtk+ input module, based on a patch by Hiroyuki Yamamoto (http://www.sraoss.jp/sylpheed/sylpheed/macosx/gtk+-2.24.0-macosx-textinputclient_ja-test1.patch). Tested with some Japanese Input Methods.
Created attachment 237003 [details] [review] Patch against gtk+-2.24.13 Fixed the patch with s/EVENT/EVENTS/ for GDK_NOTE().
Created attachment 237141 [details] [review] Patch against gtk+-2.24.13 I've found a minor glitch in a corner case, and also a few unused debug codes. Please use the updated one.
Created attachment 237274 [details] [review] Revised patch Update the patch. * Plug memory leak * Fix for widgets which return GdkWindow with gdk_quartz_window_get_nsview
Hi Kato-san. It's been a while! By chance, I've also been developing a gtk-im-module for Mac OS X: GtkIMCocoa: https://github.com/ashie/gtkimcocoa (In reply to comment #0) > patch by Hiroyuki Yamamoto > (http://www.sraoss.jp/sylpheed/sylpheed/macosx/gtk+-2.24.0-macosx-textinputclient_ja-test1.patch). I also reffered to Hiroyuki's patch, but I decided to develop it from scratch. Because, as Hiroyuki says (http://sylpheed.sraoss.jp/diary/?date=20110328), it has some issues: * GtkIMQuarz isn't completed only by it, it's needed to modify GDK. * This modification forces to implement NSTextInputClient to all NSView, although most of them doesn't need it in fact. * It uses g_object_set_data() and g_object_get_data() to communicate with GDK. It's inefficient. (In addition, I think "Quartz" suffix isn't suitable because NSTextInputClient is almost unrelated with Quartz layer, isn't it?) To solve these problems, GtkIMCocoa instantiates its own NSView and forward key events to it. This way requires no patch against GDK. It works fine with default Japanese input method of OS X. (Although it seems broken with Google Japanese Input yet...)
(In reply to comment #4) > Hi Kato-san. It's been a while! :) > By chance, I've also been developing a gtk-im-module for Mac OS X: > > GtkIMCocoa: https://github.com/ashie/gtkimcocoa Great! I think This is the way to go. My updates for Hiroyuki-san's patch is just a toy project in my spare time. > (In reply to comment #0) > > patch by Hiroyuki Yamamoto > > (http://www.sraoss.jp/sylpheed/sylpheed/macosx/gtk+-2.24.0-macosx-textinputclient_ja-test1.patch). > > I also reffered to Hiroyuki's patch, but I decided to develop it from scratch. > Because, as Hiroyuki says (http://sylpheed.sraoss.jp/diary/?date=20110328), it > has some issues: > > * GtkIMQuarz isn't completed only by it, it's needed to modify GDK. > * This modification forces to implement NSTextInputClient to all NSView, > although most of them doesn't need it in fact. > * It uses g_object_set_data() and g_object_get_data() to communicate with GDK. > It's inefficient. > To solve these problems, GtkIMCocoa instantiates its own NSView and forward key > events to it. > This way requires no patch against GDK. OK. > (Although it seems broken with Google Japanese Input yet...) Really? With my patch, Google Japanese IME also works. Please refer my implementation if needed. Anyway, problems in my patch so far are * It doesn't handle reset correctly. I'm not sure how to force to reset in NSTextInputClient, which should end with commitComposition:sender in IMKInputController. * It requires to use toplevel window's key events, as you know.
(In reply to comment #5) > * It requires to use toplevel window's key events, as you know. I mean, I haven't implemented the handlers for toplevel window yet, which should be implemented for the correct Input Method handling.
Changing the component to quartz from mac integration, which is for the gtk-mac-integration and gtk-mac-bundler projects. Matthias, is this too big a change for 2.24?
Sorry for my late response. (In reply to comment #5) > > (Although it seems broken with Google Japanese Input yet...) > > Really? With my patch, Google Japanese IME also works. Please refer my > implementation if needed. I investigated this issue and finally I found that it's not immodules's bug, it's a GdkQuartz's bug. Here is the patch for this issue: https://github.com/ashie/gtkimcocoa/tree/master/patches I'll report it as another bug.
(In reply to comment #8) > I investigated this issue and finally I found that it's not immodules's > bug, it's a GdkQuartz's bug. Here is the patch for this issue: > > https://github.com/ashie/gtkimcocoa/tree/master/patches > > I'll report it as another bug. Bug 698183
So how do I make this work? I tried GTK_IM_METHOD=quartz gtk_demo lainched the Entry Buffer demo, turned on Kotoeri Hiragana, and typed. The automatic completion didn't happen. All I got was the Hiragana letters, which works without any IM module.
Created attachment 242881 [details] screenshot of Kotoeri
(In reply to comment #10) > So how do I make this work? I tried > GTK_IM_METHOD=quartz gtk_demo > lainched the Entry Buffer demo, turned on Kotoeri Hiragana, and typed. The > automatic completion didn't happen. All I got was the Hiragana letters, which > works without any IM module. I'm not sure what you exactly mean about the automatic completion, but automatic prediction is working as the attached screenshot. You should check whether the im-quartz modules is listed correctly by ${gtk_inst_path}/etc/gtk-2.0/gtk.immodules. I think ${gtk_inst_path}/bin/gtk-query-immodules-2.0 will help.
Thanks, that did it. make install didn't actually populate the gtk.immodules file. Running gtk-query-immodules-2.0 fixed it. So I'm ready to commit this for you. Can I have your actual name for the "author" field?
The patch should at least follow the coding style, as in for example spaces before ( and { on their own line.
Good point. Thanks for reminding me.
Created attachment 242965 [details] [review] Patch reformated to comply with Gnome Coding Style I ran the affected files through astyle and then fixed up the ObjC bits that astyle gets wrong along with wrapping some over-long lines. Emacs doesn't do a very good job indenting ObjC method arguments so I left those long.
Created attachment 242966 [details] [review] As before, but with - ( in the ObjC function defs fixed
Created attachment 242968 [details] [review] One more time, with a couple of formatting errors I missed the first two passes.
Created attachment 244130 [details] [review] Patch against 3.6.4 For reference here's the same patch against gtk 3.6.4. Only minor edits were required - most of the original patch applied verbatim With this applied, Japanese and Chinese input appears essentially functional in gtk3-demo Some quirks are visible in Japanese input when used with a more complex application (Gnote), and the input pre-selection list appears at some random (?) location rather than below the input text, but it's still light years better than before
Both patches applied to their respective branches, including backport to gtk-3-8. Thanks.
I'm sorry for my too late response、but I have a question to John. (Sorry, I failed to receive notification mails from Bugilla for a months...) What do you think about following issues? (In reply to comment #4) > I also reffered to Hiroyuki's patch, but I decided to develop it from scratch. > Because, as Hiroyuki says (http://sylpheed.sraoss.jp/diary/?date=20110328), it > has some issues: > > * GtkIMQuarz isn't completed only by it, it's needed to modify GDK. > * This modification forces to implement NSTextInputClient to all NSView, > although most of them doesn't need it in fact. > * It uses g_object_set_data() and g_object_get_data() to communicate with GDK. > It's inefficient. I regret that you merged the patch without discussion about important base design. I read all comments in this bug, but I can't find your opinion about it. Did you read GtkIMCocoa's code? (https://github.com/ashie/gtkimcocoa) Did you compare both implementations? Did you understand both implementations? I'd like to know how you judge about these issues.
(In reply to comment #21) > I'm sorry for my too late response、but I have a question to John. > (Sorry, I failed to receive notification mails from Bugilla for a months...) > > What do you think about following issues? > > (In reply to comment #4) > > I also reffered to Hiroyuki's patch, but I decided to develop it from scratch. > > Because, as Hiroyuki says (http://sylpheed.sraoss.jp/diary/?date=20110328), it > > has some issues: > > > > * GtkIMQuarz isn't completed only by it, it's needed to modify GDK. > > * This modification forces to implement NSTextInputClient to all NSView, > > although most of them doesn't need it in fact. > > * It uses g_object_set_data() and g_object_get_data() to communicate with GDK. > > It's inefficient. > > I regret that you merged the patch without discussion about important base > design. > I read all comments in this bug, but I can't find your opinion about it. > Did you read GtkIMCocoa's code? (https://github.com/ashie/gtkimcocoa) > Did you compare both implementations? > Did you understand both implementations? > > I'd like to know how you judge about these issues. I understood those concerns to be about the original Hiroyuki patch, and it seemed that Kato-san thought that he'd resolved them. I applied Kato-san's patch to 2.24 after correcting the formatting to comply with our coding standards and testing that it does in fact work. I also applied Matthew Francis's mods to my patch for Gtk3 to master and to 3.8. It does appear to work from the limited testing that I'm able to do. No, I didn't look at your Github branch. You said that it doesn't quite work, and you didn't offer a patch. I don't "judge". This isn't a competition. Kato-san reported a (well-known, long-standing) problem and offered a patch that appears to correct it. I fixed the formatting, tested it, and committed it. That's generally the way we handle patch submissions.
John, thank you for replying to closed bug. But I must point out your mistake. > I understood those concerns to be about the original Hiroyuki patch, and it > seemed that Kato-san thought that he'd resolved them. I applied Kato-san's Unfortunately you are misunderstanding them. Obviouslly they still remained in Kato-san's patch. (You should notice it if you read the patch.) This is the why I pointed out them at this place. And Kato-san agreed with me (Comment #5). You ignored the context. > You said that it doesn't quite work, I also said "it's a GdkQuartz's bug" (Comment #9) and reported the bug (Bug #698183). > and you didn't offer a patch. Because I was waiting your opinion about above context. I pointed out some base design issues. If you are a honest developer, you should answer to them. If you are a honest developer, you will have interest to another implementation. But now I understand that you couldn't do it because you misunderstood our discussion. > This isn't a competition. Yes, I know it. I don't intent to beat Kato-san (I respect him!), I just only aim to make GTK+ better and better. I'll accept that you reject my indication if you explain rational reasons, but I couldn't accept it because you simply ignore me. > Kato-san reported a (well-known, > long-standing) problem and offered a patch that appears to correct it. I > fixed the formatting, tested it, and committed it. That's generally the way > we handle patch submissions. Hmm... it lacks an important process. Do you not check the desgin? I think most developers of most OSS projects reviews the desgin to keep their code maintainable when they receives a patch. Finally, I have one more question. Can I get an opportunity to discuss about it again when I provide a patch and report it as another bug?
(In reply to comment #23) > This is the why I pointed out them at this place. > And Kato-san agreed with me (Comment #5). Not exactly. He said: > Anyway, problems in my patch so far are > * It doesn't handle reset correctly. I'm not sure how to force to reset in > NSTextInputClient, which should end with commitComposition:sender in > IMKInputController. > * It requires to use toplevel window's key events, as you know. The first is a relatively minor problem and the second is a consequence of Gdk's design where only toplevels have NSWindows and NSViews. Your IM, which subclasses NSView, is an interesting approach, but it seems to depend upon NSTextInputContext, which is available only in 10.6 and later. Current policy is to support 10.4 and later. Can you find a different way to activate your non-windowed view? > I also said "it's a GdkQuartz's bug" (Comment #9) and reported the bug (Bug > #698183). > > > > and you didn't offer a patch. > > Because I was waiting your opinion about above context. > I pointed out some base design issues. > If you are a honest developer, you should answer to them. > If you are a honest developer, you will have interest to another > implementation. > > But now I understand that you couldn't do it because you misunderstood our > discussion. > > > > This isn't a competition. > > Yes, I know it. > > I don't intent to beat Kato-san (I respect him!), I just only aim to make GTK+ > better and better. > > I'll accept that you reject my indication if you explain rational reasons, > but I couldn't accept it because you simply ignore me. One more time: You didn't submit a patch. But having now looked at GtkIMCocoa it appears to be a stand-alone product; aside from your bug 698183 patch, it appears that there's nothing to do. Once 698183 is committed, a user can choose to install your IM module and load it or to load Kato-san's. Your objection that the Hiroyuki/Kato patch requires extending Gdk is a non-issue. In fact, your change in 698183 is far more invasive (which has nothing to do with its correctness or necessity, it's just an observation). Also, your version requires a special GtkIMContext. We're trying to move as much of the platform-specific code -- especially objective-c code -- as possible into Gdk, so from that standpoint Kato-san's approach is preferred. > > > > Kato-san reported a (well-known, > > long-standing) problem and offered a patch that appears to correct it. I > > fixed the formatting, tested it, and committed it. That's generally the way > > we handle patch submissions. > > Hmm... it lacks an important process. > Do you not check the desgin? > I think most developers of most OSS projects reviews the desgin to keep their > code maintainable when they receives a patch. I don't see any problem with Kato-san's design other than adopting the NSTextInputClient protocol, which was introduced in 10.5 and will therefore break 10.4 builds. > > > Finally, I have one more question. > Can I get an opportunity to discuss about it again when I provide a patch and > report it as another bug? You can open a new bug or add on to bug 698183.
Now you've explained some reasons to go with Kato-san's patch and to reject my approach. This is the answer I wanted to hear from you. I'm almost satisfied with it. Thank you very much! FYI there are some supplements I should explain to you: (In reply to comment #24) > Current policy is to support 10.4 and later. Sorry, I didn't know it. > NSTextInputContext, which is available only in 10.6 and later. -- snip -- > Can you find a different way to activate your > non-windowed view? Since I'm also afraid it, I have some ideas against it. But currently I don't have any implementation of them yet. So, GtkIMCocoa doesn't satisfy your requirement at this time. It's reasonable to reject my approach. > Also, your version requires a special GtkIMContext. We're trying to move as > much of the platform-specific code -- especially objective-c code -- as > possible into Gdk, so from that standpoint Kato-san's approach is preferred. OK, I understand. GtkIMCocoa doesn't satisfy this policy. It's reasonable to reject my approach. BTW I'm wondering why you avoid to include platform-specific code to an immodule. It's a pluggable module so it can be separated from libgtk. imxim (for X11) and imime (for Windows) also includes platform-specific code. On the other hand, it can also be embed to libgtk when you add "--with-included-immodules" to configure. It may be the reason you avoid it. > Not exactly. No. You are still misunderstanding. He exactly agreed with me at least at this time: (In reply to comment #5) >> GtkIMCocoa: https://github.com/ashie/gtkimcocoa > >> Great! I think This is the way to go. But, of course he may have other opinions when he knows your policy. > He said: Although I couldn't understand what you wanted to say in this paragraph, probably you thought following 2 issues are weak points of Kato-san's patch against GtkIMCocoa and you tried to cover them, but you don't need to afraid it. Because I don't think it as being weak points of only Kato-san's patch. GtkIMCocoa has also same problem. Probably Kato-san also think so and just only intent to take over remaining works to successors. > the second is a consequence of > Gdk's design where only toplevels have NSWindows and NSViews. Probably you are misunderstanding this issue. Please see bug 90082 comment 2 for more detail. This is a famous hack among input method developers. So Kato-san said "as you know". Most Japanese input methods want to take highest priority to get key events since they want to use function keys (F6, F7, F8 …) and so on. Because they conflicts with system's key accelerators, GTK+ does't allow immodules to do it in usual key handlings (filter_keypress function of GtkIMContext). To solve this issue, Owen Taylor who is the original author of immodule subsystem (and many features of GTK+ related modules) introduced this hack to us input method developers. When the immodule connects to the top-level GtkWindow's "key-press" event, it can steal these key events. > One more time: You didn't submit a patch. OK. it's my mistake. I'm sorry about it. > But having now looked at GtkIMCocoa > it appears to be a stand-alone product BTW I was planning to contribute the code when it become stable enough. Why I started it as stand-alone product is that I wanted to control release cycle of it on early development stage to get more feedbacks from early adaptors and to keep the module bundled by GTK+ stable. Once when I was developing an immodule for Windows (imime) which is currently included in GTK+, this approach succeeded very much: bug 135195 >> Can I get an opportunity to discuss about it again when I provide a patch and>> report it as another bug?> >You can open a new bug or add on to bug 698183. Since bug 698183 is completely different issue, I think it's not suitable place for this topic. I'll open a new bug with a patch if I want to discuss about it again.
> BTW I'm wondering why you avoid to include platform-specific code to an > immodule. It's not the module that's the concern, it's the context (gtkimcontextcocoa.c) which would go in the gtk directory. We'd like to make that directory generic with all of the platform-specific stuff in gdk.
The changes committed based on this report: https://git.gnome.org/browse/gtk+/commit/?h=gtk-2-24&id=21c775a2c811662881ae0b2f7606ab6c0875011b break Inkscape's text input on-canvas if compiled with GTK+/Quartz: https://bugs.launchpad.net/inkscape/+bug/1198597 Both latest stable (Inkscape 0.48.4) as well as current trunk (Inkscape 0.48+devel) builds are affected: - with GTK+ 2.24.18: text input on-canvas is ok - with GTK+ 2.24.18 + patch from commit 21c775a2c811662881ae0b2f7606ab6c0875011b: text input on-canvas broken - GTK+ 2.24.19, 2.24.20: text input on-canvas broken
That's odd. This code isn't even enabled unless one has GTK_IM_MODULES=quartz set in the environment, nor should it have anything to do with "shortcuts", neither accelerators, bindings, nor mnemonics. Which sort of shortcut behavior do you observe? Has Inkscape extended GtkQuartzView with its own NSTextInputClient protocol implementation?
I haven't tried GIMP's text tool on the mac since that change, will check if it is also affected.
(In reply to comment #28) > That's odd. This code isn't even enabled unless one has GTK_IM_MODULES=quartz > set in the environment, You are misunderstanding it in two aspects: * When the immodules.cache file is created correctly, imquartz will always be enabled because it accepts all locales: https://github.com/GNOME/gtk/blob/gtk-2-24/modules/input/imquartz.c#L63 When the current locale is acceptable by an immodule, gtk will enable it automatically. (But gtk+-2.24.20 doesn't create immodules.cache correctly due to bug 703789.) * This commit modifies GdkQuartzView and allows all native views to become first responder and to handle NSTextInputClient Protocol. It's possible to affect to text input even if you don't enable imquartz. https://github.com/GNOME/gtk/blob/gtk-2-24/gdk/quartz/GdkQuartzView.c#L41 At least it's not same with gtk+-2.24.18.
(In reply to comment #27) > The changes committed based on this report: > > > https://git.gnome.org/browse/gtk+/commit/?h=gtk-2-24&id=21c775a2c811662881ae0b2f7606ab6c0875011b > > break Inkscape's text input on-canvas if compiled with GTK+/Quartz: > > https://bugs.launchpad.net/inkscape/+bug/1198597 Although imquartz requires native event (NSEvent): https://github.com/GNOME/gtk/blob/gtk-2-24/modules/input/imquartz.c#L146 Inkscape drops it: http://bazaar.launchpad.net/~inkscape.dev/inkscape/RELEASE_0_48_BRANCH/view/head:/src/display/sp-canvas.cpp#L1294 This is the cause of this issue.
Created attachment 250250 [details] [review] Fallback to slave IM context if no NSEvent exists At least imquartz should fallback to slave IM context to avoid no response. Notice that native IM doesn't work even if you apply this patch. It only commits ASCII characters to avoid the regression. To solve it completely, probably we should fix Inkscape.
I agree, GdkEvents cannot be copied like that. Inkscape should simply use gdk_event_copy() which will also copy the backend-specific event data, in this case the attached NSEvent.
With GTK+/Quartz 2.24.20 + patch from comment 32 applied, Inkscape's default on-canvas text input works again (so far no changes to inkscape itself: tested with the same build of current Inkscape trunk (r12439), as well as with Inkscape stable 0.48.4, on OS X 10.7.5).
Thanks Ashie-san for the workaround in comment #32. John, could you apply the Ashie-san's patch? And don't forget to consider applying the patch in bug #701332 too.
(In reply to comment #31) > Although imquartz requires native event (NSEvent): > > https://github.com/GNOME/gtk/blob/gtk-2-24/modules/input/imquartz.c#L146 Sorry, above link is wrong. The correct one is: https://github.com/GNOME/gtk/blob/gtk-2-24/modules/input/imquartz.c#L158
Yes the patch from comment 32 should be applied, but it is only a workaround for broken application code. The actual bug is in inkscape, it has just been triggered by the new input method code.
I imagine that it should go to master and gtk-3-8 as well as gtk-2-24, even though OP reported (in the Inkscape bug) that it works OK in gtk-3-8? Pushed to gtk-2-24.
John Ralls wrote: > even though OP reported (in the Inkscape bug) that it works OK in gtk-3-8? Likely because the change is not yet available in a stable release? The experimental GTK3 builds I mention in the inkscape bug report are based on current stable GTK3, built from the release tarball, not from the git branch.
(In reply to comment #39) > John Ralls wrote: > > even though OP reported (in the Inkscape bug) that it works OK in gtk-3-8? > > Likely because the change is not yet available in a stable release? > The experimental GTK3 builds I mention in the inkscape bug report are based on > current stable GTK3, built from the release tarball, not from the git branch. Ah, you're right. Missed 3.8.2 by a week.
OK, Ashie-san's and Kato-san's patches are both applied to master and gtk-3-8 as well as gtk-2-24. Thanks to both of you.
Many thanks to all of you for the prompt response and the committed workaround - I'll keep the inkscape bug report open for now and try to get input from inkscape devs about the proposed changes (comment #33 by Mitch).