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

fix #288679 and #269262: ensure support of set QStandardKey #4991

Merged
merged 1 commit into from
Feb 10, 2020

Conversation

malaka712
Copy link
Contributor

The used QKeySequence::StandardKey may not be set on all Platforms, as described here (and in table above given link): https://doc.qt.io/archives/qt-4.8/qkeysequence.html#StandardKey-enum

This way the UI has an unsupported StandardKey as a shortcut (in the reported bug, it is Save-As, which has number 63). When now adding a manual shortcut, the Ms::Shortcut object has two shortcuts, the (unsupported) StandardKey and the manual entry. As a result, QAction* Shortcut::action() const will return the StandardKey and the added shortcut is never applied.

This commit double checks that the OS supports the defined StandardKey, and if not, it blocks setting it as a shortcut.

Note: This also applies when clicking on "Reset All Preferences to Default", as the default shortcuts.xml contains the StandardKey causing these issues.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented May 6, 2019

Please fix the issue number in PR- and commit title to #288679, #288769 has got nothing to do with it (obviously a typo).
Maybe add #269262, which seems related and fixed along the way?

@malaka712 malaka712 changed the title fix #288769: ensure support of set QStandardKey fix #288679 and #269262: ensure support of set QStandardKey May 6, 2019
@malaka712
Copy link
Contributor Author

malaka712 commented May 6, 2019

You're right, yes. #269262 is basically the same problem. Issue number fixed, too.
Edit: Oh wait, title in commit is still wrong. Will fix soon.

@Jojo-Schmitz
Copy link
Contributor

Also you need to rebase, not to merge. Try git pull --rebase upstream master && git push -f

@malaka712 malaka712 force-pushed the 288679-add-shortcut branch 2 times, most recently from 1d0b5e0 to 14dd148 Compare May 6, 2019 15:41
@malaka712
Copy link
Contributor Author

With the last changes, the default settings for Save-As are a QKeySequence. The Save-As shortcut is now "Ctrl+Shift+S" on all platforms (Ctrl is translated to Command on a Mac).

The additional checks are not needed with the modified shortcuts.xml, as all used StandardKeys are supported on all platforms (as defined in first table here: https://doc.qt.io/archives/qt-4.8/qkeysequence.html). Yet, I recommend keeping the checks, as these make the code less error-prone, especially, when the shortcuts.xml changes in future.

@Jojo-Schmitz
Copy link
Contributor

What about mscore/data/shortcuts{_AZERTY,-Mac}.xml?

@malaka712
Copy link
Contributor Author

Hm, I have now modified the shortcuts_AZERTY.xml as it is platform independent. But I did not touch shortcuts-Mac.xml, as it is only applied for APPLE (due to mscore/CMakeLists.txt) and the StandardKey for Save-As is supported on Mac OS X.

@Jojo-Schmitz
Copy link
Contributor

Thanks

given platform and replace 'Save As' StandardKey with KeySequence to
enable it on all platforms by default.

Signed-off-by: Patrick <my_web_spam_address@web.de>
@dmitrio95 dmitrio95 merged commit e065bf9 into musescore:master Feb 10, 2020
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.

None yet

3 participants