X11: fix XIM input method support

[Edgaru089]

Description

This PR is a (good enough) fix to #1840.

It fixes the existing XIM code, supporting input methods (at least with Fcitx5).

How to test

Dump all the KeyPressed/TextInput event from any window. If you don't have a input method, nothing should change. If you do have one, it should be working now.

#include <SFML/Graphics.hpp>

int main()
{
    sf::RenderWindow window(sf::VideoMode(800, 600), "Window");
    window.setFramerateLimit(60);

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            switch (e.type) {
                case sf::Event::Closed:
                    win.close();
                    break;
                case sf::Event::KeyPressed:
                    printf("Key pressed: %d\n", (int)e.key.code);
                    break;
                case sf::Event::KeyReleased:
                    printf("Key released: %d\n", (int)e.key.code);
                    break;
                case sf::Event::TextEntered:
                    printf("Text entered: %d\n", (int)e.text.unicode);
                    break;
            }
        }

        window.clear();
        window.display();
    }
}

[Edgaru089]

I added a printf() in checkEvent():

// Filter the events received by windows (only allow those matching a specific window)
Bool checkEvent(::Display*, XEvent* event, XPointer userData)
{
	printf("CheckEvent: event type=%d, window=%ld, user=%ld\n",event->type,event->xany.window, userData);
	// Just check if the event matches the window
	return event->xany.window == reinterpret_cast< ::Window >(userData);
}

Looks like upon keypresses, the IME is firing ClientMessage events into the queue with a different Window ID:

...
CheckEvent: event type=33, window=94371847, user=94371845
CheckEvent: event type=33, window=94371847, user=94371845
CheckEvent: event type=33, window=94371847, user=94371845
... (33 = ClientMessage)

I added a || event->type == ClientMessage to the condition and the IME worked again, at least for a single window.

This is what happened when I press a key:

                   This is the output: | CheckEvent: event type=2, window=94371845, user=94371845
The first KeyPress event is filtered.  | event type=2, filtered
A ClientMessage is sent by the IME.    | CheckEvent: event type=33, window=94371847, user=94371845
The ClientMessage is also filtered.    | event type=33, filtered
After that, the real KeyPress is sent. | CheckEvent: event type=2, window=94371845, user=94371845
This event is (at last!) not filtered. | event type=2

Main: Event::TextEntered, keycode=99(c)

Gonna test it with multiple windows.

[Edgaru089]

With 2 windows, the IME sometimes glitches out if we switch to the other window while typing something.

I looked up some documentations, and it seems that the XIM object is per-display, and the XIC context is bound to a specific window. So I saved the XIM object to the namespace WindowsImplX11Impl and have all the window share the same XIM object. It works now! It still need to be mutex-protected though.

This is the snippet I used to test:

#include <SFML/Window.hpp>
#include <SFML/Graphics.hpp>
#include <iostream>

sf::RenderWindow win1,win2;

bool processEvent(sf::Event& event,int win){
	switch(event.type){
	case sf::Event::Closed:
		win1.close();
		win2.close();
		return false;
	case sf::Event::KeyPressed:
		printf("(Win%d) Key Pressed: %d\n",win,event.key.code);
		break;
	case sf::Event::TextEntered:
		printf("(Win%d) Text Entered: %d\n",win,event.text.unicode);
		break;
	}

	return true;
}

int main(int argc, char* argv[])
{
	win1.create(sf::VideoMode(400, 200), "Window1");
	win1.setVerticalSyncEnabled(true);
	win2.create(sf::VideoMode(400, 200),"Window2");
	win2.setVerticalSyncEnabled(true);

	while (win1.isOpen() && win2.isOpen()) {
		sf::Event event;
		while (win1.pollEvent(event))
			if(!processEvent(event,1))
				return 0;
		while(win2.pollEvent(event))
			if(!processEvent(event,2))
				return 0;

		win1.clear(sf::Color::Magenta);
		win2.clear(sf::Color::Cyan);
		win1.display();
		win2.display();
	}
	return 0;
}

[Edgaru089]

The updated code uses XCheckIfEvent to pick out events only for a single window.

It also keeps a global XIM object instead of creating a new one with every window. Maybe the commit name should include that?

[Edgaru089]

Rebased with latest master. @eXpl0it3r Any updates?

[eXpl0it3r]

I don't know much about the whole X11 setup and hope @binary1248 can have a closer look at this 🙂

[binary1248]

I still don't quite understand why it was necessary to make such significant changes to the logic around m_events. m_events was introduced in #1291 to fix #1223. Removing this code risks re-introducing the bug the code was written to prevent.

I would prefer this PR is modified to affect the existing m_events logic as little as possible.

Alternatively, this would have to be tested on multiple different systems/configurations to verify that the original bug is not re-introduced. Maybe @naezith or @texus could help with this since they worked on the issue.

[Edgaru089]

I would prefer this PR is modified to affect the existing m_events logic as little as possible.

This is possible. The only things we need to do are:

• setlocale(LC_ALL, "") on XIM creation
• Call XFilterEvent(&event, None) on ClientMessage events
• Global XIM context across SFML windows (XICs are per-window)
• Send multiple TextInput events from one X event (there may be multiple codepoints in a single UTF-8 string from the raw event)

Working on it. Should we change the commit/pr name?

[Edgaru089]

The new commit is a moderate change set to have XIM working across multiple windows. m_events is kept untouched and things work on my setup. The global XIM instance is kept with the Display connection.

[Edgaru089]

Rebased with latest master.

Sorry for the mess... It was indeed not necessary to change this much. To my defense, there are very little and even conflicting information on how to handle XIM correctly, not to mention with multiple windows.

I removed m_events because I observed some strange KeyPress events being sent both to the IM and the application early on while hacking the code. It was not necessarily related to m_events itself, but more likely because of not calling XFilterEvent() properly, or even the crappy test snippet I was using. The drastic code changes in the first commit were more of a guess and less of reasoning, and you have my more sincere apologies for that...

I rewrote the code, pushed the second commit and was never able to repeat the bug.

---

Just being curious, is it possible for this pull request to make it into SFML 2.6.0? I sure can do a back port.

[binary1248]

There is no need to apologize for standard development practice. 😉

After all is said and done this PR should/would be squashed and rebased anyway, so any intermediate steps wouldn't have become part of the commit history.

Regarding the backport to 2.6.0: @eXpl0it3r will have to comment on that. I personally don't have anything against it.

[Edgaru089]

All fixed. You really have those eagle eyes👍

[eXpl0it3r]

Yeah, I think it could be a great addition and is isolated enough for SFML 2.6.
Could you rebase the commit onto the 2.6.x branch and change the PR target to the 2.6.x branch?

[Edgaru089]

@eXpl0it3r I still don't quite get it. Is it the case that the feature changes on 2.6.x will be ported to 3.0 once all the major API changes are sorted out? That sounds like a major undertaking.

[eXpl0it3r]

No, we'll continuously merge changes from 2.6.x to master.
It's a bit work, but at the same time, we don't really plan to add much more to SFML 2.6

[Edgaru089]

Rebased with 2.6.x.

[binary1248]

Looks good to me, although I must admit I don't really have the technical expertise or experience to test these changes because I have never had to type using an input method before. Perhaps there are others willing to give this a try.

[eXpl0it3r]

Thank you for diving into this! 🙂

[kimci86]

I noticed since this commit that more events are generated, which looks like a bug to me.

Testing with SFML-Input and SFML feature/scancode branch on Ubuntu 20.04: when pressing a key, two KeyPressed events are generated with a small delay between them, instead of one event. If the key is held down, many KeyPressed events are regularly generated when the key is down (which is normal) but also after the key is released (not normal) for some time.

I did not test the newly supported input method though. I never used such input method.

[Edgaru089]

@kimci86 Can you try again with export XMODIFIERS=@im=none? This effectively disables the entire mess with XIM. You can also take a look at GTK_IM_MODULE and QT_IM_MODULE environment variables, they effectively decide what IM protocol to use for GTK and Qt.

Maybe you have the input method framework IBus installed. It comes bundled with GNOME and is notoriously hard to replace, many of us end up giving up GNOME just to not use IBus.

[kimci86]

Running again with XMODIFIERS=@im=none restores the expected behavior indeed 👍
GTK_IM_MODULE is empty (not defined) and QT_IM_MODULE is ibus in my environment.

Is there a way to have this behavior by default, without having to set an environment variable?

[Edgaru089]

@kimci86 IBus is, how to say... Neglected. It kind of works, but most people who care more about input methods often use others like Fcitx or Uim. Also it's part of good-old GNOME. The fault is most likely IBus's IMHO.

Is there a way to have this behavior by default, without having to set an environment variable?

AFAIK I'm afraid no, but I'm not an expert. I've never used IBus before. Gonna do some testing.

[Edgaru089]

@kimci86 Oh no, the fault is mine in fact...

If you look at WindowImplX11.cpp:L1880

// Key down event
case KeyPress:
{
	// Fill the event parameters
	Event event;
	event.type        = Event::KeyPressed;
	event.key.code    = (......)
	pushEvent(event);

	// Generate a TextEntered event
	if (!XFilterEvent(&windowEvent, None)) // Feed the event to the IME, discard it if requested
	{
		event.type = Event::TextEntered;
		......
	}
}

Clearly there's something wrong here, and the if (!XFilterEvent(&windowEvent, None)) should include the part sending the KeyPress event. I'm sorry...

I wasn't able to catch that mainly because when the IME takes some text, IBus sends a KeyPress without text input attached, while Fcitx and XWayland (what I use) sends no KeyPress at all. Thus if the previous event is not filtered, it gets duplicated by IBus.

---

I opened #1894 with the fix.