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

[4.4.4] MacOSShortcutsInstanceModel: Normalise shortcuts spelling in mapping #25273

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

Conversation

cbjeukendrup
Copy link
Contributor

@cbjeukendrup cbjeukendrup commented Oct 22, 2024

Resolves: #24497
Resolves: #25314
Resolves: #25321
Resolves: #25373 (according to #25373 (comment))

Ports: #25489

It seems that the problem was that both Ctrl+Meta+E and Meta+Ctrl+E got in the list, which confuses QML, which considers them ambiguous.

QString keyStr = keyCodeToString(keyboard, keyNativeCode);
QString modifStr = keyModifiersToString(keyNativeModifiers);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these changes? Could you please explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to return QKeySequence from this method to be able to get consistent modifier order. It seems to make no sense to convert from Qt modifiers to native modifiers and then from native modifiers back to Qt string notation, and then from Qt string notation to QKeySequence. Instead, we can go directly from Qt modifiers to QKeySequence.

Copy link
Contributor

Choose a reason for hiding this comment

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

we need to think about how risky this is for the patch

Copy link

Choose a reason for hiding this comment

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

Owing to the capricious nature of shortcuts, I suspect this warrants more extensive testing than we currently have time for in 4.4.3. Let's target this for 4.5.

@DmitryArefiev
Copy link
Contributor

Tested #24497 on Mac13.6 - FIXED

@zacjansheski
Copy link
Contributor

Tested on MacOS 14, Windows 11, Ubuntu 22.04.3. Approved
#24497 FIXED
#25314 FIXED

@cbjeukendrup cbjeukendrup force-pushed the port/4.4.3/normalise_shortcut_spelling_in_mapping branch from c38f57f to 834d407 Compare November 13, 2024 00:43
@cbjeukendrup cbjeukendrup changed the title MacOSShortcutsInstanceModel: Normalise shortcuts spelling in mapping [4.4.4] MacOSShortcutsInstanceModel: Normalise shortcuts spelling in mapping Nov 13, 2024
@Eism
Copy link
Contributor

Eism commented Nov 14, 2024

@cbjeukendrup please change the merge branch to 4.4.4

@cbjeukendrup cbjeukendrup changed the base branch from 4.4.3 to 4.4.4 November 14, 2024 09:26
@DmitryArefiev
Copy link
Contributor

@cbjeukendrup The builds are still 4.4.3. Can you rebase it on 4.4.4 please? (I'll check it briefly before merge)

@cbjeukendrup cbjeukendrup force-pushed the port/4.4.3/normalise_shortcut_spelling_in_mapping branch from 834d407 to ebb9d29 Compare November 14, 2024 16:37
@cbjeukendrup
Copy link
Contributor Author

It shouldn't matter much (only the commit that changed the version number was not yet present on this branch) but I've rebased anyway for now.

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.

5 participants