Created attachment 10465 [details] [review]
patch to avoid crash at XIMserver's absence and do mimumam thing

Moving input method related bugs to input-methods component

If XOpenIM fails, gtk_im_context_xim_new() will
return NULL and GTK+ will fall back to the default 
input method. (tested this by setting

  XMODIFIERS=@im=foo
)

If XCreateIC() fails [don't see how this could happen,
but the code tries to handle it anyways] then the input 
context should just do nothing ... won't work, but won't crash.

Maybe you could look at this again?

Please reopen when adding more information

When XOpenIM fails, context_xim->im_info is set
NULL but context_xim is not. GTK+ does not give
it up and goes until it crashes at the point shown
below. (I tried XMODIFIERS=@im=none but it
samely crashes.) Maybe this could be Solaris specific, due to
different XIM implementation?
but from the code, I don't see how GTK+ can give
it up and go to the default. Reopening.

(dbx) run
Running: testtext 
(process id 5086)

(testtext:5086): Gtk-WARNING **: The "invisible" property on
GtkTextTag is not supported for GTK 2.0, it will be added in a future
release. see http://bugzilla.gnome.org bug #66194 for status.
Reading ximp40.so.2
signal SEGV (no mapping at the fault address) in
gtk_im_context_xim_get_ic at line 872 in file "gtkimcontextxim.c"
  872         if ((context_xim->im_info->style & PREEDIT_MASK) ==
XIMPreeditCallbacks)
(dbx) print context_xim
context_xim = 0x8c538
(dbx) print context_xim->im_info
context_xim->im_info = (nil)
(dbx) where 10
=>[1] gtk_im_context_xim_get_ic(context_xim = 0x8c538), line 872 in
"gtkimcontextxim.c"
  [2] gtk_im_context_xim_set_cursor_location(context = 0x8c538, area =
0xffbeda58), line 463 in "gtkimcontextxim.c"
  [3] gtk_text_view_update_im_spot_location(0x76bd0, 0xffbedb20,
0xffbedb20, 0x1, 0x1a4, 0x0), at 0xff0b4638
  [4] changed_handler(0x786b8, 0x0, 0xf, 0xf, 0x49f78, 0x77db0), at
0xff0b7454
  [5] g_closure_invoke(0x766d0, 0x0, 0x4, 0xffbedd88, 0xffbedc2c,
0x4), at 0xfecaff1c
  [6] signal_emit_unlocked_R(0x4a118, 0x0, 0x786b8, 0x0, 0xffbedd88,
0xfecf1da8), at 0xfecc3e08
  [7] g_signal_emit_valist(0x203, 0xc, 0x4a118, 0xffbedfc4,
0xfeceea8c, 0xfecf1da8), at 0xfecc3228
  [8] g_signal_emit(0x786b8, 0x87, 0x0, 0x0, 0xf, 0xf), at 0xfecc356c
  [9] gtk_text_layout_set_cursor_visible(0x786b8, 0x1, 0x0, 0x1f4,
0x1dc, 0xa2), at 0xff0a672c
  [10] gtk_text_view_focus_in_event(0x76bd0, 0xffbee5bc, 0x4a210,
0x2f, 0x35, 0x71ed0), at 0xff0b884c

Looks like some changes in gtk-2-0 never got merged to HEAD.

Oh, I see, wasn't that. Rather, with the multihead changes
XOpenIM isn't called until the client window is set, so
at that point we can't fail to load the input method.

With that in mind, I think the general idea of your code
is right, but:

+  static XIM im = NULL;		/* should open only once */

Is not right, and you have an extraneous diff from another
bug:
@@ -361,7 +364,7 @@
 
   XKeyPressedEvent xevent;
 
-  if (!ic)
+  if (event->type != GDK_KEY_PRESS)
     return FALSE;
 
   xevent.type = (event->type == GDK_KEY_PRESS) ? KeyPress : KeyRelease;
@@ -382,7 +385,13 

Finally, I think it's good to give a little nicer warning than:

+      if (!im)
+	g_warning ("can not open input methods.");
+

Maybe

 g_warning ("Unable to open XIM input method, falling back to
XLookupString()")

Created attachment 10942 [details] [review]
code diffs from the version containing a fix for 81759

Attaching a new patch. Apart from the changes you
suggested, I also changed a wanring for XSetLocaleModifers() failure
so that it sounds more consistent to that for XOpenIM.

For ChangeLog,
	* modules/input/gtkimcontextxim.c
	(get_im): modify a warning when XSetLocaleModifiers() fails, and
	add a warning when XOpenIM() fails
	(gtk_im_context_xim_filter_keypress): use XLookupString when xic
	is not available. (#90661)

+	g_warning ("Unable to set locale modifiers in XSetLocaleModifiers()"); 

"in" here is a little odd. "with" maybe?

+  if (!context_xim || !context_xim->client_window ||
!context_xim->im_info)
+    return (XIC)0;

We shouldn't be checkiung for things that can't happen;
context_xim can't be NULL here. And the client_window check
is already immediately below. Why not simply change:

-  if (!context_xim->ic && context_xim->client_window)               
           

To:

   if (!context_xim->ic && context->im_info)

[ im_info can never be set unless client_window is set ]

Created attachment 10987 [details] [review]
new diff including changes for the above review comment

Please go ahead and commit to both branches.

Err, go ahead and commit to HEAD, not relevant to stable.

commit to HEAD - marking fixed.

This was back by recent changes. Can we commit the change below?
Reopening..

retrieving revision 1.29
diff -u -r1.29 gtkimcontextxim.c
--- gtkimcontextxim.c   15 Oct 2002 21:27:45 -0000      1.29
+++ gtkimcontextxim.c   18 Oct 2002 23:27:43 -0000
@@ -454,7 +454,8 @@
   if (context_xim->client_window)
     {
       context_xim->im_info = get_im (context_xim->client_window,
context_xim->locale);
-      context_xim->im_info->ics = g_slist_prepend
(context_xim->im_info->ics, context_xim);
+      if (context_xim->im_info)
+       context_xim->im_info->ics = g_slist_prepend
(context_xim->im_info->ics, context_xim);
     }
 }

Don't you have to also have context->im_info in the first
half of the function?

Maybe it would be better to make get_im() always return
a structure, and then handle info->im == NULL?  
Seems a bit cleaner and also avoids us repeatedly trying
to open the input method and failing.

[ Hmm, I guess we really should be dealing 
  with XRegisterIMInstantiateCallback to handle the case
  where the app is started before input method and
  XNDestroyCallback to handle the case where the input
  method terminates while the app is still running ]

Created attachment 12133 [details] [review]
get_im() always returns a structure so check info->im whereever it's used

Attaching a patch according to your suggestion.
verified it works.

	* modules/input/gtkimcontextxim.c (get_im):
	Fix #90661: get_im () always returns a strucuture and handles
	info->im == NULL? whereever info->im is used.

Please let me know if okay to commit to the HEAD.

Hmm, what I had imagined was more along the lines of:

 - Not calling setup_im if im_info->im was NULL.
 - Return NULL immediately from get_ic_real if
   im_info->im was NULL.

I'm not sure it makes sense to run a bunch of code that
has no meaning in the failed-to-open-im case then do
nothing.

Created attachment 12230 [details] [review]
2nd patch - check info->im at the beginnings of set_im() and get_ic_real()

Looks good to me.

commit to the HEAD.
  Fix #90661: add im_info->im switch at the top of   
setup_im() and
  get_ic_real().