Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pentamassiv
Copy link
Contributor

@pentamassiv pentamassiv commented Oct 23, 2023

Fixes #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 pentamassiv marked this pull request as draft October 23, 2023 19:44
@pentamassiv pentamassiv force-pushed the stabilize_name branch 2 times, most recently from bb78a4e to 1684b95 Compare October 23, 2023 19:49
@pentamassiv
Copy link
Contributor Author

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

@psychon
Copy link

psychon commented Oct 24, 2023

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
Copy link
Contributor Author

pentamassiv commented Oct 24, 2023

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
Copy link
Member

notgull commented Oct 24, 2023

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
Copy link

psychon commented Oct 24, 2023

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 pentamassiv force-pushed the stabilize_name branch 2 times, most recently from e33da9b to 14645c4 Compare October 24, 2023 20:45
@pentamassiv pentamassiv changed the title DRAFT: Fix Keysym names for named Keysyms and test them agains xkbcommon Fix Keysym names for named Keysyms and test them agains xkbcommon Oct 24, 2023
@pentamassiv
Copy link
Contributor Author

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 #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

@pentamassiv pentamassiv marked this pull request as ready for review October 24, 2023 22:05
@notgull
Copy link
Member

notgull commented Oct 25, 2023

Please rebase on #26

@pentamassiv
Copy link
Contributor Author

Sure, it has been rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Match output of Keysym::name() with xdotools names
3 participants