rust-windowing / xkeysym

X11 keyboard symbol utilities for Rust
Apache License 2.0
8 stars 4 forks source link

Fix Keysym names for named Keysyms and test them agains xkbcommon #27

Open pentamassiv opened 11 months ago

pentamassiv commented 11 months ago

Fixes https://github.com/rust-windowing/xkeysym/issues/18.

This PR changes the names of the named keysyms to get them in line with the names that xkbcommon returns and tools such as xdotool expect. The only change needed for named keysyms was to get rid of the XK_ part. The only part I am struggling with, is returning the correct name for keysyms representing unnamed unicode codepoints. I have no experience with Rust no_std and don't know how to implement it. The corresponding C code is very simple, so I left a placeholder and left this PR in a "Draft" state. Hopefully somebody can fill in the correct Rust code for it.

I also added a test to make sure the function continues to return the same names for all possible keysyms. There are a few commented out parts that I needed to try the test on my machine with a newer libxkbcommon. I will remove them once the placeholder is properly implemented

I added the KEYSYM_MAX constant. It was added in xkbcommon 1.6.0 and shortens the time the test runs.

pentamassiv commented 11 months ago

The test runs for a long time, so it might be a good idea to split it into a separate Workflow

psychon commented 11 months ago

I have no experience with Rust no_std and don't know how to implement it.

TL;DR: I believe this would require an API break to implement.

Let's ignore no_std for the moment. How would you implement this "normally"? This has to return Option<&'static str> (and this is publically reachable via Keysym::name(). So, the only possibility is to have some kind of static, huge string table (well, it could be lazily initialised, but this is still complicated).

This in contrast to the API from xkbcommon which gets a buffer as its argument and writes the string into that. Thus, no 'static (in Rust-speak) is involved.

pentamassiv commented 11 months ago

Can't the static str get generated dynamically? Something like this:

fn name() -> Option<&'static str> {
    static RESULT: [u8; 3] = [97u8, 98u8, 99u8];
    core::str::from_utf8(&RESULT).ok()
}

The values of the RESULT buffer would have to get calculated. I guess the (big) downside of this is that there is more and more static memory getting allocated and never freed

notgull commented 11 months ago

This would require a massive table for computation that would surely bloat the program. Just returning names like this is enough; we don't have to be concerned about the entirety of Unicode.

We could also add a new method that handles Unicode, but returns Cow<'static, str> instead. This would only be enabled on an alloc feature.

For the tests, try running the test cases in parallel with rayon. It should speed things up.

psychon commented 11 months ago

Something like this:

That RESULT can only store a single value. As a caller, I can keep references to multiple names at once: let foo = [k1.name(), k2.name(), k3.name()];. Thus, to be sound, RESULT may no longer be modified after it was filled once (since the borrow is 'static and thus effectively forever).

pentamassiv commented 11 months ago

Okay, returning names for keysyms representing Unicode codepoints will not be part of this PR.

I used rayon for the test. The tests are failing right now, because the newest keysyms were not yet added to this crate. Once https://github.com/rust-windowing/xkeysym/pull/26 gets merged, it should succeed. This will happen on a regular basis, but the output of the test lists the differences.

The test is ignored per default and does not run in the CI workflow, because running the test still takes around 5 mins. Unfortunately we have to install xkbcommon even if the test is ignored. This adds around 2 min to the time the workflow takes to complete.

I think the PR is ready to be reviewed

notgull commented 11 months ago

Please rebase on #26

pentamassiv commented 11 months ago

Sure, it has been rebased