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.

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

refactor: avoid QKeyCombination warnings on Qt6 #1079

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

letsfindaway
Copy link
Collaborator

This PR is an attempt to fix #1077. See there for a discussion of the problem.

- Qt6 introduced a new class QKeyCombination
- in Qt5, a simple int was used
- use the proper functions for each Qt version in UBShortcutManager
@Vekhir
Copy link
Contributor

Vekhir commented Sep 6, 2024

This looks like a good solution for the warnings. I find the code more readable now too.

@Vekhir
Copy link
Contributor

Vekhir commented Sep 9, 2024

        if (action->property("builtIn").toBool())
        {
            for (const QKeySequence& shortcut : action->shortcuts())
            {
                shortcuts << shortcut;
            }
        }

https://github.com/OpenBoard-org/OpenBoard/blob/dev/src/core/UBShortcutManager.cpp#L599-L605

Not directly related, but why aren't there ctrlShortcuts added? Are builtIns guaranteed to never have a CTRL? Though it's never checked anywhere else.
I'd expect the QKeySequence(shortcut ^ Qt::CTRL) that's in the other lines, even if it might not actually do anything useful.

@letsfindaway
Copy link
Collaborator Author

Not directly related, but why aren't there ctrlShortcuts added? Are builtIns guaranteed to never have a CTRL? Though it's never checked anywhere else.
I'd expect the QKeySequence(shortcut ^ Qt::CTRL) that's in the other lines, even if it might not actually do anything useful.

The buildins are those which are not defined as action shortcuts but are interpreted from key press events. Here the ignore Ctrl feature is not and cannot be applied. Therefore they are not added to the ctrlShortcuts because they are not affected by the ignoreCtrl flag.

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