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 163251 - GtkTextView derivatives need some way to set the need_im_reset flag.
GtkTextView derivatives need some way to set the need_im_reset flag.
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkTextView
2.20.x
Other All
: Normal blocker
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on: 592405
Blocks: 586476 593601 594716 595791 597610
 
 
Reported: 2005-01-07 17:26 UTC by Paolo Maggi
Modified: 2011-02-04 16:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Implement gtk_reset_im_state (1.90 KB, patch)
2010-01-22 17:07 UTC, Christian Dywan
none Details | Review
Create IM context lazily, and recognize "none" (7.90 KB, patch)
2010-01-25 12:23 UTC, Christian Dywan
needs-work Details | Review
Expose gtk_text_view_reset_im_context (1.38 KB, patch)
2010-01-25 12:27 UTC, Christian Dywan
none Details | Review
Expose gtk_text_view_reset_im_context #2 (2.41 KB, patch)
2010-01-25 12:30 UTC, Christian Dywan
none Details | Review
Added all the required api (8.15 KB, patch)
2010-05-05 00:14 UTC, Javier Jardón (IRC: jjardon)
committed Details | Review

Description Paolo Maggi 2005-01-07 17:26:22 UTC
As discussed on IRC (see attached log), GtkTextView derivatives probably need
some way to set need_im_reset flag.

<paolo> mclasen: hi. May you please comment on patch for bug #154039. Is
need_im_reset in GtkTextView public? 
<owen> paolo:Please don't commit that
<owen> paolo: Writing to fields of an object structure is *NEVER* acceptable
<paolo> owen: ok, I had the same impression. Any idea on how to fix it?
<mclasen> owen: GtkTextView derivatives probably need some way to set that flag
<owen> paolo: Not a lot, really. At least without GtkTextView changes
<owen> Maybe that patch is OK as a temporary thing.... though certainly don't
commit it without a GTK+ bug open to allow doing beter
Comment 1 Paolo Borelli 2008-06-20 12:19:29 UTC
With gseal merged in trunk this is now critical to fix.
Comment 2 Javier Jardón (IRC: jjardon) 2009-09-08 18:15:24 UTC
Also, an accesor to ->im_context is needed too
Comment 3 Matthias Clasen 2009-09-08 19:05:02 UTC
No. the im context is not part of the public api
Comment 4 Andrew Cowie 2009-09-09 02:06:48 UTC
If there was a way to construct GtkTextView [and GtkEntry] telling it not to use any IM context at all, then the developer could set up their own.

