GNOME Bugzilla – Bug 417446
Include multi-press-im-context in gtk+
Last modified: 2007-03-18 05:37:54 UTC
This is a follow-up of the gnome support ticket #2565 The multi-press-im-context is particularly useful for devices like mobile phones where it is the usual way to enter text (in short message service for example). Attached is the latest tarball from the private openismus SVN.
Created attachment 84425 [details] Tarball of the latest version
I don't think it would be wise to add the multipress method to GTK+ itself, because it is useless (and confusing) on most systems, with normal keyboards. Also, I anticipate that mobile vendors might want to customize it, and that is easier if it's independent of their GTK+ packages.
The GTK+ developers wanted this bug report so I created it. They haven't said much about their reasons in the support ticket.
Tim Janik seems to like the idea of having this in GTK+ itself anyway, to get more testing, maybe disabled via a configure option. I don't mind too much either way.
From Matthias Clasen: > (...=But since the IM we are talking about here is not language-specific, > and we have been accepting a lot of features to make gtk work in > embedded situations, I have no objections to adding the multipress > IM to gtk itself. OK, I will prepare a patch against Gtk+ svn then attach it here.
Thanks, Johannes.
> OK, I will prepare a patch against Gtk+ svn then attach it here. > Some questions about the Gtk+ integration: - What namespace would you prefer? Currently the methods are name GtkMultiPressImContext but I assume that it would be better to have GtkImContextMultiPress as all other modules are named GtkImContext* - What about the config file that is used to know which buttons to map (on a normal mobile phone "1" = abc1 for example). It is in GKeyFile format but where should I put it in Gtk+? Thanks!
I guess I would prefer GtkIMContextMultipress and gtk_im_context_multipress (or maybe MultiPress and multi_press if you really want to maximize the number of underscores...) If the input method needs a config file, the most natural place for it in svn would be in modules/input next to the source, I'd say. Where do you look for it at runtime ? /etc/gtk-2.0 ? $HOME/.config ?
a couple thoughts on integration/enabling of multi-press from IRC: if a multi-press input method goes into stock gtk, it should be testable by developers of course. but if on a stock desktop it proves to be rather useless and shouldn't be part of the IM menu, a display setting for showing the menu entry defaulting to FALSE could be a solution. a configure.in option could be another one. and/or a configure default to auto-build it only in development branches (like we already do with debugging info). this isn't meant to imply decisions on integration or visibility on menu items just yet.
Realistically, a lot of the IMs we ship with gtk are not testable by developers either, because there is no documentation at all for how they map keypresses to characters, and what you should expect on screen...
> If the input method needs a config file, the most natural place for it in svn > would be in modules/input next to the source, I'd say. > Where do you look for it at runtime ? /etc/gtk-2.0 ? $HOME/.config ? It is currently installed in $(sysconfdir) but /etc/gtk-2.0 looks like a good place to put this in the future. I don't think anyone will ever want to change the behaviour on a per-user base because it depends on the available hardware keys.
Created attachment 84576 [details] [review] Add MultiPress IM Context to gtk+ This patch will add the required files to gtk+ and adjust the Makefile.am. The config file will be installed in $(sysconfdir)/gtk-2.0. I hope I got everything correctly with the namespaces and conventions. I did NOT add anything to build this conditionally as I think the maintainers will know better how they want to do this.
Looks good in general, some comments: - it would be nice if we could start to improve the state of user-level documentation of input methods by including a README.multipress or similar describing "how it works". - stylistic nitpick: we always leave a space before a (, both in for( and in g_free(foo) - return types go on a line by themselves - All uses of bsearch seem to be commented out, still +#include <stdlib.h> /* For bsearch() */ I guess you should either sort the sequence table and use bsearch or remove the commented out parts together with the include + /* GDK_THREADS_ENTER and GDK_THREADS_LEAVE might not be necessary. + * I added them while investigating a crash (fixed elsewhere now, in accept_character(), I believe), + * because I saw them in Movial's sequence-input-module implementation. + */ Yes, they are necessary, since timeouts are executed outside the gdk lock. Just remove the comment. + /* TODO: The delete key does not seem to be handled when we do this. + * danielk: Yes, that's normal and it's not the only one. I'll get to that later. */ Is this planned ? + else if(event->keyval == GDK_Return || event->keyval == GDK_Tab) There are some more keys you may want to check for here: GDK_KP_Enter GDK_ISO_Enter GDK_KP_Tab GDK_ISO_Left_Tab + parent = (GtkIMContextClass *)gtk_im_context_multipress_parent_class; + if(parent && parent->filter_keypress) No need to check parent here. It can't be NULL
> + /* GDK_THREADS_ENTER and GDK_THREADS_LEAVE might not be necessary. > + * I added them while investigating a crash (fixed elsewhere now, in > accept_character(), I believe), > + * because I saw them in Movial's sequence-input-module implementation. > + */ > > Yes, they are necessary, since timeouts are executed outside the gdk lock. > Just remove the comment. Thanks. I'd rather have a comment saying why they are necessary (now possible). I am intolerant of the lack of comments in most GTK+ code. > + /* TODO: The delete key does not seem to be handled when we do this. > + * danielk: Yes, that's normal and it's not the only one. I'll get to > that later. */ > > Is this planned ? No Idea. CCing Daniel.
> it would be nice if we could start to improve the state of user-level > documentation of input methods by including a README.multipress or similar > describing "how it works". I plan to patch the GtkIMContext gtk-doc documentation sometime, based on this: http://www.murrayc.com/blog/permalink/2006/12/22/creating-gtk-input-methods/
> I am intolerant of the lack of comments in most GTK+ code. That may be so, however, the code you are presenting here is heavily over-commented. Not that I think this should block it from going in. But comments expressing uncertainty about how other parts of the very module you want to get this in are working are guaranteed to raise questions during review... > I plan to patch the GtkIMContext gtk-doc documentation sometime, based on this: > http://www.murrayc.com/blog/permalink/2006/12/22/creating-gtk-input-methods/ Thats nice, but it doesn't address my request for _user_ documentation, ie a file describing what key sequences have to create to produce a certain character.
> But comments expressing uncertainty about how other parts of the very > module you want to get this in are working are guaranteed to raise > questions during review... Of course, and thanks. That's what they are there for. > _user_ documentation, There was a README in the original tarball. I'll upload it separately so it's more obvious.
Created attachment 84665 [details] README README for the multipress input method.
Thanks, having that README next to the code would be a good.
Created attachment 84672 [details] [review] Improved version of the patch mentioned above - fixed coding style - added GDK_KP_Enter, etc. - Removed unused code (bsearch) - Added REAME.multipress (see Murray's comment) I am not sure about the TODO you mention. I haven't written the code and I don't really know what is needed there. Maybe Daniel could tell us.
Created attachment 84673 [details] [review] Another improved version Same as above but also fixes the parent checking issue. Sorry for missing that one before.
Looking much better now, thanks. Murray, do you want to leave your email address in the README ?
> do you want to leave your email address in the README ? If that's OK. (Not for copyright, but just as advertising)
Sure its ok. Just asking, because normally people want their email addresses _not_ exposed...
Johannes, can you commit this yourself, or should I do it ?
By the way, we should still replace the object-data hack with a function call to set the current input method programatically. But I guess we can do that later.
2007-03-18 Matthias Clasen <mclasen@redhat.com> * modules/input/gtkimcontextmultipress.[hc]: * modules/input/im-multipress.conf: * modules/input/immultipress.c: * modules/input/README.multipress: Add the multipress input method. (#417446, Johannes Schmid, Murray Cumming) * modules/input/Makefile.am: Glue