Skip to content

fix(keyboardstate.cc): forgotten Win modifier + Qt >= 6.2 optimization#2673

Merged
xiaoyifang merged 1 commit intoxiaoyifang:stagedfrom
zaufi:staged
Jan 12, 2026
Merged

fix(keyboardstate.cc): forgotten Win modifier + Qt >= 6.2 optimization#2673
xiaoyifang merged 1 commit intoxiaoyifang:stagedfrom
zaufi:staged

Conversation

@zaufi
Copy link
Contributor

@zaufi zaufi commented Jan 8, 2026

The original code didn't check for the Win modifier. I left the previous implementation untouched1 and rewrote it for Qt 6.2 and later, where the QFlags::testFlags() method was introduced.

Footnotes

  1. cuz it doesn't work for me, even updated 🤔

@xiaoyifang
Copy link
Owner

/home/runner/work/goldendict-ng/goldendict-ng/src/keyboardstate.cc:7:10: fatal error: QtVersionChecks: No such file or directory
    7 | #include <QtVersionChecks>
      |          ^~~~~~~~~~~~~~~~~
compilation terminated.

{
auto modifiers = QApplication::queryKeyboardModifiers();

#if QT_VERSION >= QT_VERSION_CHECK( 6, 2, 0 )
Copy link
Owner

@xiaoyifang xiaoyifang Jan 9, 2026

Choose a reason for hiding this comment

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

this should be always true (in current situation). just keep this branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any minimum version required in the CML:

find_package(Qt6 REQUIRED COMPONENTS ${GD_QT_COMPONENTS})

Shall I set it? To what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the patch with the minimum required Qt version 6.2

@xiaoyifang xiaoyifang enabled auto-merge (squash) January 12, 2026 01:01
@xiaoyifang xiaoyifang merged commit 616ff17 into xiaoyifang:staged Jan 12, 2026
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants