With gseal merged in trunk this is now critical to fix.

Also, an accesor to ->im_context is needed too

No. the im context is not part of the public api

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

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.

(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.

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

(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

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

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.

Created attachment 152022 [details] [review]
Implement gtk_reset_im_state

Pablo, what do you think about this function?

Note that GTK_ENTRY()->im_context is needed too (See bug #595791)

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.

(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.

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.

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.

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.

Created attachment 152222 [details] [review]
Expose gtk_text_view_reset_im_context #2

Re-uploading a complete patch. Sorry.

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

Did you consider calling the parent key press instead, to let the GtkTextView handle it?

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

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

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

> I think this would be better off as a patch to GtkIMMultiContext.

I've done this now.

For the remaining problems, bug 592405 seems to be a better approach

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

I'd propose to look at parametrizing the signals, similar to how it is done for move-cursor

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)

I liked the other approach better, but if this is going to work better for you, lets just do that then.

Created attachment 160308 [details] [review]
Added all the required api

Patch to add all the required api as discussed in the 04052010 meeting

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