(and there would be no need to expose the admittedly very crafty and custom internals of GtkTextView and GtkEntry's use of GtkIMContext)

I've found myself needing to hook up my own GtkIMContext to a GtkTextView's "key-press-event" and "key-release-event" signals. Then using GtkIMContext's "commit" signal solved lots of problems (it means all gtk_text_buffer_insert() stuff is programmatic, so I control what's in the view, and don't have to figure out which "insert-text" signals are programmatic and which are from user input)

No, I'm not fixated on this. Just aware of all the duplicaion. I don't know how heavy each GtkIMContext is. Seems like it might be an idea, perhaps via a "none" setting in the "gtk-im-module" global, something like that?

AfC
Comment 5 Christian Dywan 2010-01-18 17:07:57 UTC
Being able to disable IM support might be a good idea, however I'd rather make that a construction only property than a global value, so you can disable it in the subclass.

The interesting question is whether im_reset is superfluous if applications with subclasses can override it completely, ie GtkSourceView, or if there is still a different case.
Comment 6 Christian Dywan 2010-01-18 17:23:05 UTC
(In reply to comment #5)
> Being able to disable IM support might be a good idea, however I'd rather make
> that a construction only property than a global value, so you can disable it in
> the subclass.

Actually after being pointed out on IRC that "im-module" is a property in GtkTextView, I think we should use that and allow "none" there. If we delay creation of the IM context that should do the trick.
Comment 7 Paolo Borelli 2010-01-18 17:26:19 UTC
Note that in gtksourceview we do not want to override completely im-context or disable it completely, we just need to avoid bugs when handling keypresses
Comment 8 Andrew Cowie 2010-01-20 07:55:26 UTC
(In reply to comment #6)

> Actually after being pointed out on IRC that "im-module" is a property in
> GtkTextView, I think we should use that and allow "none" there. If we delay
> creation of the IM context that should do the trick.

I'd appreciate anything along these lines. I'm using GtkTextView heavily, but we've had to do our own input handling and so have fired up our own GtkIMContexts and are feeding keystrokes to it rather than to TextView, so the orignal one in the TextView isn't doing anything. It's not killing us being there, of course, but for completeness it seems it'd be nice to be able to disable it.

AfC
Comment 9 Paolo Borelli 2010-01-20 08:38:28 UTC
I gave a quick look at the current code in gedit and gsv which originally triggered this request and I am not sure anymore that we actually need getters and setters for need_im_reset: this only case where we need to access it is this:


/* There is no "official" way to reset the im context in GtkTextView */
static void
reset_im_context (GtkTextView *text_view)
{
	if (text_view->need_im_reset)
	{
		text_view->need_im_reset = FALSE;
		gtk_im_context_reset (text_view->im_context);
	}
}


Maybe a different approach would be to add a "reset" signal to ImContext, gtktextview would connect to that signal and clear its need_im_reset when the signal is emitted. At that point gedit would simply call gtk_im_context_reset without having to care about gtktextview internal state
Comment 10 Christian Dywan 2010-01-22 16:31:26 UTC
Gedit cannot call any signal on the IM context because in the future it won't have access to the IM context. I would rather think about a gtk_text_view_reset_im_context () which would do about what your helper function does.
Comment 11 Christian Dywan 2010-01-22 17:07:21 UTC
Created attachment 152022 [details] [review]
Implement gtk_reset_im_state

Pablo, what do you think about this function?
Comment 12 Javier Jardón (IRC: jjardon) 2010-01-22 17:18:40 UTC
Note that GTK_ENTRY()->im_context is needed too (See bug #595791)
Comment 13 Paolo Borelli 2010-01-22 19:48:17 UTC
Obviously (given that it is the same code) the proposed function would work too for me. In my proposal I was assuming that the IM context object would be exposed too as a property of textview.
Comment 14 Christian Dywan 2010-01-23 06:38:32 UTC
(In reply to comment #12)
> Note that GTK_ENTRY()->im_context is needed too (See bug #595791)

As far as I see, Epiphany does the same as gedit, it has to reset the IM context but doesn't do anything else.
Comment 15 Christian Dywan 2010-01-23 06:40:27 UTC
Actually, that's not true. It needs to reset a GtkEntry according to grep. So I suppose we would need gtk_entry_rest_im_state, too.
Comment 16 Christian Dywan 2010-01-25 12:23:06 UTC
Created attachment 152220 [details] [review]
Create IM context lazily, and recognize "none"

I moved creation of the IM context to a function so that it can be created lazily. If the text view is focussed or a key is pressed or released, the IM context is created. If "im-module" is "none" focus and key handling are no-ops.
Comment 17 Christian Dywan 2010-01-25 12:27:52 UTC
Created attachment 152221 [details] [review]
Expose gtk_text_view_reset_im_context

On second thought, I modified the patch to allow resetting the IM context to expose the existing gtk_text_view_reset_im_context instead of adding another function. So internal and external code share the same code path.
Comment 18 Christian Dywan 2010-01-25 12:30:02 UTC
Created attachment 152222 [details] [review]
Expose gtk_text_view_reset_im_context #2

Re-uploading a complete patch. Sorry.
Comment 19 Paolo Borelli 2010-01-25 13:31:09 UTC
Actually I was wrong when I said that all we need is resetting the im context: I didn't see this other bit of code:

...
/* Allow input methods to internally handle a key press event.
 * If this function returns TRUE, then no further processing should be done
 * for this keystroke. */
if (gtk_im_context_filter_keypress (GTK_TEXT_VIEW(view)->im_context, event))
	return TRUE;
...


For which we would need access to the im_context of the view
Comment 20 Christian Dywan 2010-01-25 14:55:43 UTC
Did you consider calling the parent key press instead, to let the GtkTextView handle it?
Comment 21 Paolo Borelli 2010-01-25 15:00:53 UTC
I don't think that would work: basically we need to know if the input method handled the keypress (in particular <return> in this case) and otherwise override default textview's keypress handler

see https://bugzilla.gnome.org/show_bug.cgi?id=154039
Comment 22 Paolo Borelli 2010-01-25 15:05:02 UTC
btw, maybe an alternative to solve the last issue would be to provide a specific textbuffer signal for when a newline is added, similar to the backspace signal: that way we could use that extension point to manage autoindentation without having to jump through oops inside the key-press-handler
Comment 23 Matthias Clasen 2010-02-08 18:27:13 UTC
Review of attachment 152220 [details] [review]:

I think this would be better off as a patch to GtkIMMultiContext. If you make it just have no slave when the id is 'none', things should work out smoothly in both GtkEntry and GtkTextView. 
You'll have to update gtk_im_multicontext_append_menuitems at the same time.

::: gtk/gtktextview.c
@@ +1287,3 @@
 
+static GtkIMContext*
+gtk_text_view_create_im_context (GtkTextView *text_view)

I think I would prefer the method to set the im_context in the text_view and return void.

@@ +3039,3 @@
       g_free (priv->im_module);
       priv->im_module = g_value_dup_string (value);
+      if (g_strcmp0 (priv->im_module, "none"))

please use == 0 / != 0 for strcmp and the like

@@ +4241,3 @@
+    {
+      GtkTextViewPrivate *priv = GTK_TEXT_VIEW_GET_PRIVATE (text_view);
+      if (!g_strcmp0 (priv->im_module, "none"))

== 0 here too
Comment 24 Matthias Clasen 2010-02-08 19:24:10 UTC
> I think this would be better off as a patch to GtkIMMultiContext.

I've done this now.
Comment 25 Matthias Clasen 2010-02-09 14:50:42 UTC
For the remaining problems, bug 592405 seems to be a better approach
Comment 26 Paolo Borelli 2010-02-09 20:46:53 UTC
We briefly discussed the approach of bug 592405 to prepare a patch, but an issue came up: we also need to be able to handle shift+enter, ctrl+enter, shit+tab etc... how would we do that? One thing that comes to mi mind is add e.g. "indent/unindent" action signals to gtksourceview and bind them to tab and to shift+tab and then in the default handler do a g_signal_emit(buffer, "tab"), but I am not sure that 1) would work, 2) is good practice
Comment 27 Matthias Clasen 2010-02-09 21:14:40 UTC
I'd propose to look at parametrizing the signals, similar to how it is done for move-cursor
Comment 28 Paolo Borelli 2010-05-02 15:58:54 UTC
It's been a while since we discussed this, but Javier asked me to add a comment about the current state and viable solutions. 

I think for the time beeing the easiest way is to add a method that proxies

gtk_im_context_filter_keypress (GTK_TEXT_VIEW(view)->im_context, event)

and document that when connecting to key-press-event in GtkTextView you are supposed to call that method from your handler.


Adding such method does not rule out better exetension points in the future and would be needed in any case as long as a program can connect to key-press-event (which I do not think will go away any time soon)
Comment 29 Matthias Clasen 2010-05-04 20:59:19 UTC
I liked the other approach better, but if this is going to work better for you, lets just do that then.
Comment 30 Javier Jardón (IRC: jjardon) 2010-05-05 00:14:21 UTC
Created attachment 160308 [details] [review]
Added all the required api

Patch to add all the required api as discussed in the 04052010 meeting
Comment 31 Javier Jardón (IRC: jjardon) 2010-05-05 01:16:59 UTC
Comment on attachment 160308 [details] [review]
Added all the required api

committed a bit improved patch (added some documentation with the help of mclasen) in:
master 7692a427a6d27f438e18dc3555a91eef2f4205ff
gtk-2-22  fc35cd9bfe63b777c9c42466cf4591d8ba081554