jfx icon indicating copy to clipboard operation
jfx copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

8305418: [Linux] Replace obsolete XIM as Input Method Editor

Open tsayao opened this issue 2 years ago • 104 comments

This replaces obsolete XIM and uses gtk api for IME. Gtk uses ibus

Gtk3+ uses relative positioning (as Wayland does), so I've added a Relative positioning on InputMethodRequest.

Screencast from 17-09-2023 21:59:04.webm

Progress

  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue
  • [ ] Change must be properly reviewed (3 reviews required, with at least 1 Reviewer, 2 Authors)

Issue

  • JDK-8305418: [Linux] Replace obsolete XIM as Input Method Editor (Enhancement - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1080/head:pull/1080
$ git checkout pull/1080

Update a local copy of the PR:
$ git checkout pull/1080
$ git pull https://git.openjdk.org/jfx.git pull/1080/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1080

View PR using the GUI difftool:
$ git pr show -t 1080

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1080.diff

Webrev

Link to Webrev Comment

tsayao avatar Apr 02 '23 20:04 tsayao

:wave: Welcome back tsayao! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

bridgekeeper[bot] avatar Apr 02 '23 20:04 bridgekeeper[bot]

I am a native speaker of Chinese. I tested it using the ibus pinyin input method on Ubuntu. I found some problems:

  • Input method candidate window sometimes appear in the wrong location:

image

  • When the focus moves away from a text field, the console will continuously output an assertion error log:
(java:3296): Gtk-CRITICAL **: 20:18:35.210: gtk_im_context_set_cursor_location: assertion 'GTK_IS_IM_CONTEXT (context)' failed

Glavo avatar Apr 03 '23 12:04 Glavo

Thank you for the feedback. Will look into it.

tsayao avatar Apr 03 '23 12:04 tsayao

@Glavo the selection window should appear in the right location now.

tsayao avatar Apr 04 '23 23:04 tsayao

I'm sure I am missing details of other languages. @kevinrushforth Is there anyone at Oracle that could provide guidance on language variations so I can make this right?

tsayao avatar Apr 04 '23 23:04 tsayao

It appears to be in the correct position, but another issue still exists.

Glavo avatar Apr 05 '23 15:04 Glavo

Yeah, I know many things are probably no working. The plan is to get feedback and keep fixing until it's all good. I only speak/read latin-based languages so I have never needed IME.

tsayao avatar Apr 05 '23 20:04 tsayao

⚠️ @tsayao This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk[bot] avatar Apr 07 '23 15:04 openjdk[bot]

I see that this PR changes the behavior of dead keys. You may be aware of this but I thought I would write this up for others to review.

In the past when the user pressed a dead key they got no feedback until they pressed the next key at which point the final character would appear. With this PR when the user presses a dead key they get a preview of the diacritic which looks the same as previewing text from an IME.

This is a change from the old JavaFX behavior but not necessarily a bug. In fact it matches the new behavior of Ubuntu. I'm not sure when they made the switch but in Ubuntu 18 there was no diacritic preview and in Ubuntu 22 there is. It also matches the way the Mac works.

At the API level this changes the event stream. In the past the dead key only generated an incorrectly encoded RELEASED KeyEvent. The fully composed character was then delivered as the commit string in an InputMethodEvent. With this PR the (sort of bogus) RELEASED KeyEvent goes away and is replaced by an InputMethodEvent that delivers the preview diacritic in the compose string.

One minor detail: after entering a dead key the cursor moves to a point before the preview diacritic instead of after it. That looks like a bug and doesn't match the way other parts of Ubuntu behave.

beldenfox avatar Apr 25 '23 19:04 beldenfox

@beldenfox Thanks for taking your time to look at this.

I have fixed the preview diacritic caret position.

tsayao avatar May 06 '23 21:05 tsayao

@beldenfox I'm getting the bogus KEY_RELEASED event you mentioned. It's weird because tbe dead key produces a KEY_RELEASED but not a KEY_PRESS.

Edit:

bool WindowContextBase::im_filter_keypress(GdkEventKey *event) {
    return gtk_im_context_filter_keypress(im_ctx.ctx, event);
}

This is why. I will check, but I think dead keys should either produce both KEY_PRESS and KEY_RELEASED or none.

I have attached a simple test app on JDK-8305418

On Windows (using the test app mentioned) I get when typing é:

Pressed: ´
Pressed: e
Released: e

I think it should have a Released event with ´, shouldn't it?

Also Windows does not deliver the InputMethodEvent.INPUT_METHOD_TEXT_CHANGED event.

tsayao avatar May 06 '23 23:05 tsayao

@tsayao The bogus RELEASED event happens on every platform after an InputMethodEvent that commits. I suppose the key press event is swallowed by the IM context which then commits the text and exits the compose state. Then the release event arrives and is processed as a standard KeyEvent.

On Windows dead keys are handled entirely using KeyEvents, no InputMethodEvents involved. The final character is delivered as the text in a TYPED KeyEvent.

Your new code is handling dead keys just fine, the event sequence matches the way the Mac works and is compatible with the way the previous Linux code worked.

But with this PR I'm not seeing any KeyEvents as I type regular text. If you're typing, say, Spanish most characters (other than dead keys) should generate PRESSED and TYPED KeyEvents. Your code delivers every character in an InputMethodEvent commit event. This needs to be fixed, the system relies on PRESSED events to trigger accelerators.

I did a little Googling and I think this is just the way the Gtk IM works; all text is delivered in a commit signal. I think you need to track whether the IM is in pre-edit mode or not to distinguish which commits should turn into KeyEvents and which should remain as InputMethodEvents. There are signals for tracking pre-edit mode but I haven't tested them out.

beldenfox avatar May 08 '23 00:05 beldenfox

The process is now reporting KEY_PRESS events even if filtered by IME.

@beldenfox You seem to have a lot of expertise on the topic. Should dead keys be reported as KEY_PRESS with the corresponding character?

tsayao avatar May 14 '23 13:05 tsayao

I asked around on Gtk IRC and gdk_keyval_to_unicode won't work for dead keys. We need a custom map including dead keys as seen on gdkkeyuni.c

tsayao avatar May 14 '23 20:05 tsayao

Dead keys should work now, but not sure I covered them all.

tsayao avatar May 14 '23 22:05 tsayao

@tsayao You should expand your test app to show TYPED KeyEvents or use the KeyEventViewer app I just attached to the original bug report. Then run it against an unmodified JFX build on Linux and see what events go by. If you have a Mac that's even better, the sequence of events between Mac and Linux should be very similar.

As I said in my earlier note you already had dead keys working just fine. Just treat them like any other IM event. And they don't generate PRESSED KeyEvents on Linux, that only happens on Windows.

It's the non-dead letter keys that are the problem. If you fire up one of the test apps and immediately press the X key you should see this sequence of events on all platforms:

  1. PRESSED KeyEvent with KeyCode.X
  2. TYPED KeyEvent with the text "x".
  3. RELEASED KeyEvent with KeyCode.X

Your code is generating this sequence:

  1. InputMethodEvent with commit text "x"
  2. PRESSED KeyEvent with KeyCode.X
  3. RELEASED KeyEvent with KeyCode.X

Each of these characters is coming in as a commit signal without a preedit-changed signal before it. These commits shouldn't be sent to jViewNotifyInputMethod. You need to detect this case and a) ignore the commit and b) return false from filterIME so the event is handled by process_key instead.

beldenfox avatar May 15 '23 00:05 beldenfox

Yesterday I was reading some of the Mac code for handling IME to understand it better. I started to get the idea. @beldenfox Your test app is very handy.

tsayao avatar May 16 '23 10:05 tsayao

@beldenfox I've made some progress. I've copied the Mac method from MacView.java for now, still need to figure it out how to deal with IME_ATTR_* and probably the selection/surrounding functionality.

tsayao avatar May 21 '23 22:05 tsayao

@tsayao This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Jul 17 '23 00:07 bridgekeeper[bot]

Thank you, this PR is extremely, extremely useful.

leewyatt avatar Jul 26 '23 15:07 leewyatt

I did put a lot of work on it.

Now I need feedback from users if everything is working as expected. I'm unsure if I missed a detail from a specific language/culture.

tsayao avatar Sep 03 '23 22:09 tsayao

Webrevs

mlbridge[bot] avatar Sep 03 '23 22:09 mlbridge[bot]

I will check the failing tests.

tsayao avatar Sep 06 '23 12:09 tsayao

I ran into similar failures when I added a method to a core class but did not add it to the Stub version of the same class. In my case I added a call to Stage.java but didn't add it to SubStage.java.

I'll be away from my computer for about a week and will take a closer look at this when I get back. I did notice that when I press a dead key the caret ends up at the beginning of the preview string when it should be at the end. There's also an issue with the way keys on the numeric keypad are being encoded. The fix is minor (I think) and I'll send details when I get back. Beyond that I don't understand the IME machinery so here's hoping that someone who does can spare some cycles to review this.

beldenfox avatar Sep 06 '23 16:09 beldenfox

I think it's usable now. I'm not really sure because I'm on unknown land here :).

To my understanding the gtk ime system will use only one preedit attribute which I detect on pango attribute list where:

  • PANGO_ATTR_BACKGROUND: it's a IME preedit suggestion and must be highlighted
  • PANGO_ATTR_UNDERLINE: it's a dead key

otherwise it's a normal input (probably will never hit this case on preedit as it will go to commit directly).

tsayao avatar Sep 18 '23 00:09 tsayao

This is also a step towards Wayland since it removes X11 specific calls for calls that would work on both X11 and Wayland.

tsayao avatar Sep 27 '23 12:09 tsayao

/csr

andy-goryachev-oracle avatar Sep 27 '23 15:09 andy-goryachev-oracle

this needs a CSR due to a change in InputMethodRequests

andy-goryachev-oracle avatar Sep 27 '23 15:09 andy-goryachev-oracle

/reviewers 2

andy-goryachev-oracle avatar Sep 27 '23 15:09 andy-goryachev-oracle

@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@tsayao please create a CSR request for issue JDK-8305418 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk[bot] avatar Sep 27 '23 15:09 openjdk[bot]