query keyboard LED status via X11 C API

[jwt27]

Executing xset on every tick is a rather expensive operation, as it has to spawn a new process and open and close a TCP connection each time, slowing the game down significantly. Additionally, if $DISPLAY is set but no X server is present, error messages appear continuously and make the game unplayable.

To resolve this, we can use the X11 C API to open an X display connection on startup, and keep it open for the duration of the game. Then the keyboard LED status can be queried directly via XGetKeyboardControl(). With PuTTY, if X11 forwarding is enabled ($DISPLAY is set) but no X server is running on the client, this still results in a single error message on startup, but it quickly disappears after raise_curtain().

Still, it seems vcxsrv does not report the keyboard LED status, and I think reading LEDs from /dev/tty will not work on any SSH connection, so Fast Mode is still unavailable. Maybe this needs an alternative key binding.

[jwt27]

I added a new macro ROGUE_NO_X11, which can be defined if the dependency on X11 libraries is undesirable.

[MestreLion]

First of all, thanks a LOT for this contribution!

Some comments and thoughts

• Yes, the check on __linux__ is needed as the idea behind this project is to be as widely compatible and standard as possible. So I'm (trying to) strictly adhere to both ISO C11 and POSIX C. Yes, it should compile and run in any modern platform, from Linux to BSD to Mac to Windows, as long as they have Curses/NCurses (or PDCurses, etc)
• Which is why I'm reluctant add a hard-dependency on X11. ROGUE_NO_X11 is a good compromise.
• I really loved the X11 library calls. It is a great drop-in replacement to my xset ugly hack, as both have the same requirement of an active X11 Server
• Maybe we should check if DISPLAY is both set and not empty, just to be safer? My C is a bit rusty, but I believe it would be something like char *env = NULL; if ((env = getenv("DISPLAY")) && *env) ...
• I wasn't sure if setup()/unsetup() were an appropriate place to inject the xdisplay stuff. But, since they're always called by main() and md_exit(), and as a bonus it keeps all #ifdef's in mach_dep.c, it seems a good choice.

[jwt27]

First of all, thanks a LOT for this contribution!

No problem, and thanks for the feedback!

Some comments and thoughts

• Yes, the check on __linux__ is needed as the idea behind this project is to be as widely compatible and standard as possible. So I'm (trying to) strictly adhere to both ISO C11 and POSIX C. Yes, it should compile and run in any modern platform, from Linux to BSD to Mac to Windows, as long as they have Curses/NCurses (or PDCurses, etc)
• Which is why I'm reluctant add a hard-dependency on X11. ROGUE_NO_X11 is a good compromise.

Windows at least will need some different method to get LED status, and probably other things too. I don't think it will compile as-is on Windows (can check later). Ideally you'd have a configure script or some build system to toggle system-specific macros, but I think you have an issue # open for that already.

• I really loved the X11 library calls. It is a great drop-in replacement to my xset ugly hack, as both have the same requirement of an active X11 Server
• Maybe we should check if DISPLAY is both set and not empty, just to be safer? My C is a bit rusty, but I believe it would be something like char *env = NULL; if ((env = getenv("DISPLAY")) && *env) ...

XOpenDisplay() already performs that check, so it's not necessary. In fact, the check for getenv("DISPLAY") is already pointless, we just get NULL if the display connection couldn't be opened. PuTTY always sets DISPLAY=localhost:10.0 so it also doesn't help in my specific case.

[MestreLion]

Windows at least will need some different method to get LED status, and probably other things too. I don't think it will compile as-is on Windows (can check later).

I believe it does compile and runs fine (but never tested), provided there's a compatible ncurses C library for Windows's cmd.exe (PDCurses?) and a C compiler that has a strict ISO C11 mode. Should run fine at least on Cygwin or similar.

Ideally you'd have a configure script or some build system to toggle system-specific macros, but I think you have an issue # open for that already.

Yup. Build system is a mess, and my (poorly) hand-crafted Makefile is starting to show its limitations.

In fact, the check for getenv("DISPLAY") is already pointless, we just get NULL if the display connection couldn't be opened.

But that prints a one-time error if connection cannot be opened, right? The DISPLAY check is just a run-time safeguard to avoid any X11-specifc calls under plain SSH or Linux Console

PuTTY always sets DISPLAY=localhost:10.0 so it also doesn't help in my specific case.

Fair point, but I think it's handy to allow DISPLAY='' ./rogue instead of unset DISPLAY; ./rogue

[jwt27]

In fact, the check for getenv("DISPLAY") is already pointless, we just get NULL if the display connection couldn't be opened.

But that prints a one-time error if connection cannot be opened, right? The DISPLAY check is just a run-time safeguard to avoid any X11-specifc calls under plain SSH or Linux Console

PuTTY always sets DISPLAY=localhost:10.0 so it also doesn't help in my specific case.

Fair point, but I think it's handy to allow DISPLAY='' ./rogue instead of unset DISPLAY; ./rogue

An empty or invalid DISPLAY already works, and causes no error messages. The error only occurs when DISPLAY=localhost:10.0 and the X server is not running. It is generated by PuTTY itself, not the X11 library:

LED reading still doesn't work on PuTTY in any case, and I don't see any way to make that work. I think it's better to have an option that disables LED display entirely and rebinds Fast mode to some other key.