Remove QShortcutMap::matches() and use QKeySequence::matches() instead

The custom matching function was added in 4d4857027db3 with the rationale
that people were mixing up Key_hyphen and Key_Minus all the time, and
tried to solve it by treating the two as one and the same in the match
function.

Unfortunately this doesn't work in practice, as when a keyboard event
comes in we resolve a set of possible key sequences from that, and then
look those up in the list of sorted shortcut sequences. That lookup
is just a binary search, and does not take into account the added
logic of the custom matching function. So the binary search will fail
to find the matching key sequence, and as a result we never get a chance
to call matches() with a potentially malleable key sequence (for example
Qt::Key_Minus when the shortcut is Qt::Key_hyphen or vice versa).

The only case we do hit the matches function is if we by chance happen
to land the binary search iterator on the "unmatched" shortcut, e.g.
Qt::Key_hyphen, but this relies on there not being any other shortcuts
that sort between Qt::Key_Minus and Qt::Key_hyphen.

Task-number: QTBUG-116873
Change-Id: Iaa90991911f32276e29e37e8c7ae87643898bfc9
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
This commit is contained in:
Tor Arne Vestbø 2023-09-15 22:42:32 +02:00
parent 709c93083e
commit 0bfb25d178
2 changed files with 2 additions and 37 deletions

View File

@ -369,7 +369,7 @@ bool QShortcutMap::hasShortcutForKeySequence(const QKeySequence &seq) const
auto it = std::lower_bound(d->shortcuts.cbegin(), itEnd, entry);
for (;it != itEnd; ++it) {
if (matches(entry.keySequence, (*it).keySequence) == QKeySequence::ExactMatch
if (entry.keySequence.matches(it->keySequence) == QKeySequence::ExactMatch
&& (*it).correctContext() && (*it).enabled) {
return true;
}
@ -418,7 +418,7 @@ QKeySequence::SequenceMatch QShortcutMap::find(QKeyEvent *e, int ignoredModifier
const auto itEnd = d->shortcuts.constEnd();
auto it = std::lower_bound(d->shortcuts.constBegin(), itEnd, entry);
for (; it != itEnd; ++it) {
QKeySequence::SequenceMatch match = matches(entry.keySequence, (*it).keySequence);
QKeySequence::SequenceMatch match = entry.keySequence.matches(it->keySequence);
qCDebug(lcShortcutMap) << " -" << match << "for shortcut" << it->keySequence;
// If we got a valid match, there might still be more keys to check against,
@ -527,40 +527,6 @@ void QShortcutMap::createNewSequences(QKeyEvent *e, QList<QKeySequence> &ksl, in
}
}
/*! \internal
Basically the same function as QKeySequence::matches(const QKeySequence &seq) const
only that is specially handles Key_hyphen as Key_Minus, as people mix these up all the time and
they conceptually the same.
*/
QKeySequence::SequenceMatch QShortcutMap::matches(const QKeySequence &seq1,
const QKeySequence &seq2) const
{
uint userN = seq1.count(),
seqN = seq2.count();
if (userN > seqN)
return QKeySequence::NoMatch;
// If equal in length, we have a potential ExactMatch sequence,
// else we already know it can only be partial.
QKeySequence::SequenceMatch match = (userN == seqN
? QKeySequence::ExactMatch
: QKeySequence::PartialMatch);
for (uint i = 0; i < userN; ++i) {
int userKey = seq1[i].toCombined(),
sequenceKey = seq2[i].toCombined();
if ((userKey & Qt::Key_unknown) == Qt::Key_hyphen)
userKey = (userKey & Qt::KeyboardModifierMask) | Qt::Key_Minus;
if ((sequenceKey & Qt::Key_unknown) == Qt::Key_hyphen)
sequenceKey = (sequenceKey & Qt::KeyboardModifierMask) | Qt::Key_Minus;
if (userKey != sequenceKey)
return QKeySequence::NoMatch;
}
return match;
}
/*! \internal
Converts keyboard button states into modifier states
*/

View File

@ -62,7 +62,6 @@ private:
void dispatchEvent(QKeyEvent *e);
QKeySequence::SequenceMatch find(QKeyEvent *e, int ignoredModifiers = 0);
QKeySequence::SequenceMatch matches(const QKeySequence &seq1, const QKeySequence &seq2) const;
QList<const QShortcutEntry *> matches() const;
void createNewSequences(QKeyEvent *e, QList<QKeySequence> &ksl, int ignoredModifiers);
void clearSequence(QList<QKeySequence> &ksl);