Linux Keyboard Repeat Patch Review

[egrath]

Hello,
can someone please review my patch for 2795321 and tell me if it's working as expected for him.

What the Patch is all about is to fix a problem with the Auto Keyboard Repeat Feature in X11 (KeyPress/KeyRelease Events always sent even if the Key is hold down)

For further information please read the comment in the Bugtracker.

Thanks,
Egon

https://sourceforge.net/tracker/index.p ... tid=540676

[Cloudef]

Oh yes, i saw a problem in jaunty that my arrow keys did not work anymore. I found a workaround for it tho. Does this patch make them work again?

[egrath]

Yes this patch should address (and hopefully fix!) this issue.
Egon

[FreakNigh]

Damn this is such an old bug I thought it was fixed like years ago and I'm going to need it working right too here shortly..

[CuteAlien]

Hm, I can't reproduce it so far.

You missed one check, I don't think that is the problem, but maybe try again with adding first:

Code: Select all

if (event.EventType == EET_KEY_INPUT_EVENT)

But you have a newer Xorg, I am on Debian Lenny which still uses Xorg 1.4.2, so maybe the problem was introduced in a newer X version.

You wrote that XkbSetDetectableAutoRepeat is broken - do you maybe have a link with more info about that?
edit: ok, found it already. Hm, don't like that bug. I try to look some more into it at the weekend.

[CuteAlien]

I'm not sure if this got fixed in X by now. I found a thread where they discuss a patch, but at least that patch discussed there does not seem to have been added to Xorg (at least if "master" is their main repository). Though I can't tell if they maybe fixed it in another way.

But I don't see a problem with the patch, so I disabled the XkbSetDetectableAutoRepeat and added also your XPending check (nice catch!) to trunk now. If that works it can probably also go in 1.5. Thanks for the patch.

[pc0de]

I'm seeing a small hiccup in my movement code with this patch. I added debug to CIrrDeviceLinux.cpp as follows:

Code: Select all

			case KeyRelease:
				if (0 == AutorepeatSupport && (XPending( display ) > 0) )
				{
					// check for Autorepeat manually
					// We'll do the same as Windows does: Only send KeyPressed
					// So every KeyRelease is a real release
					XEvent next_event;
					XPeekEvent (event.xkey.display, &next_event);
					if ((next_event.type == KeyPress) &&
						(next_event.xkey.keycode == event.xkey.keycode) &&
						(next_event.xkey.time == event.xkey.time))
						//(next_event.xkey.keycode == event.xkey.keycode))
					{
						/* Ignore the key release event */
                  printf("Condition 1 - KeyRelease Ignored...\n");
						break;

					}
               else
               {
                   printf("Condition 2 - KeyRelease Accepted...\n");
                   printf("next_event.type: %d-%d\n", next_event.type, KeyPress);
                   printf("next_event.xkey.keycode: %d-%d\n", next_event.xkey.keycode,
                   event.xkey.keycode);
                   printf("next_event.xkey.time: %d-%d\n", next_event.xkey.time,
                   event.xkey.time);
               }
				}
            else printf("Condition 3 - KeyRelease no pending...\n");
				// fall-through in case the release should be handled

and found that while holding down any of the movement keys, I sporadically see "Condition 2":

Code: Select all

Condition 1 - KeyRelease Ignored...
Condition 1 - KeyRelease Ignored...
Condition 1 - KeyRelease Ignored...
Condition 2 - KeyRelease Accepted...
next_event.type: 2-2
next_event.xkey.keycode: 25-25
next_event.xkey.time: 671290-671289
Condition 1 - KeyRelease Ignored...
Condition 1 - KeyRelease Ignored...
Condition 1 - KeyRelease Ignored...
Condition 1 - KeyRelease Ignored...

The cause is the time being off consistently by 1ms. Removing the time check fixes my problem and also works with the Irrlicht examples. Is there any reason to not remove it permanently?

[CuteAlien]

Thanks, I will take a look at this in the weekend. Not sure if removing the time-check couldn't lead to real key-released getting missed (for example with a slow framerate). Have to check that.

[CuteAlien]

I've patched up the patch some more in svn. Couldn't remove the time-check because otherwise real key releases could be missed (you can add a sleep in the mainloop to test that). So I test for <2 now. I googled around afterwards a little and this actually seems to be a solution also taken by other libraries.

Further testing is very welcome, so here's a little test-app (for the trunk-version):

Code: Select all

#include <irrlicht.h>
#include <iostream>

using namespace irr;
using namespace core;
using namespace scene;
using namespace video;
using namespace io;
using namespace gui;

#ifdef _IRR_WINDOWS_
#pragma comment(lib, "Irrlicht.lib")
#endif

class MyEventReceiver : public IEventReceiver
{
	bool OnEvent( const SEvent &event )
	{
		if (event.EventType == EET_KEY_INPUT_EVENT) 
		{
			if( event.KeyInput.PressedDown )
			{
				std::cout << "PressedDown" << std::endl;
			}
			else if( !event.KeyInput.PressedDown )
			{
				std::cout << "!PressedDown" << std::endl;
			}
		}
	
		return false;
	}
};

int main()
{
	video::E_DRIVER_TYPE driverType = video::EDT_OPENGL;
	IrrlichtDevice * device = createDevice(driverType, core::dimension2d<u32>(640, 480));
	if (device == 0)
		return 1; // could not create selected driver.

	video::IVideoDriver* driver = device->getVideoDriver();
	MyEventReceiver receiver;
	device->setEventReceiver(&receiver);
	
	while(device->run() && driver)
	{
		if (device->isWindowActive())
		{
			// device->sleep(1000);
		}
	}

	device->drop();

	return 0;
}

[pc0de]

Tested the "< 2" update and it works well against my code. Thanks!