set shortcuts for console’s resize buttons #281

pull jarolrod wants to merge 3 commits into bitcoin-core:master from jarolrod:mul-shortcuts-resize changing 3 files +43 −14
  1. jarolrod commented at 5:05 pm on April 14, 2021: member

    On master the only way to resize the console font is to manually move your mouse and click the resize buttons. This PR introduces convenient keyboard shortcuts to resize the console font.

    The common resize shortcuts for applications are Ctrl+=/Ctrl++ and Ctrl+-/Ctrl+_. This means that the resize QPushButtons need two shortcuts each, but you cannot assign multiple shortcuts to a QPushButton. See: https://doc.qt.io/qt-5/qabstractbutton.html#shortcut-prop

    To get around this, we introduce a new function in guiutil, which connects a supplied QKeySequence shortcut to a QAbstractButton. This function can be reused in other situations where more than one shortcut is needed for a button.

    PR on macOS PR on Linux
    mac-resize-shortcuts linux-resize-shortcuts
  2. jarolrod renamed this:
    qt: set shortcuts for console's resize buttons
    set shortcuts for console's resize buttons
    on Apr 14, 2021
  3. hebasto added the label Feature on Apr 14, 2021
  4. hebasto commented at 5:18 pm on April 14, 2021: member
    Concept ACK.
  5. jarolrod force-pushed on Apr 14, 2021
  6. jarolrod commented at 5:33 pm on April 14, 2021: member

    Updated 0caccb5 -> 3282d36

    Changes:

    • updated spacing to adhere to clang-format
  7. hebasto commented at 5:40 pm on April 14, 2021: member

    The common resize shortcuts for applications are Ctrl+=/Ctrl++ and Ctrl+-/Ctrl+_.

    Are they and QKeySequence::ZoomIn, QKeySequence::ZoomOut (https://doc.qt.io/qt-5/qkeysequence.html#StandardKey-enum) the same?

  8. jarolrod commented at 5:42 pm on April 14, 2021: member
    @hebasto See: https://doc.qt.io/qt-5/qkeysequence.html#standard-shortcuts. I don’t think it includes Ctrl+= or Ctrl+_
  9. in src/qt/guiutil.cpp:128 in 3282d362c5 outdated 
    119@@ -119,6 +120,14 @@ void setupAddressWidget(QValidatedLineEdit *widget, QWidget *parent)
120     widget->setCheckValidator(new BitcoinAddressCheckValidator(parent));
121 }
122 
123+void setButtonShortcuts(QPushButton* button, QWidget* parent, const QList<QKeySequence>& shortcuts)
124+{
125+    QAction* action = new QAction(parent);
126+    action->setShortcuts(shortcuts);
127+    parent->addAction(action);
128+    parent->connect(action, &QAction::triggered, [button]() { button->animateClick(); });
    hebasto commented at 5:43 pm on April 14, 2021:
    Why the action is added to the parent, not to the button?
    jarolrod commented at 5:56 pm on April 14, 2021:
    right, will fix
    jarolrod commented at 6:30 pm on April 14, 2021:
    addressed in ab95506
  10. jarolrod force-pushed on Apr 14, 2021
  11. jarolrod commented at 6:32 pm on April 14, 2021: member

    Updated from 3282d36 -> ab95506

    Changes:

    • Action can be set on the button directly. Remove the parent parameter from function and docs.
  12. hebasto commented at 9:47 pm on April 14, 2021: member

    @hebasto See: https://doc.qt.io/qt-5/qkeysequence.html#standard-shortcuts. I don’t think it includes Ctrl+= or Ctrl+_

    Keyboard Layout Issues describe exactly this PR :)

  13. hebasto commented at 12:56 pm on April 18, 2021: member

    @jarolrod

    Do you mind looking into https://github.com/hebasto/gui/commits/pr281-alt? It seems more general and concise, no?

  14. promag commented at 6:23 pm on April 19, 2021: contributor
    Any reason to not use https://doc.qt.io/qt-5/qabstractbutton.html#shortcut-prop? And why we want multiple shortcuts for the same button?
  15. hebasto commented at 6:26 pm on April 19, 2021: member

    And why we want multiple shortcuts for the same button?

    https://doc.qt.io/qt-5/qkeysequence.html#keyboard-layout-issues

    Ctrl+Plus is, actually, Ctrl+Equal_Sign on some keyboard layouts, no?

  16. jarolrod commented at 6:27 pm on April 19, 2021: member

    @promag you can only assign one shortcut directly to a button. The reason to have multiple shortcuts is to allow for the following shortcuts on the resize buttons:

    • Smaller = Ctrl+-, Ctrl+_
    • Bigger = Ctrl+=, Ctrl++ These two shortcuts are common and standard for resizing and having both of them helps get around any differences in keyboard layouts. @hebasto’s approach seems to be a more focused approach as we’re unlikely to need 3 shortcuts for a button. Still looking.
  17. hebasto commented at 6:29 pm on April 19, 2021: member

    … we’re unlikely to need 3 shortcuts for a button.

    It will work for any number of shortcuts. But one of them could be considered as “main” one.

  18. hebasto commented at 6:32 pm on April 19, 2021: member

    Any reason to not use https://doc.qt.io/qt-5/qabstractbutton.html#shortcut-prop?

    https://github.com/hebasto/gui/commits/pr281-alt is using it for a “main” shortcut.

  19. in src/qt/guiutil.cpp:125 in ab9550665c outdated 
    119@@ -119,6 +120,14 @@ void setupAddressWidget(QValidatedLineEdit *widget, QWidget *parent)
