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 #101991 Ignore score command shortcuts in menus #5890

Merged
merged 1 commit into from
Apr 20, 2020

Conversation

shoogle
Copy link
Contributor

@shoogle shoogle commented Mar 30, 2020

Resolves: https://musescore.org/en/node/101991

Previously, certain keyboard shortcuts were active in menus when they shouldn't be. For example, pressing the letter keys A to G while inside a menu would cause notes to be added to the score, and S would add a slur. This was problematic because letter keys are traditionally used to open menu items (e.g. pressing A in the Help menu should open the About dialog).

This commit prevents unwanted edits from the menus by terminating the edit operation if the score does not have focus. The shortcut is still active, however, so this commit DOES NOT restore the traditional behaviour where pressing letter keys opens menu items. As such, this is a quick fix rather than a long-term solution.

I investigated whether it would be possible to properly disable or override score shortcuts in the menus. I tried various methods:

  • Install event filters on the menubar, menus and submenus to accept all Qt::ShortcutOverride events. This worked in a bare-bones Qt project but not in MuseScore for some reason (shortcuts were triggered despite filters).

  • Change shortcut context from Qt::WidgetWithChildrenShortcut to the default Qt::WindowShortcut for all score actions. This worked to deactivate shortcuts in the menus, but it made them active in other parts of the program. For example, pressing Spacebar while on a checkbox in the Inspector would cause playback to begin (i.e. it didn't check the checkbox).

The ideal solution would probably combine these methods. I had some success changing the shortcut context to Qt::WindowShortcut and then filtering events on checkboxes and other widgets. However, this would require filtering events on every single widget that implements its own keypress handling, hence I decided to stick with the easy (albeit incomplete) fix for the time being.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • [N/A] I created the test (mtest, vtest, script test) to verify the changes I made

@Jojo-Schmitz
Copy link
Contributor

Not shown here, but the build (actually the mtests part) failed on Travice CI, see https://travis-ci.org/github/musescore/MuseScore/jobs/668644613#L4430-L4433. Might just be a timeout though

shoogle added a commit to shoogle/MuseScore that referenced this pull request Apr 15, 2020
Previously, certain keyboard shortcuts were active in menus when they
shouldn't be. For example, pressing the letter keys A to G while inside
a menu would cause notes to be added to the score, and S would add a slur.
This was problematic because letter keys are traditionally used to open
menu items (e.g. pressing A in the Help menu should open the About dialog).

This commit prevents unwanted edits from the menus by terminating the edit
operation if the score does not have focus. The shortcut is still active,
however, so this commit DOES NOT restore the traditional behaviour where
pressing letter keys opens menu items. As such, this is a quick fix rather
than a long-term solution. An investigation showed that a better solution
is not possible without significant changes to the shortcut system.

See description in PR musescore#5890 for commentary on various potential ways to solve
this issue that were investigated and rejected in favor of this quick fix.
@Jojo-Schmitz
Copy link
Contributor

Travis still isn't happy, see https://travis-ci.org/github/musescore/MuseScore/jobs/675319311#L4405-L4411
Seems to hang in tst_sfzloop, the 2nd to last mtest

shoogle added a commit to shoogle/MuseScore that referenced this pull request Apr 15, 2020
Previously, certain keyboard shortcuts were active in menus when they
shouldn't be. For example, pressing the letter keys A to G while inside
a menu would cause notes to be added to the score, and S would add a slur.
This was problematic because letter keys are traditionally used to open
menu items (e.g. pressing A in the Help menu should open the About dialog).

This commit prevents unwanted edits from the menus by terminating the edit
operation if the score does not have focus. The shortcut is still active,
however, so this commit DOES NOT restore the traditional behaviour where
pressing letter keys opens menu items. As such, this is a quick fix rather
than a long-term solution. An investigation showed that a better solution
is not possible without significant changes to the shortcut system.

See description in PR musescore#5890 for commentary on various potential ways to solve
this issue that were investigated and rejected in favor of this quick fix.
@Jojo-Schmitz
Copy link
Contributor

And again...
I can't explain why, don't see any connection to your code change

shoogle added a commit to shoogle/MuseScore that referenced this pull request Apr 16, 2020
Previously, certain keyboard shortcuts were active in menus when they
shouldn't be. For example, pressing the letter keys A to G while inside
a menu would cause notes to be added to the score, and S would add a slur.
This was problematic because letter keys are traditionally used to open
menu items (e.g. pressing A in the Help menu should open the About dialog).

This commit prevents unwanted edits from the menus by terminating the edit
operation if the score does not have focus. The shortcut is still active,
however, so this commit DOES NOT restore the traditional behaviour where
pressing letter keys opens menu items. As such, this is a quick fix rather
than a long-term solution. An investigation showed that a better solution
is not possible without significant changes to the shortcut system.

See description in PR musescore#5890 for commentary on various potential ways to solve
this issue that were investigated and rejected in favor of this quick fix.
Previously, certain keyboard shortcuts were active in menus when they
shouldn't be. For example, pressing the letter keys A to G while inside
a menu would cause notes to be added to the score, and S would add a slur.
This was problematic because letter keys are traditionally used to open
menu items (e.g. pressing A in the Help menu should open the About dialog).

This commit prevents unwanted edits from the menus by terminating the edit
operation if the score does not have focus. The shortcut is still active,
however, so this commit DOES NOT restore the traditional behaviour where
pressing letter keys opens menu items. As such, this is a quick fix rather
than a long-term solution. An investigation showed that a better solution
is not possible without significant changes to the shortcut system.

See description in PR musescore#5890 for commentary on various potential ways to solve
this issue that were investigated and rejected in favor of this quick fix.
@@ -5879,6 +5879,12 @@ void MuseScore::cmd(QAction* a)
tr("Command %1 not valid in current state").arg(cmdn));
return;
}
if (sc->assignedWidget() == MsWidget::SCORE_TAB
&& QApplication::focusWidget()
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Apr 16, 2020

Choose a reason for hiding this comment

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

So this additional condition fixes the Travis mtest hang? Black Magic to me...

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 figured there might not be a widget that has focus during the tests since there's no GUI. It was just a guess though.

Copy link
Contributor

Choose a reason for hiding this comment

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

An excellent one it seems, an 'educated guess' I'd say ;-)

@anatoly-os anatoly-os merged commit 023b71f into musescore:master Apr 20, 2020
anatoly-os added a commit that referenced this pull request Apr 20, 2020
@anatoly-os
Copy link
Contributor

@shoogle I had to revert the changes since they broke note input: https://musescore.org/node/304164.

@shoogle shoogle deleted the menu-mnemonics branch April 23, 2020 02:09
shoogle added a commit to shoogle/MuseScore that referenced this pull request Apr 23, 2020
Slightly better fix than originally attempted in PR musescore#5890. This time
editing is terminated if the main window does not have focus, which
means that it is safe to use toolbar buttons that take focus away from
the score, such as the buttons for Voices 1 to 4.

As with PR musescore#5890, these changes do not allow letter keys to open menu
items. That would require substantial changes to the shortcut system
as discussed in PR musescore#5890.
shoogle added a commit to shoogle/MuseScore that referenced this pull request Apr 23, 2020
Slightly better fix than originally attempted in PR musescore#5890. This time
editing is terminated if the main window does not have focus, which
means that it is safe to use toolbar buttons that take focus away from
the score, such as the buttons for Voices 1 to 4.

As with PR musescore#5890, these changes do not allow letter keys to open menu
items. That would require substantial changes to the shortcut system
as discussed in PR musescore#5890.
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.

4 participants