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

Keysyms: improve generator #364

Merged
merged 3 commits into from
Sep 20, 2023

Conversation

wismill
Copy link
Member

@wismill wismill commented Jul 21, 2023

Simplify evdev and XK_ substitution and improve alignment. Also, perform some additional XK_ substitutions in comments.

Motivation: see MR in xorgproto

Simplify evdev and XK_ substitution and improve alignment. Also, perform
some additional XK_ substitutions in comments.
@wismill wismill added enhancement Indicates new feature requests keysyms generator labels Jul 21, 2023
@wismill wismill requested a review from whot July 21, 2023 15:32
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

I generated the before/after and I see the improvement, thanks.

@bluetech
Copy link
Member

@wismill I wanted to ask, when you open a PR as draft do you still want reviews or should we wait until it's not a draft?

@wismill
Copy link
Member Author

wismill commented Jul 24, 2023

@wismill I wanted to ask, when you open a PR as draft do you still want reviews or should we wait until it's not a draft?

@bluetech Good question! My take on draft PR is: “I inform you that I work on this topic; the code is sufficiently advanced to open an MR but not quite finished; you are welcome to review it but wait that I finish the draft if you do not have much time”.

@wismill wismill marked this pull request as ready for review July 24, 2023 18:33
Copy link
Contributor

@whot whot left a comment

Choose a reason for hiding this comment

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

a few minor nitpicks but this is a definitive improvement. would be nice to hook up black in the CI [1] to ensure this doesn't accidentally get different formatting in the future.

[1] possibly via pre-commit.ci

scripts/makeheader Show resolved Hide resolved
scripts/makeheader Outdated Show resolved Hide resolved
re.VERBOSE,
)
# Match remaining XK_ references
xorgproto_keysym_prefix_pattern = re.compile(r'\b(?P<prefix>\w*)XK_(?!KOREAN\b)')
Copy link
Contributor

Choose a reason for hiding this comment

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

got confused by this and had to check the various header files, would be nice to add something like:

# XK_KOREAN has ifdefs, ensure we handle those too

Copy link
Member Author

Choose a reason for hiding this comment

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

It is confusing, indeed! This pattern is intended to match remaining keysyms references in comments, e.g. replace:

#define XKB_KEY_SunCompose		0x0000FF20	/* Same as XK_Multi_key */

with:

#define XKB_KEY_SunCompose		0x0000FF20	/* Same as XKB_KEY_Multi_key */

I updated the comment.

@wismill wismill merged commit b900faf into xkbcommon:master Sep 20, 2023
4 checks passed
@wismill wismill mentioned this pull request Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates new feature requests keysyms generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants