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

Update to match xorgproto 2024.1 #29

Merged
merged 3 commits into from
Apr 14, 2024

Conversation

wysiwys
Copy link
Contributor

@wysiwys wysiwys commented Apr 1, 2024

Hello,

This pull request provides an updated version of xkeysym to match the latest xorgproto release (2024.1) and the latest version of libxkbcommon (1.7.0). Currently, xkeysym is only in sync with an earlier xorgproto release, 2022.1.

The changes in this pull request reflect the new keysym definitions added by xorgproto 2024.1 and 2023.1 . Additionally, xorgproto 2023.1 deprecated several keysym names and added their new, preferred names (e.g. guillemotleft becomes guillemetleft), although the old names are kept for compatibility, in order to not break applications that depend on these older symbol names.

One question relates to the choice of environment to fetch the most recent keyboard symbol headers, and generate the keyboard symbols from them. The rust:slim image, which is currently used by this crate to set up the environment to generate Rust code from headers, only has x11proto-core-dev 2022.1.1 (corresponding to xorgproto 2022.1) as the latest version available in apt. This means that the current symbol set is only consistent with that, older release, and does not reflect recent changes. The other Rust official Docker images all only have older versions of xorgproto available through the package manager, except rust:alpine which has a somewhat more recent xorgproto version at 2023.2.

As a way to work around this, this pull request shows that the archlinux:base image can be used instead, since Arch's pacman has xorgproto 2024.1 available. However, this choice makes the code generation step slower, since cargo must now be installed in the container first.

Would there be a better way to accomplish this, or is this fine? I thought it could be possible instead to automatically generate directly from the downloaded raw header files, without installing a package, although this would require a significantly different setup process and might have its own downsides.

Thanks

@kchibisov kchibisov requested a review from notgull April 9, 2024 18:07
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated file is fine.

There's also one place where update might be needed, though, I haven't checked myself. We have a KEYSYM tab that matches what is present in libxkbcommon, so we might need to update it as well.

See the from_char and the top level comment where it came from in src/lib.rs.

--

I'm fine with using e.g. arch since the thing is generated manually.

@wysiwys
Copy link
Contributor Author

wysiwys commented Apr 11, 2024

@kchibisov

Glad to hear that. The main advantage of an Arch container that I can see is that its xorgproto package is updated very quickly, generally within a few days of release, so it is easy to get the latest version.

I have taken a look at the file keysym-utf.c in libxkbcommon to check if any changes to the unicode keysym table are needed. A look through the commits that have happened since libxkbcommon 1.4.1 (which is the version that xkeysym's KEYSYM table matches) confirms that there are no new entries in that table since the current standpoint.

Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will wait for @notgull for a while.

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, this looks good to me

@wysiwys
Copy link
Contributor Author

wysiwys commented Apr 12, 2024

Hi, thanks for the approvals. However, before merging, there is one small improvement to this PR that that does not affect the generated keysyms, but makes the pacman commands a bit better:

  • The two pacman commands are condensed into one to avoid refreshing the package database twice (since pacman -Sy <packages> already refreshes the database before installing the new packages)
  • Added the -u flag, which updates all packages on the system that are out of date. Although the install was previously fine without this flag, I have meanwhile learned that this flag is important, because it avoids the possibility of a partial upgrade in Arch which could cause packages to break in this container.

Although the previous pacman commands worked, I have learned in the meantime that pacman -Syu <packages> is generally considered to be the best way to upgrade the package database and then install packages under Arch.

After making this change, I checked that re-running the keysym generation gives the same result.

@kchibisov kchibisov merged commit a4d971d into rust-windowing:master Apr 14, 2024
5 checks passed
@wysiwys wysiwys deleted the latest-xorgproto branch April 15, 2024 03:03
@notgull notgull mentioned this pull request Jun 1, 2024
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.

3 participants