Created attachment 100573 [details] [review]
proposed patch

The parts for adding the x setting and gtk setting are looking fine.

Some comments on the immodule part:

_gtk_im_module_get_default_context_id() is called exactly once, and you are replacing that call. Therefore there is no need to add a new
_gtk_im_module_get_default_context_id_from_settings() function, simply
add the client_window argument to the existing function and add the
settings-handling code there.

(Not your fault, but) the return type of both get_default_context_id 
variants should be gchar *, not const gchar *, since they return 
allocated memory.

+  if (client_window == NULL)
+    g_warning("No client window is set.");
+  if (client_window == NULL ||
+      !GDK_IS_DRAWABLE (client_window))
+    return _gtk_im_module_get_default_context_id (locale);

I'd get rid of the warning, and simply do this as

  if (client_window != NULL) 
    {
      ... settings-handling code ...

      return context_id;
    }

  ... old code from get_default_context_id ...


If you want to keep a check for client_window, do a 

  g_return_val_if_fail (client_window == NULL || GDK_IS_DRAWABLE (client_window), NULL);

But I don't think that is necessary.


One thing I'd reconsider when combining the two get_default_context_id
functions into a single one is whether you really want the setting to
override the envvar. In any case, the priorities need to be documented,
perhaps in the paragraph on the GTK_IM_MODULE envvar in 
docs/reference/gtk/running.sgml and in the docs for the gtk-im-module
setting.


+      /* destroy the slave and the global_context_id because it might
+       * be set the different thing from XSETTINGS
+       */

Setting the global_context_id to a value that depends on the screen on which 
an im context happens to lie does not really work. Also, randomly freeing
global_context_id is problematic, considering the following:

     multicontext->context_id = global_context_id;
  
You probably need to switch to giving each multicontext its own allocated
context_id. Don't forget to free it in finalize.

(In reply to comment #2)
> _gtk_im_module_get_default_context_id() is called exactly once, and you are
> replacing that call. Therefore there is no need to add a new
> _gtk_im_module_get_default_context_id_from_settings() function, simply
> add the client_window argument to the existing function and add the
> settings-handling code there.

I have been worrying about the ABI breakage since it was declared as public API. even though the function name has a prefix '_'. if that change is acceptable, I'll go that way.

> (Not your fault, but) the return type of both get_default_context_id 
> variants should be gchar *, not const gchar *, since they return 
> allocated memory.

You're right.

> I'd get rid of the warning, and simply do this as

No problem. I should do that, it was just to check if there are any cases that applications goes through there without client_window.

> If you want to keep a check for client_window, do a 
> 
>   g_return_val_if_fail (client_window == NULL || GDK_IS_DRAWABLE
> (client_window), NULL);
> 
> But I don't think that is necessary.

I agree.

> One thing I'd reconsider when combining the two get_default_context_id
> functions into a single one is whether you really want the setting to
> override the envvar. In any case, the priorities need to be documented,
> perhaps in the paragraph on the GTK_IM_MODULE envvar in 
> docs/reference/gtk/running.sgml and in the docs for the gtk-im-module
> setting.

Hmm, ok. well, I was thinking of overriding GTK_IM_MODULE with XSETTINGS because  in most distributions, they set GTK_IM_MODULE to ensure bringing up IM they want.  but indeed this entirely makes GTK_IM_MODULE unusable. so it may be good to be changed outside gtk+. I mean it follows to XSETTINGS if GTK_IM_MODULE isn't set.

> Setting the global_context_id to a value that depends on the screen on which 
> an im context happens to lie does not really work. Also, randomly freeing
> global_context_id is problematic, considering the following:
> 
>      multicontext->context_id = global_context_id;
> 
> You probably need to switch to giving each multicontext its own allocated
> context_id. Don't forget to free it in finalize.
> 

Ok, thanks for your comment.

Created attachment 100693 [details] [review]
proposed patch: take 2

attached the re-worked patch, including a fix of crash issue and use GtkSettings to deliver the changes from IM menu instead of global_context_id.

> I have been worrying about the ABI breakage since it was declared as public
> API. even though the function name has a prefix '_'. if that change is
> acceptable, I'll go that way.

_-prefixed functions are not exported from the .so, so there is no danger of anybody outside gtk using it.


/**
  * _gtk_im_module_get_default_context_id:
+ * @widget: a #GtkWidget
  * @locale: a locale id in the form 'en_US'

That should be 

@client_window: the client window 

instead.


  gtk_im_multicontext_set_slave (multicontext, NULL, TRUE);
+  if (multicontext->priv->settings)
+    {
+      g_signal_handlers_disconnect_by_func (multicontext->priv->settings,
+					    G_CALLBACK (gtk_im_multicontext_changes_im_module),
+					    multicontext);
+    }


GTK+ coding style omits the {} if the body is a single statement.


 if (GDK_IS_DRAWABLE (window) &&
+      multicontext->priv->client_window != window)
+    {
+      GdkScreen *screen = gdk_drawable_get_screen (GDK_DRAWABLE (window));
+
+      if (multicontext->priv->settings)
+        {
+          g_signal_handlers_disconnect_by_func (multicontext->priv->settings,
+						G_CALLBACK (gtk_im_multicontext_changes_im_module),
+						multicontext);
+	}
+      if (screen)
+        multicontext->priv->settings = gtk_settings_get_for_screen (screen);
+      else
+	multicontext->priv->settings = gtk_settings_get_default ();
 

It would perhaps be nicer to only do the full re-get-settings-and-reconstruct-slave thing if the settings actually changed ?
many different windows can give you the same settings, thus the same slave...


Looks good in general, otherwise. Have you tested it a bit ?

One thing that is still needed is some documentation about the priorities of
GTK_IM_MODULE envvar vs gtk-im-module setting

Created attachment 100749 [details] [review]
proposed patch: take 3

(In reply to comment #5)
> _-prefixed functions are not exported from the .so, so there is no danger of
> anybody outside gtk using it.

Ok, I see.

> /**
>   * _gtk_im_module_get_default_context_id:
> + * @widget: a #GtkWidget
>   * @locale: a locale id in the form 'en_US'
> 
> That should be 
> 
> @client_window: the client window 
> 
> instead.

Fixed.

> GTK+ coding style omits the {} if the body is a single statement.

I have looked through the whole code in the patch and corrected them.

> It would perhaps be nicer to only do the full
> re-get-settings-and-reconstruct-slave thing if the settings actually changed ?
> many different windows can give you the same settings, thus the same slave...

Sure. fixed.

> Looks good in general, otherwise. Have you tested it a bit ?

Yes, I have tested with some applications and among some widgets, and looks good so far.

> One thing that is still needed is some documentation about the priorities of
> GTK_IM_MODULE envvar vs gtk-im-module setting

I have added some in the latest patch though, is it close to what you expect?

Created attachment 100758 [details] [review]
proposed patch: take 4

Sorry, found a bug that doesn't apply the change from the input method menu.

Hmm, that looks wrong to me.

You are right, that calling gtk_settings_set_string_property
in activate_cb will not work as intended, since the setting is global for the application (or at least shared by all windows on a screen) whereas the input method menu is supposed to only change a specific im context.

You probably need to add something like gtk_im_multicontext_set_context_id()
and call that from the activate_cb. That function would then have to remember that the context was explicitly set, so that it knows to ignore changes of the setting.

We probably also need to change the meaning of the "Default" menutitem to not mean
"imcontextsimple" but rather "use the setting".

That being said, I am not sure why you thought you needed the changed_from_menu
boolean. The (wrong, as explained above) call to gtk_settings_set_string_property
should set the setting with application priority, which is higher than xsettings priority, so a change from the menu should have an effect, even without the 
changed_from_menu boolean.

(In reply to comment #9)
> Hmm, that looks wrong to me.
> 
> You are right, that calling gtk_settings_set_string_property
> in activate_cb will not work as intended, since the setting is global for the
> application (or at least shared by all windows on a screen) whereas the input
> method menu is supposed to only change a specific im context.

No, actually the change from the IM menu effects to all the IM contexts on the application, and global_context_id was needed to do that. what I made mistake in the previous patch was actually the change is just ignored when GTK_IM_MODULE is set, because now GTK_IM_MODULE takes a priority.

> You probably need to add something like gtk_im_multicontext_set_context_id()
> and call that from the activate_cb. That function would then have to remember
> that the context was explicitly set, so that it knows to ignore changes of the
> setting.

changed_from_menu boolean was supposed to do it.

> We probably also need to change the meaning of the "Default" menutitem to not
> mean
> "imcontextsimple" but rather "use the setting".

Indeed, current behavior may causes misleading if one changes the IM context from the menu and changes the IM context through XSETTINGS after that. in this case, it doesn't effect to such application. I don't know if this is correct behavior. but actually it didn't.  However it's still useful that I can see from the menu which IM is currently selected. so I'll miss that "feature" if we change "Default" to "use the setting" in the label or the behavior.

(In reply to comment #10)
> That being said, I am not sure why you thought you needed the changed_from_menu
> boolean. The (wrong, as explained above) call to
> gtk_settings_set_string_property
> should set the setting with application priority, which is higher than
> xsettings priority, so a change from the menu should have an effect, even
> without the 
> changed_from_menu boolean.

Right but as I mentioned above, I needed to ignore GTK_IM_MODULE envvar explicitly for the change from the menu.

I don't like the changed_from_menu thing. I'd much rather have a very simple behaviour a la:

if GTK_IM_MODULE is set and yields a im context, use that

otherwise, if the gtk-im-module setting yields a im context, use that

else determine the im context from the locale

And then just make the menu set the setting; we'll have to add a menu item for 
"System" (and probably have to rename the "Default" to be "None" instead). 
The "System" menuitem must unset the setting. (Unfortunately, there is no way to "unset" an application-set setting currently, but that is fixable)


Doing things this way has 2 consequences which you may not find ideal:

1) The envvar wins, period. Don't like it ? Don't set it...

2) If != System has been selected in the menus, the app does not react to global settings changes until the menu is set back to System.

But it has the advantage of giving an easily understandable algorithm.

By the way, where is the GTK_IM_MODULE environment variable documented? I've seen it mentioned, but I never got it to work.

http://library.gnome.org/devel/gtk/unstable/gtk-running.html

Created attachment 101042 [details] [review]
my own patch

Ok, unsetting a setting turned out to be harder than I thought, so here is a patch that does things a bit differently. Notable differences to your patch:

- uses only a single signal handler for the setting, not one per context
- both setting and menu set global_context_id and the last one to set it wins
- I've made _gtk_im_module_get_default_context_id actually return const again,
  to simplify memory management

I haven't tested changes of the setting, but changing from the menu works fine.
If it is necessary to reset all contexts when the setting changes (instead of 
waiting for the next focus-in), then we must keep all contexts in a list and iterate over them in im_context_setting_changed().

(In reply to comment #15)
> I haven't tested changes of the setting, but changing from the menu works fine.
> If it is necessary to reset all contexts when the setting changes (instead of 
> waiting for the next focus-in), then we must keep all contexts in a list and
> iterate over them in im_context_setting_changed().
> 

I have tried your patch, but unfortunately gtk+ applications got a crash after sending the change through XSETTINGS. I'll look into it.

There is three or four problems in your patch:

1. as I described in the previous comment, the following code has to check if global_context_id is NULL:

  /* If the globalcontext type is different from the context we were
   * using before, get rid of the old slave and create a new one
   * for the new global context type
   */
  if (!multicontext->context_id ||
      strcmp (global_context_id, multicontext->context_id) != 0)
    gtk_im_multicontext_set_slave (multicontext, NULL, FALSE);

2. when the applications is closing, the warnings happens like:

Gdk-CRITICAL **: gdk_drawable_get_screen: assertion `GDK_IS_DRAWABLE (drawable)' failed.

3. gtk-im-module isn't applied at startup time if it has the different settings to the default thing came from current locale. this issue happens global_context_id already has something when the client window is set.

4. I'm not sure what the behavior is useful and people are expecting, but if people explicitly changes the IM settings from the menu say, XSETTINGS stuff shouldn't affects to such applications IMHO. someone may want to keep current settings for some reason, otherwise they could just change it through XSETTINGS.

Ok, thanks for testing.

For 2.,
'lll
+  screen = gdk_drawable_get_screen (GDK_DRAWABLE (window));
+  if (screen)
+    settings = gtk_settings_get_for_screen (screen);
+  else
+    settings = gtk_settings_get_default ();

This part of the set_client_window() function needs to be wrapped in
if (window) 
  {
   }


For 3., you probably need to reset global_context_id in the 

  if (!connected) 
    {
    }

block in the same function.


For 4., I don't really care at all, since as far as I am concerned, the menu will never be visible. Anyway, to implement the behaviour you propose, I'll introduce another static variable, user_context_id, and have the menus set that,
and take it into account when calculating the new id.


Let me produce a new patch with those fixes.

Created attachment 101127 [details] [review]
new patch

Thank you for updates. it works fine as expected.

2007-12-17  Matthias Clasen  <mclasen@redhat.com>

        * gtk/gtksettings.c: Add a gtk-im-module GTK setting
        * gdk/win32/gdkproperty-win32.c:
        * gdk/x11/gdksettings.c: ...and back it by a Gtk/IMModule X setting.

        * gtk/gtkimmodule.[hc]:
        * gtk/gtkimmulticontext.[hc]: When determining the default context,
        look at the gtk-im-module setting, and listen for changes to the
        setting.  (#502446, Akira Tagoh)

Hi Matthias Clasen,  Akira TAGOH,

I tested it. I found gtk does not destroy im context immediately, when the im module is changed by xsettings. So some IMs will not exit immediately for those alive clients. And I must click every input widgets on the screen, and then the IM will exit. It is very weird. (especially there are many input widgets on the screen.) Could you fix this problem? 

Haung, could you file a bug or reopen this bug? Maybe you could attach a test case.

Filed bug 528502.

I don't quite understand what this does. It's documented as 
"Which IM module should be used by default."
http://library.gnome.org/devel/gtk/unstable/GtkSettings.html#GtkSettings--gtk-im-module

Does that mean I can change the IM at runtime, if the user has never chosen an IM  from the context menu? Can I change it again afterwards?

Yes to both questions.

If you go to the context menu you'll see that the first entry is "System (...)", which will bring you back to the im selected by the setting.