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.

Sign up for GitHub

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

Jump to bottom

Add new warning for deprecated keysyms #356

Merged
merged 4 commits into from
Sep 23, 2024
Merged

Add new warning for deprecated keysyms #356

merged 4 commits into from
Sep 23, 2024

Conversation

wismill
Copy link
Member

@wismill wismill commented Jul 2, 2023 •
edited
Loading

Add new warning for deprecated keysyms. Useful in particular for xkeyboard-config.

TODO:

  • Perf?
  • Public API?
  • Changelog entry

@wismill wismill mentioned this pull request Jul 2, 2023
Closed
@wismill wismill added the compile-keymap Indicates a need for improvements or additions to keymap compilation label Jul 3, 2023
@wismill wismill added this to the 1.6.0 milestone Jul 3, 2023
@wismill wismill force-pushed the wip/deprecated-keysyms branch 2 times, most recently from b704736 to 9dedbb1 Compare July 3, 2023 19:50
@bluetech
Copy link
Member

bluetech commented Jul 8, 2023

I am doubtful on the cost vs. benefit of warning about deprecated keysym names. The deprecated names are never going to be removed. And more generally, basically all of the named keysyms are deprecated in favor of Unicode keysyms...

Maybe xkeyboard-config wants to avoid using deprecated names? If so, maybe do it as a lint script directly in xkeyboard-config?

@wismill
Copy link
Member Author

wismill commented Jul 12, 2023

I am doubtful on the cost vs. benefit

Are we talking about perf, maintenance, both, other? About perf, I thought we could use a flag to make it opt-in. About maintenance: I think there is no maintenance issue, as long as the conventions to deprecate keysyms hold. The table is generated in the same script that update keysyms name lookup.

The deprecated names are never going to be removed.

Yes they are. Well the patch has been reverted since, but the plan is to delete them.

And more generally, basically all of the named keysyms are deprecated in favor of Unicode keysyms...

This is not the policy in xkeyboard-config, where non-deprecated named keysyms are preferred over Unicode ones.

Maybe xkeyboard-config wants to avoid using deprecated names? If so, maybe do it as a lint script directly in xkeyboard-config?

This is the case. But there is also the case for people developing their own layouts.

@wismill wismill removed this from the 1.6.0 milestone Sep 19, 2023
@wismill
Copy link
Member Author

wismill commented Sep 19, 2023

I think it would be enough to check for deprecated keysyms only with a special version dev version of compile-keymap; thus keeping the published version fast. The dev version would be only used in CI, e.g. for xkeyboard-config.

@wismill wismill added this to the 1.7.0 milestone Sep 20, 2023
@wismill wismill force-pushed the wip/deprecated-keysyms branch 2 times, most recently from d90814d to 463883b Compare September 22, 2023 09:02
@wismill
Copy link
Member Author

wismill commented Sep 22, 2023

Rebased. Testing keysym deprecation is now only done when verbosity is ≥ 10.

@wismill
Copy link
Member Author

wismill commented Sep 22, 2023

Deprecation test is now done only when the new flag XKB_CONTEXT_EXHAUSTIVE_CHECKS is set. Without it, the new benchmark bench-compile-keymap gives only about +0,2% more time that on master. With the test, it take about +0,8%. I think this is reasonable!

bluetech
bluetech reviewed Sep 24, 2023
View reviewed changes
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.

The deprecated names are never going to be removed.

Yes they are. Well the patch has been reverted since, but the plan is to delete them.

I'm surprised a breaking change was attempted. IMO it's better to leave the deprecated keysyms. But if xorgproto is going to remove them then I agree it would be good to warn about them.

Deprecation test is now done only when the new flag XKB_CONTEXT_EXHAUSTIVE_CHECKS is set. Without it, the new benchmark bench-compile-keymap gives only about +0,2% more time that on master. With the test, it take about +0,8%. I think this is reasonable!

I thought the verbosity guard was pretty good myself, and allows the user/keymap auther to control it. Why is a context flag better?

scripts/makekeys Outdated Show resolved Hide resolved
scripts/makekeys Outdated Show resolved Hide resolved
scripts/makekeys Outdated Show resolved Hide resolved
src/keysym.h Outdated Show resolved Hide resolved
src/keysym.c Outdated Show resolved Hide resolved
@wismill
Copy link
Member Author

wismill commented Sep 25, 2023 •
edited
Loading

@bluetech Rebased, cleaned commit history.

I thought the verbosity guard was pretty good myself, and allows the user/keymap auther to control it. Why is a context flag better?

I am not sure, just experimenting 😄. I think the first motivation was to make it a bit faster. Also it would allow to select some tests on demand. But looking at it a day later it is maybe overcomplicated.

I think we could go with the verbosity check, but I would reduce the threshold: using deprecated keysyms means your layout may break sooner or later, depending of the pace the xorg-proto devs go. I would be useful for xkeyboard-config, but also for anyone that use a custom layout.

So maybe set the threshold to 5? The deprecation test is quite fast compared to all the keymap processing.

@wismill
Copy link
Member Author

wismill commented Sep 25, 2023

Valgrind detected a bug that does not trigger on my computer. Will check it later.

@wismill
Copy link
Member Author

wismill commented Sep 25, 2023

Run locally in podman without error; re-run the failed jobs and no error 🤔

@wismill wismill modified the milestones: 1.7.0, 1.8.0 Feb 8, 2024
@wismill wismill force-pushed the wip/deprecated-keysyms branch 3 times, most recently from 5891692 to 001a290 Compare September 19, 2024 18:02
@wismill
Copy link
Member Author

wismill commented Sep 19, 2024

Rebased and improved benchmark.

@wismill wismill marked this pull request as ready for review September 19, 2024 18:10
@wismill
Copy link
Member Author

wismill commented Sep 19, 2024

TODO: changelog

@wismill
Add benchark for compile-keymap
3577024 
Implement `tasty-bench` method:
1. Set n = 1.
2. Measure execution time t_n of n iterations and execution time t_2n
   of 2n iterations.
3. Find t which minimizes deviation of (n·t, 2·n·t) from (t_n, t_2n),
   namely t = (t_n + 2·t_2n)/5n.
4. If deviation is small enough (see --stdev CLI parameter), return t
   as a mean execution time.
5. Otherwise set n = 2·n and jump back to Step 2.

See: https://hackage.haskell.org/package/tasty-bench
@wismill
Add function xkb_keysym_is_deprecated
0af9c50 
This function allow to check whether a keysym is deprecated, based
on the keysym and optionally its name.

The generation of the table of deprecated keysyms relies on the
rules described in `xkbcommon-keysyms.h`.

The `ks_table.h` is now generated deterministically by setting
explicitly the random seed to a constant. This will avoid noisy
diffs in the future.
@wismill
Add new warnings for deprecated keysyms
888aa60 
Add 2 new warnings:
- Deprecated keysym name (typo, historical alias, etc.);
- Deprecated keysym (all names and forms).

Guard deprecated keysym tests with verbosity level ≥2, so they are
run only when actually needed.
@wismill
scripts: Improve messages registry update
b146193
@wismill wismill merged commit b5f0779 into xkbcommon:master Sep 23, 2024
4 checks passed
@wismill wismill deleted the wip/deprecated-keysyms branch September 23, 2024 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluetech bluetech bluetech left review comments

Assignees
No one assigned
Labels
compile-keymap Indicates a need for improvements or additions to keymap compilation
Projects
None yet
Milestone
1.8.0
Development

Successfully merging this pull request may close these issues.
2 participants
@wismill @bluetech