120     widget->setCheckValidator(new BitcoinAddressCheckValidator(parent));
121 }
122 
123+void setButtonShortcuts(QPushButton* button, const QList<QKeySequence>& shortcuts)
124+{
125+    QAction* action = new QAction(button);
    Talkless commented at 2:57 pm on April 25, 2021:
    nit: could be “QAction *const action” as pointer itself is never changed in a function.
  20. in src/qt/guiutil.cpp:123 in ab9550665c outdated 
    119@@ -119,6 +120,14 @@ void setupAddressWidget(QValidatedLineEdit *widget, QWidget *parent)
120     widget->setCheckValidator(new BitcoinAddressCheckValidator(parent));
121 }
122 
123+void setButtonShortcuts(QPushButton* button, const QList<QKeySequence>& shortcuts)
    Talkless commented at 2:58 pm on April 25, 2021:
    I believe maintainers will want you to split this PR into two commits - adding new utility function and actually using it.
    Talkless commented at 3:00 pm on April 25, 2021:
    This function could be more generic if it would accept QAbstractButton* instead.
  21. Talkless changes_requested
  22. Talkless commented at 3:02 pm on April 25, 2021: none
    Concept ACK
  23. hebasto added this to the milestone 22.0 on May 8, 2021
  24. qt: Use native presentation of shortcut 4ee9ee7236
  25. jarolrod force-pushed on May 14, 2021
  26. jarolrod force-pushed on May 14, 2021
  27. jarolrod commented at 7:42 pm on May 14, 2021: member

    Updated from ab95506 -> 2d2632a

    Main Change: adopt @hebasto’s approach as it is more focused

    Changes from @hebasto’s branch:

    • Rename shortcut function from AddShortcut to AddButtonShortcut
    • Add Doxygen comment for AddButtonShortcut function
    • Use translator comments instead of disambiguation strings

    Will add test coverage in a follow-up PR as some refactors to the current apptests is required.

  28. in src/qt/guiutil.h:71 in 2d2632ac7a outdated 
    61@@ -60,6 +62,12 @@ namespace GUIUtil
62     // Set up widget for address
63     void setupAddressWidget(QValidatedLineEdit *widget, QWidget *parent);
64 
65+    /** Add a shortcut to a QAbstractButton
66+         @param[in] button    QAbstractButton to assign shortcut to
67+         @param[in] shortcut  QKeySequence to use as shortcut
68+     */
69+    void AddButtonShortcut(QAbstractButton* button, const QKeySequence& shortcut);
    hebasto commented at 10:59 am on May 15, 2021:
    One could wonder why this function is useful while we have QAbstractButton::setShortcut?
    jarolrod commented at 7:38 pm on May 17, 2021:
    added an additional note in a2e122f
  29. in src/qt/rpcconsole.cpp:493 in 2d2632ac7a outdated 
    485@@ -486,8 +486,18 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
486         ui->openDebugLogfileButton->setIcon(platformStyle->SingleColorIcon(":/icons/export"));
487     }
488     ui->clearButton->setIcon(platformStyle->SingleColorIcon(":/icons/remove"));
489+
490     ui->fontBiggerButton->setIcon(platformStyle->SingleColorIcon(":/icons/fontbigger"));
491+    //: Main shortcut to increase the RPC console font size.
492+    ui->fontBiggerButton->setShortcut(tr("Ctrl++"));
493+    //: Secondary shortcut to increase the RPC console font size.
    hebasto commented at 11:02 am on May 15, 2021:
    Maybe add a note that a secondary shortcut differs from a main one only by presence/absence of the <Shift> key in the key sequence?
    jarolrod commented at 7:41 pm on May 17, 2021:
    I think this is ok as is. If we include that i’d be worried if someone translates into Ctrl+Shift+_.
  30. hebasto approved
  31. hebasto commented at 11:43 am on May 15, 2021: member
    ACK 2d2632ac7aa0c4ebcffb5fd9d62ff9ef527fc1e6
  32. qt: Add GUIUtil::AddButtonShortcut 
    Co-authored-by: Jarol Rodriguez <jarolrod@tutanota.com>
    a2e122f0fe
  33. qt: Add shortcuts for console font resize buttons 
    Co-authored-by: Jarol Rodriguez <jarolrod@tutanota.com>
    2a45134b56
  34. jarolrod force-pushed on May 17, 2021
  35. jarolrod commented at 7:40 pm on May 17, 2021: member

    updated from 2d2632a -> 2a45134

    Adresses @hebasto’s comment:

    • Added a note to make it clear that the reason the AddButtonShort function exists is to get around the one shortcut at a time limitation of the button’s shortcut property
    • While here, adjust the formatting of my added Doxygen comment to ascribe to the formatting specified in the dev notes

    Doxygen Comment: Screen Shot 2021-05-17 at 1 56 41 PM

  36. hebasto approved
  37. hebasto commented at 7:41 pm on May 17, 2021: member
    re-ACK 2a45134b5694c12546d77cdff541612881f7e3e7
  38. hebasto added the label UX on May 17, 2021
  39. hebasto removed the label Feature on May 17, 2021
  40. in src/qt/rpcconsole.cpp:836 in 2a45134b56 
    845+                "<br>" +
846+                tr("Use up and down arrows to navigate history, and %1 to clear screen.")
847+                    .arg("<b>" + ui->clearButton->shortcut().toString(QKeySequence::NativeText) + "</b>") +
848+                "<br>" +
849+                tr("Use %1 and %2 to increase or decrease the font size.")
850+                    .arg("<b>" + ui->fontBiggerButton->shortcut().toString(QKeySequence::NativeText) + "</b>")
    Talkless commented at 5:55 pm on May 19, 2021:
    nit: .arg(foo, bar) overload could be used instead of .arg(foo).arg(bar)
    hebasto commented at 6:15 pm on May 19, 2021:
    But isn’t the current code more readable?
    Talkless commented at 6:26 pm on May 19, 2021:

    Yes, maybe, though it could be something like

    0                    .arg(
1                    "<b>" + ui->fontBiggerButton->shortcut().toString(QKeySequence::NativeText) + "</b>",
2                    "<b>" + ui->fontSmallerButton->shortcut().toString(QKeySequence::NativeText) + "</b>"
3                    ) +

    Not that bad.

    jarolrod commented at 6:36 pm on May 19, 2021:
    I think that should be debated here: https://github.com/bitcoin-core/gui/pull/331
    hebasto commented at 7:29 pm on May 19, 2021:

    @jarolrod

    I think that should be debated here: #331

    Done :)

    Talkless commented at 6:14 pm on May 20, 2021:
    Yes that ends debate.
  41. Talkless approved
  42. Talkless commented at 6:15 pm on May 20, 2021: none
    tACK 2a45134b5694c12546d77cdff541612881f7e3e7, tested on Debian Sid with Qt 5.15.2, shortcuts still work.
  43. laanwj merged this on May 20, 2021
  44. laanwj closed this on May 20, 2021
  45. jarolrod deleted the branch on May 20, 2021
  46. sidhujag referenced this in commit 4cbc1dee20 on May 21, 2021
  47. gwillen referenced this in commit ed64f56509 on Jun 1, 2022
  48. MarcoFalke referenced this in commit 2cf8c2caea on Jun 3, 2022
  49. bitcoin-core locked this on Aug 16, 2022


jarolrod hebasto promag Talkless

Labels
UX

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-12-22 03:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me