Add keyboard shortcuts to context menus #362

pull luke-jr wants to merge 3 commits into bitcoin-core:master from luke-jr:kbshortcuts_context changing 6 files +33 −33
  1. luke-jr commented at 7:42 pm on June 12, 2021: member

    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.

  2. GUI: Restore keyboard shortcuts for context menu entries 
    This partially reverts f385ad765174afb02e60900581612a19c143cf83.
    02b5263cd4
  3. GUI: Add keyboard shortcuts for other context menus 94e7cdd7e0
  4. hebasto commented at 7:47 pm on June 12, 2021: member

    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

  5. hebasto requested review from jarolrod on Jun 12, 2021
  6. hebasto requested review from promag on Jun 12, 2021
  7. hebasto added the label Bug on Jun 12, 2021
  8. hebasto added the label UI on Jun 12, 2021
  9. hebasto added this to the milestone 22.0 on Jun 12, 2021
  10. in src/qt/rpcconsole.cpp:680 in 94e7cdd7e0 outdated 
    680-        peersTableContextMenu->addAction(ts.ban_for + " " + tr("1 day"), [this] { banSelectedNode(60 * 60 * 24); });
681-        peersTableContextMenu->addAction(ts.ban_for + " " + tr("1 week"), [this] { banSelectedNode(60 * 60 * 24 * 7); });
682-        peersTableContextMenu->addAction(ts.ban_for + " " + tr("1 year"), [this] { banSelectedNode(60 * 60 * 24 * 365); });
683+        peersTableContextMenu->addAction(tr("&Disconnect"), this, &RPCConsole::disconnectSelectedNode);
684+        peersTableContextMenu->addAction(ts.ban_for + " " + tr("1 &hour"), [this] { banSelectedNode(60 * 60); });
685+        peersTableContextMenu->addAction(ts.ban_for + " " + tr("1 &day"), [this] { banSelectedNode(60 * 60 * 24); });
    jarolrod commented at 7:56 pm on June 13, 2021:

    This causes a collision with &Disconnect on line 678, lets avoid collisions:

    0        peersTableContextMenu->addAction(ts.ban_for + " " + tr("1 d&ay"), [this] { banSelectedNode(60 * 60 * 24); });
    hebasto commented at 6:00 am on June 14, 2021:
    It reminds me #129.
  11. jarolrod commented at 8:53 pm on June 13, 2021: member

    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:

     0diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
 1index 3d632ec70..3266ca13c 100644
 2--- a/src/qt/bitcoingui.cpp
 3+++ b/src/qt/bitcoingui.cpp
 4@@ -28,6 +28,7 @@
 5 
 6 #ifdef Q_OS_MAC
 7 #include <qt/macdockiconhandler.h>
 8+    extern void qt_set_sequence_auto_mnemonic(bool b);
 9 #endif
10 
11 #include <functional>
12@@ -214,6 +215,7 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
13 
14 #ifdef Q_OS_MAC
15     m_app_nap_inhibitor = new CAppNapInhibitor;
16+    qt_set_sequence_auto_mnemonic(true);
17 #endif
  12. jarolrod commented at 8:54 pm on June 13, 2021: member

    @hebasto

    hebasto added this to the 22.0 milestone yesterday

    What about the translation freeze?

  13. hebasto commented at 6:05 am on June 14, 2021: member

    @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 https://github.com/bitcoin/bitcoin/issues/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”.

  14. hebasto commented at 6:23 am on June 14, 2021: member
    Approach ACK 94e7cdd7e04ca79f89474df1400e1a189e068939, modulo #362 (review).
  15. Bugfix: GUI: Use a different shortcut for "1 d&ay" banning, due to conflict with "&Disconnect" e4c916a0ea
  16. hebasto approved
  17. hebasto commented at 7:44 am on June 14, 2021: member

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

    Also checked that 02b5263cd4e02aa540cab35c2bf6cf9eda3522ae reverts 79311750b58d650d49a3f0edd59d31dd132ab8c0.

  18. jarolrod commented at 6:02 pm on June 14, 2021: member
    Code Review ACK e4c916a
  19. hebasto merged this on Jun 14, 2021
  20. hebasto closed this on Jun 14, 2021
  21. sidhujag referenced this in commit 69e3599eaf on Jun 16, 2021
  22. hebasto referenced this in commit d3203a99d8 on Aug 23, 2021
  23. gwillen referenced this in commit a92037772b on Jun 1, 2022
  24. bitcoin-core locked this on Aug 16, 2022


luke-jr hebasto jarolrod


promag

Labels
Bug UI

Milestone
22.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-23 00:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me