p-e-w / finalterm

At last – a modern terminal emulator (NO LONGER MAINTAINED)
http://finalterm.org
GNU General Public License v3.0
3.84k stars 179 forks source link

Added proper Unicode input handling #317

Closed hamzadis closed 10 years ago

hamzadis commented 10 years ago

The current keyboard input uses Gdk.EventKey.str to capture the entered keys. While this works fine for the most common use cases, it is not sufficient for advanced input. International keyboard layouts often require multiple key presses to enter a single character. For example, my keyboard layout requires me to enter input combination of alt+3, space to produce the character ^. Other examples include various umlauts or even more complex characters. Hexcode combinations work as well now (ctrl+shift+U, followed by a hexcode, followed by enter).

This patch makes limited use of the gtk input methods api to capture the entered input. The patch doesn't fully implement the IM functionality, there is no pre-enter preview and the surrounding text callbacks are not implemented, but it's better than nothing.

To make it work, I had to move the input handling above the user key command handler. This might break something there, so you might want to check that.

Edit: seems like this might be issue #10

p-e-w commented 10 years ago

Thank you for solving that riddle. I would have never guessed it is necessary to manually define an input context; I had always assumed that would be built into the event mechanism already.

Two remarks:

  1. The input method should definitely not override user configuration; if the user wishes to make a key combination do a certain thing, we should not prevent him from doing that. Therefore, I believe the call to filter_keypress in on_key_press_event should be moved to line 233, where send_text_to_shell currently is.
  2. In the same vein: Since send_text_to_shell is now invoked by the IMContext's commit signal, shouldn't the call to send_text_to_shell in on_key_press_event be removed? Please correct me if I misunderstood something.
p-e-w commented 10 years ago

Minor quibble: There are two typos in the commit message ("propper" -> "proper", "handlind" -> "handling"). Not a big deal though.

hamzadis commented 10 years ago

About your remarks:

  1. The user configuration handling currently captures the enter key event making hex entry impossible if the IM filtering is placed after the keybind handling. There might be other issues as well there, I only noticed that one though. The IM filtering however only captures events it finds interesting i.e. things that produce a character. Other keys are only captured in special contexts, like with the enter key above.
  2. The other send_text_to_shell has to stay as the IM api doesn't capture all events. These still have to be manually inserted. One example is the backspace key.
  3. I noticed the typos, but only after I created the pull request, and now I can't change it I think. Way to rub salt in my wounds there. :P
p-e-w commented 10 years ago

The user configuration handling currently captures the enter key event making hex entry impossible if the IM filtering is placed after the keybind handling.

I see... the proper way would be to introduce another state for keybindings that lets them react to whether the input method is currently inside an input sequence or not, but that would be overkill IMO. Sadly, some relatively interesting keys, such as ^ on my German keyboard, can not be bound by the user configuration in the current state.

but only after I created the pull request, and now I can't change it I think

git commit --amend -m "Added proper Unicode input handling"
git push -f origin master

:)


Another idea, I believe

private bool on_key_release_event(Gdk.EventKey event) {
    im_context.filter_keypress(event);
    return true;
}

should be changed to

private bool on_key_release_event(Gdk.EventKey event) {
    return im_context.filter_keypress(event);
}

so that other handlers can still capture key_release_event if necessary. What to you think?

hamzadis commented 10 years ago

I had an idea on how to handle this earlier. The IM context emits signals for when it detects a pre-edit, and when that edit is completed. So I have it set a flag that disables the keybind handling. This allows the IM filtering to be moved below the keybind handling giving it a lower priority in normal situations. I don't like using flags like this as it feels wonky a lot of the times, but it seems elegant enough in this situation.

About the typo, I knew of git amend but read somewhere that it doesn't work with github pull requests. Reading it now, I see he only mentioned pulls, not pull requests. Thank you for the tip, I'm not much of a git/github aficionado yet.

p-e-w commented 10 years ago

Sorry for the delay in merging; I'm a little busy at the moment but will try my best to get to it this weekend. :8ball:

p-e-w commented 10 years ago

Merged at last! Thank you for your patience. I tested this thoroughly and it worked perfectly no matter what I tried. Therefore, I think it is fair to say that you fixed #10 :)