Skip to content

Conversation

@letsfindaway
Copy link
Collaborator

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

@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.

@letsfindaway letsfindaway force-pushed the refactor-key-combination branch from bbf87c3 to a514007 Compare February 12, 2025 13:58
@letsfindaway letsfindaway force-pushed the refactor-key-combination branch from a514007 to 0ddd01d Compare March 13, 2025 07:13
- Qt6 introduced a new class QKeyCombination
- in Qt5, a simple int was used
- use the proper functions for each Qt version in UBShortcutManager
@letsfindaway letsfindaway force-pushed the refactor-key-combination branch from 0ddd01d to 16c48b7 Compare August 3, 2025 12:23
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