Add new warning for deprecated keysyms #356
Conversation
wismill
added
the
compile-keymap
wismill
force-pushed
the
wip/deprecated-keysyms
branch
2 times, most recently
from
b704736
to
9dedbb1
Compare
|
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? |
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.
Yes they are. Well the patch has been reverted since, but the plan is to delete them.
This is not the policy in
This is the case. But there is also the case for people developing their own layouts. |
|
I think it would be enough to check for deprecated keysyms only with a special version dev version of |
wismill
force-pushed
the
wip/deprecated-keysyms
branch
2 times, most recently
from
d90814d
to
463883b
Compare
|
Rebased. Testing keysym deprecation is now only done when verbosity is ≥ 10. |
|
Deprecation test is now done only when the new flag |
There was a problem hiding this comment.
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?
wismill
force-pushed
the
wip/deprecated-keysyms
branch
from
dd7f799
to
6736825
Compare
|
@bluetech Rebased, cleaned commit history.
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 So maybe set the threshold to 5? The deprecation test is quite fast compared to all the keymap processing. |
|
Valgrind detected a bug that does not trigger on my computer. Will check it later. |
|
Run locally in podman without error; re-run the failed jobs and no error 🤔 |
`xkb_keysym_from_name` does not provide the successful type of parsing, i.e. if a name, a Unicode codepoint or an hexadecimal number. A the new function `xkb_keysym_with_format_from_name`, which acts as `xkb_keysym_from_name`, but it also provide the type of successful parsing. Also improve the Unicode parsing tests.
This function allow to check whether a keysym is deprecated, based on the keysym, the type of its parsing and optionally its string. The generation of the table of deprecated keysyms relies on the assumption described in `xkbcommon-keysyms.h`.
wismill
force-pushed
the
wip/deprecated-keysyms
branch
from
6736825
to
ed99e43
Compare
Add new warning for deprecated keysyms. Useful in particular for xkeyboard-config.
TODO: