fix(keybindings): allow "." to be bound from keybindings.toml - #11
Open
nocstah wants to merge 1 commit into
Open
fix(keybindings): allow "." to be bound from keybindings.toml#11nocstah wants to merge 1 commit into
nocstah wants to merge 1 commit into
Conversation
The period key cannot be named anywhere in the keybinding system: it is
absent from KebindingResolver._namedKeys, _canonicalKeyName() rejects it (it
only special-cases "/", ":", "?" and "\"), and _eventKeyName() returns null
for Qt.Key_Period.
The result is a silent no-op rather than an error. A user writing
[keybindings]
toggle_hidden = "."
gets an entry that parses to null, is dropped as invalid, and never binds --
indistinguishable from a typo, and with nothing in the log to say why. Since
P2.5's whole premise is that alternative layouts can move any key they like,
a punctuation key that cannot be expressed at all is a real gap.
Fixes it the same way the four existing punctuation keys are handled:
_namedKeys gains "period"/"." -> Qt.Key_Period, _canonicalKeyName() accepts
".", and _eventKeyName() prefers the symbol form -- that last part matters,
because both spellings map to one Qt.Key_*, and only one canonical string may
come out of _eventKeyName() or it can never compare equal to what
_canonicalKeyName() produced.
Adds a regression check ("a punctuation key (".") can be bound from config")
covering the real user path: the override is accepted, "." reaches
toggle_hidden, its previous Ctrl+H stops working per the full-replacement
policy, and "/" -- which always worked -- still searches. Verified to fail
without the fix: `override accepted=false "."->toggle_hidden=false`.
CheckKeybindings' own NAMED_KEYS also needed "." so the harness can express
it: it is deliberately independent of the resolver's table, and its
single-character fallback computes Qt.Key_A + (charCode - 'a'), which for "."
is Qt.Key_A - 51.
Selfcheck on this branch: 125 passed, 1 failed, 126 total. The one failure is
the pre-existing thumbnail-video.sh check, which fails the same way on an
unmodified master build here (it needs ffmpegthumbnailer, which this machine
does not have -- see the separate PR).
Same class of gap remains for other unlisted punctuation (",", ";", "-",
"="...). Happy to generalize _canonicalKeyName() to accept any single
printable ASCII character instead of an allow-list if you would prefer that
shape.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The period key cannot be named anywhere in the keybinding system: it is
absent from KebindingResolver._namedKeys, _canonicalKeyName() rejects it (it
only special-cases "/", ":", "?" and ""), and _eventKeyName() returns null
for Qt.Key_Period.
The result is a silent no-op rather than an error. A user writing
gets an entry that parses to null, is dropped as invalid, and never binds --
indistinguishable from a typo, and with nothing in the log to say why. Since
P2.5's whole premise is that alternative layouts can move any key they like,
a punctuation key that cannot be expressed at all is a real gap.
Fixes it the same way the four existing punctuation keys are handled:
_namedKeys gains "period"/"." -> Qt.Key_Period, _canonicalKeyName() accepts
".", and eventKeyName() prefers the symbol form -- that last part matters,
because both spellings map to one Qt.Key*, and only one canonical string may
come out of _eventKeyName() or it can never compare equal to what
_canonicalKeyName() produced.
Adds a regression check ("a punctuation key (".") can be bound from config")
covering the real user path: the override is accepted, "." reaches
toggle_hidden, its previous Ctrl+H stops working per the full-replacement
policy, and "/" -- which always worked -- still searches. Verified to fail
without the fix:
override accepted=false "."->toggle_hidden=false.CheckKeybindings' own NAMED_KEYS also needed "." so the harness can express
it: it is deliberately independent of the resolver's table, and its
single-character fallback computes Qt.Key_A + (charCode - 'a'), which for "."
is Qt.Key_A - 51.
Selfcheck on this branch: 125 passed, 1 failed, 126 total. The one failure is
the pre-existing thumbnail-video.sh check, which fails the same way on an
unmodified master build here (it needs ffmpegthumbnailer, which this machine
does not have -- see the separate PR).
Same class of gap remains for other unlisted punctuation (",", ";", "-",
"="...). Happy to generalize _canonicalKeyName() to accept any single
printable ASCII character instead of an allow-list if you would prefer that
shape.