Re: [gtk-list] [patch] support XIM

[Owen Taylor <owt1 cornell edu>]

matsu@arch.comp.kyutech.ac.jp (Takashi Matsuda) writes:

> Feature:
>   Followings are the main feature of this patch.
> 	* support XIM protocol.
> 	* GtkEntry widget support Over-The-Spot and Root style input.
> 	* input style is configurable by command-line option.

Very nice! I thought about writing such support but I didn't
really know enough about it (and it seemed pretty complex.)

I've tried it out a bit, and I can input Japanese text using kinput2,
but I closing the window afterwards causes the program to hang. (It
might possibly be a problem with my mainloop patch.)  I'll try to
figure out what is going on when I get the time.  (maybe over the
weekend). Displaying characters stills seems a bit rough as well.

Just a few comments about your patch - it mostly looks very good:

* You've made all your changes as

#if 1
[ new stuff ]
#else
[ old stuff ]
#endif

We don't want every old version of the code hanging around in the
source (that's what CVS is for). This makes reading the code very
difficult. Just delete the old stuff. It's clear from the patch what
has been changed.

* The --xim-preedit and --xim-status arguments are very complicated.
  I don't really understand XIM very well, but are all these options
  really useful for the user?

* You've added the bindings
  C-h and M-h as delete-backward-character and delete-backward-word
  to the entry widget. Are these standard bindings? (The functionality
  is already there as Backspace and C-w.) There's a good reason not to 
  add every possible key binding - unrecognized keys should be let 
  propagate to the toplevel window so they can act as shortcuts.


>   Furthermore, this patch includes several changes which are useful without
>   XIM too.
> 	* copy and paste with other clients by compound text.

(In gtkentry.c)
+	  gtk_selection_convert (widget, GDK_SELECTION_PRIMARY,
+				 ctext_atom, event->time);

This breaks pasting selections from other clients that don't support
the COMPOUND_TEXT target. Of course, some programs, like rxvt,
are pretty much hard to get right - they don't support the
(mandatory) TARGETS target, and they don't support TEXT. But you
should at least ask for TEXT, which gives clients the option
of giving a reply of type STRING or COMPOUND_TEXT. 

It might be even better to do something like:

Ask for TARGETS
  if this succeeds, ask for the best of COMPOUND_TEXT, TEXT, or STRING
  if this fails, ask for STRING.

the "selection_received" handler would look something like : (untested code)

if (selection_data->target == targets_atom)
  {
    GdkAtom *atoms;
    gint num_atoms;
    GdkAtom best_target;

    if (selection_data->length >= 0)
      {
	atoms = (GdkAtom *)selection_data->data;
        num_atoms = selection_data->length / sizeof (GdkAtom);
         
        [ find the best returned taget ]
      }
    else
      best_target = GDK_SELECTION_TYPE_STRING;
	
    gtk_selection_convert (widget, GDK_SELECTION_PRIMARY,
		           best_target, GDK_CURRENT_TIME);

    return;
  }

This will slow things down a bit, but isn't too bad in my experience.


(Again from gtkentry.c)

+  else if (selection_data->type == text_atom)
+    type = TEXT;
[..]
+  switch (type)
+    {
+    case STRING:
+    case TEXT:

Returning the type as TEXT is illegal. I don't see why we should
assume that the selection owner meant STRING. I think this
response should be ignored, as if the selection owner had responded
with some other inappropriate type. (On the other hand, if there
are actually programs out there that behave in this broken manner,
than it might make sense to support such behavior).


> 	* gtk_selection_owner_set calls gdk_selection_owner_set as well
> 	  new owner and old owner is same (but does not send selection_clear
> 	  event).

I'm not quite sure I understand. Why do you want
gdk_selection_owner_set to be called when the widget isn't changed?  I
suppose it does change the timestamp, but is this needed?
(I'm not saying that this change isn't right, I just would
like to understand why.)

Anyways, thanks for doing this work!

Regards,
                                        Owen