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

Add keyboard shortcuts to context menus #362

Merged
merged 3 commits into from
Jun 14, 2021

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Jun 12, 2021

Various keyboard shortcuts were lost in #263; this restores them, and also adds new ones for other context menus.

Note that with a context menu open, simply the shortcut by itself (no Alt) is used.

@hebasto
Copy link
Member

hebasto commented Jun 12, 2021

Note that with a context menu open, simply the shortcut by itself (no Alt) is used.

As pointed by @luke-jr on IRC:

<luke-jr> no idea, it's an OS behaviour, not Qt-specific

@hebasto hebasto requested review from jarolrod and promag June 12, 2021 19:49
@hebasto hebasto added Bug Something isn't working UI All about "look and feel" labels Jun 12, 2021
@hebasto hebasto added this to the 22.0 milestone Jun 12, 2021
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

Concept ACK

This introduces mnemonic shortcuts for the context menu actions: QShortcut Documentation

On certain widgets, using '&' in front of a character will automatically create a mnemonic (a shortcut) for that character, e.g. "E&xit" will create the shortcut Alt+X (use '&&' to display an actual ampersand). The widget might consume and perform an action on a given shortcut.

This is a UX improvement and improves the accessibility of the GUI as well as convenience.

Tested functionality on Arch Linux Qt 5.15.2, Windows 7,8,10 cross-compiled from Linux, and macOS 11.3 Qt 5.15.2.

Linux

Mnemonic shortcuts work well. I need to hit ALT in order to learn about what the shortcuts are. Because of this, the OP is misleading in saying:

Note that with a context menu open, simply the shortcut by itself (no Alt) is used.
A user needs to know about a shortcut before they can use it, and to know about it ALT is required.

Master PR
ksnip_20210613-020514 ksnip_20210613-020440

Windows 7-10

The mnemonics work on Windows 7,8, and 10. But again, ALT is required to learn about the shortcuts.

Master PR
gui-363-real-master gui-362-pr

macOS

mnemonics aren't really supported on macOS, so it is ok that they do not work well on macOS:

However, because mnemonic shortcuts do not fit in with Aqua's guidelines, Qt will not show the shortcut character underlined.

If there is no mechanism to highlight the shortcut, then it is useless as a user would need to dig through the source code to learn about them.

Testing on macOS, the mnemonics work if you know the correct key, press it, then press enter. This is surprising as Qt docs states:

On Mac, shortcuts are disabled by default. Call qt_set_sequence_auto_mnemonic() to enable them.

I see no difference in behavior applying the following patch, so i think this is a poorly documented part of Qt and must have changed:

diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
index 3d632ec70..3266ca13c 100644
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -28,6 +28,7 @@
 
 #ifdef Q_OS_MAC
 #include <qt/macdockiconhandler.h>
+    extern void qt_set_sequence_auto_mnemonic(bool b);
 #endif
 
 #include <functional>
@@ -214,6 +215,7 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
 
 #ifdef Q_OS_MAC
     m_app_nap_inhibitor = new CAppNapInhibitor;
+    qt_set_sequence_auto_mnemonic(true);
 #endif

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
@jarolrod
Copy link
Member

@hebasto

hebasto added this to the 22.0 milestone yesterday

What about the translation freeze?

@hebasto
Copy link
Member

hebasto commented Jun 14, 2021

@jarolrod

Testing on macOS, the mnemonics work if you know the correct key, press it, then press enter. This is surprising as Qt docs states:

On Mac, shortcuts are disabled by default. Call qt_set_sequence_auto_mnemonic() to enable them.

Sound like a Qt bug :)


hebasto added this to the 22.0 milestone yesterday

What about the translation freeze?

From bitcoin/bitcoin#20851:

2021-06-01 heavy_check_mark

* Open Transifex translations for 22

* Soft translation string freeze (no large or unnecessary string changes until release)

* Finalize and close translations for 0.20

2021-06-15 construction

* Feature freeze (bug fixes only until release)

* Translation string freeze (no more source language changes until release)

As this PR fixes the behavior regression introduced in #263, it could be considered as "necessary string changes".

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Approach ACK 94e7cdd, modulo #362 (comment).

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK e4c916a, tested on Linux Mint 20.1 (Qt 5.12.8).

Also checked that 02b5263 reverts 7931175.

@jarolrod
Copy link
Member

Code Review ACK e4c916a

@hebasto hebasto merged commit 3f68f02 into bitcoin-core:master Jun 14, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 16, 2021
hebasto added a commit that referenced this pull request Aug 23, 2021
7c33e3a qt: Add missing mnemonics in menu bar options (Shashwat)

Pull request description:

  Since #362 we have defaulted to add mnemonic shortcuts for the context menus.
  The Window -> Minimize option and File -> Load PSBT from clipboard were hitherto missing a mnemonic shortcut. This PR adds mnemonic shortcuts for them

  Changes introduced in this PR:
  | Master | PR |
  | ----------| ---- |
  | ![Screenshot from 2021-08-23 23-10-07](https://user-images.githubusercontent.com/85434418/130494098-c65ec9da-c3f1-4243-9b3d-0c87cb677825.png) | ![Screenshot from 2021-08-23 23-08-41](https://user-images.githubusercontent.com/85434418/130494083-849ffd14-05e9-4a6d-bdc3-b3e55b8a8861.png)|
  |![Screenshot from 2021-08-23 23-10-21](https://user-images.githubusercontent.com/85434418/130494233-1b2e8838-c5d4-48a8-9abf-a4acc8d6146c.png)|![Screenshot from 2021-08-23 23-09-00](https://user-images.githubusercontent.com/85434418/130494181-dcdbf119-a2c6-469d-a6c9-3021506ab40b.png)|

ACKs for top commit:
  jarolrod:
    tACK 7c33e3a
  hebasto:
    ACK 7c33e3a, tested on Linux Mint 20.2 (Qt 5.12.8).

Tree-SHA512: 32f201ae7716b19ca123856292f8bfa0d805f6c7271ac1b5e08eff6b95017443754c8a76e8396ccca1869a57384e11016cbd99d63ccdd2fae6da4eaf3ae32298
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
Re-added line I (accidentally) dropped in 071510d6a8678594fd33458c6b404ad63dfd60b4
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Something isn't working UI All about "look and feel"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants