set shortcuts for console's resize buttons by jarolrod · Pull Request #281 · bitcoin-core/gui

jarolrod · 2021-04-14T17:05:57Z

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

hebasto · 2021-04-14T17:18:26Z

Concept ACK.

jarolrod · 2021-04-14T17:33:25Z

Updated 0caccb5 -> 3282d36

Changes:

updated spacing to adhere to clang-format

hebasto · 2021-04-14T17:40:08Z

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?

jarolrod · 2021-04-14T17:42:06Z

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

src/qt/guiutil.cpp

jarolrod · 2021-04-14T18:32:20Z

Updated from 3282d36 -> ab95506

Changes:

Action can be set on the button directly. Remove the parent parameter from function and docs.

hebasto · 2021-04-14T21:47:53Z

@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 :)

hebasto · 2021-04-18T12:56:56Z

@jarolrod

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

promag

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?

hebasto · 2021-04-19T18:26:53Z

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?

jarolrod · 2021-04-19T18:27:39Z

@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.

hebasto · 2021-04-19T18:29:31Z

... 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.

hebasto · 2021-04-19T18:32:06Z

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.

Talkless

Concept ACK

src/qt/guiutil.cpp

jarolrod · 2021-05-14T19:42:45Z

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.

hebasto

ACK 2d2632a

src/qt/guiutil.h

hebasto · 2021-05-15T11:02:43Z

src/qt/rpcconsole.cpp

    ui->fontBiggerButton->setIcon(platformStyle->SingleColorIcon(":/icons/fontbigger"));
+    //: Main shortcut to increase the RPC console font size.
+    ui->fontBiggerButton->setShortcut(tr("Ctrl++"));
+    //: Secondary shortcut to increase the RPC console font size.


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?

I think this is ok as is. If we include that i'd be worried if someone translates into Ctrl+Shift+_.

Co-authored-by: Jarol Rodriguez <[email protected]>

jarolrod · 2021-05-17T19:40:46Z

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:

hebasto

re-ACK 2a45134

Talkless · 2021-05-19T17:55:21Z

src/qt/rpcconsole.cpp

+                    .arg("<b>" + ui->clearButton->shortcut().toString(QKeySequence::NativeText) + "</b>") +
+                "<br>" +
+                tr("Use %1 and %2 to increase or decrease the font size.")
+                    .arg("<b>" + ui->fontBiggerButton->shortcut().toString(QKeySequence::NativeText) + "</b>")


nit: .arg(foo, bar) overload could be used instead of .arg(foo).arg(bar)

But isn't the current code more readable?

Yes, maybe, though it could be something like

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

Not that bad.

I think that should be debated here: #331

@jarolrod

I think that should be debated here: #331

Done :)

Yes that ends debate.

Talkless

tACK 2a45134, tested on Debian Sid with Qt 5.15.2, shortcuts still work.

3a171f7 logging: fix logging empty threadname (klementtan) Pull request description: Currently, `leveldb` background thread does not have a thread name and as a result, an empty thread name is logged. This PR fixes this by logging thread name as `"unknown"` if the thread name is empty On master: ```txt 2022-06-02T14:30:38Z [] [leveldb:debug] Generated table #281@0: 1862 keys, 138303 bytes ``` On this PR: ```txt 2022-06-02T14:30:38Z [unknown] [leveldb:debug] Generated table #281@0: 1862 keys, 138303 bytes ``` ACKs for top commit: laanwj: Code review ACK 3a171f7 hebasto: ACK 3a171f7 Tree-SHA512: 0af0fa5c4ddd3640c6dab9595fe9d97f74d0e0f4b41287a6630cf8ac5a21240250e0659ec4ac5a561e888d522f5304bf627104de2aba0fd0a86c1222de0897c2

jarolrod changed the title ~~qt: set shortcuts for console's resize buttons~~ set shortcuts for console's resize buttons Apr 14, 2021

hebasto added the Feature label Apr 14, 2021

jarolrod force-pushed the mul-shortcuts-resize branch from 0caccb5 to 3282d36 Compare April 14, 2021 17:31

hebasto reviewed Apr 14, 2021

View reviewed changes

src/qt/guiutil.cpp Outdated Show resolved Hide resolved

jarolrod force-pushed the mul-shortcuts-resize branch from 3282d36 to ab95506 Compare April 14, 2021 18:29

promag reviewed Apr 19, 2021

View reviewed changes

Talkless suggested changes Apr 25, 2021

View reviewed changes

src/qt/guiutil.cpp Outdated Show resolved Hide resolved

src/qt/guiutil.cpp Outdated Show resolved Hide resolved

src/qt/guiutil.cpp Outdated Show resolved Hide resolved

hebasto added this to the 22.0 milestone May 8, 2021

qt: Use native presentation of shortcut

4ee9ee7

jarolrod force-pushed the mul-shortcuts-resize branch 2 times, most recently from cfea5c7 to 2d2632a Compare May 14, 2021 19:31

hebasto approved these changes May 15, 2021

View reviewed changes

hebasto and others added 2 commits May 17, 2021 14:01

qt: Add GUIUtil::AddButtonShortcut

a2e122f

Co-authored-by: Jarol Rodriguez <[email protected]>

qt: Add shortcuts for console font resize buttons

2a45134

Co-authored-by: Jarol Rodriguez <[email protected]>

jarolrod force-pushed the mul-shortcuts-resize branch from 2d2632a to 2a45134 Compare May 17, 2021 19:37

hebasto approved these changes May 17, 2021

View reviewed changes

hebasto added the UX All about "how to get things done" label May 17, 2021

hebasto removed the Feature label May 17, 2021

Talkless reviewed May 19, 2021

View reviewed changes

hebasto mentioned this pull request May 19, 2021

Make RPC console welcome message translation-friendly #331

Merged

Talkless approved these changes May 20, 2021

View reviewed changes

laanwj merged commit 710c8ba into bitcoin-core:master May 20, 2021

jarolrod deleted the mul-shortcuts-resize branch May 20, 2021 21:20

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 21, 2021

Merge bitcoin-core/gui#281: set shortcuts for console's resize buttons

4cbc1de

jarolrod mentioned this pull request May 21, 2021

Allow prompt icon to be colorized #330

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 710c8ba into merged_master (Bitcoin PR bitcoin-core/gui#281)

ed64f56

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

jarolrod commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Apr 14, 2021

Uh oh!

jarolrod commented Apr 14, 2021

Uh oh!

hebasto commented Apr 14, 2021

Uh oh!

jarolrod commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jarolrod commented Apr 14, 2021

Uh oh!

hebasto commented Apr 14, 2021

Uh oh!

hebasto commented Apr 18, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod commented Apr 19, 2021

Uh oh!

hebasto commented Apr 19, 2021

Uh oh!

hebasto commented Apr 19, 2021

Uh oh!

Talkless left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jarolrod commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarolrod commented May 17, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Talkless left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jarolrod commented Apr 14, 2021 •

edited

Loading

jarolrod commented Apr 14, 2021 •

edited

Loading

hebasto commented Apr 19, 2021 •

edited

Loading

Talkless left a comment •

edited

Loading

jarolrod commented May 14, 2021 •

edited

Loading