gui: Add close window shortcut

IPGlider commented at 7:54 pm on April 7, 2019: contributor

CMD+W is the standard shortcut in macOS to close a window without exiting the program.

This adds support to use the shortcut in both main and debug windows.

fanquake added the label GUI on Apr 7, 2019

fanquake added the label macOS on Apr 7, 2019

jonasschnelli commented at 9:20 am on April 8, 2019: contributor

Thanks! Tested ACK 20859aef8b571542772a42b4c413c4a71a6576c5

The macOS HIG about Command+W: https://developer.apple.com/design/human-interface-guidelines/macos/user-interaction/keyboard/

in src/qt/bitcoingui.cpp:413 in 20859aef8b outdated

408@@ -409,6 +409,10 @@ void BitcoinGUI::createActions()
409 
410     connect(new QShortcut(QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_C), this), &QShortcut::activated, this, &BitcoinGUI::showDebugWindowActivateConsole);
411     connect(new QShortcut(QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_D), this), &QShortcut::activated, this, &BitcoinGUI::showDebugWindow);
412+
413+#ifdef Q_OS_MAC

promag commented at 1:58 pm on April 8, 2019:

Do you mind moving this to something like GUIUtil::handleHideShortcut(QWdiget*)? There are dialogs, like coin control dialog, transaction details, that could use the same feature.

in src/qt/bitcoingui.cpp:414 in 20859aef8b outdated

408@@ -409,6 +409,10 @@ void BitcoinGUI::createActions()
409 
410     connect(new QShortcut(QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_C), this), &QShortcut::activated, this, &BitcoinGUI::showDebugWindowActivateConsole);
411     connect(new QShortcut(QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_D), this), &QShortcut::activated, this, &BitcoinGUI::showDebugWindow);
412+
413+#ifdef Q_OS_MAC
414+    connect(new QShortcut(QKeySequence(Qt::CTRL + Qt::Key_W), this), &QShortcut::activated, this, [this]() { this->hide(); });

promag commented at 2:00 pm on April 8, 2019:

No need to lambda, could just be ... , &QShortcut::activated, this, &QWidget::hide)?

promag commented at 2:02 pm on April 8, 2019: member

Pressing ESC also closes the dialogs (except the main window). Is this something to keep in macos? (not suggesting it though, just want to mention the redundancy)

practicalswift commented at 2:29 pm on April 9, 2019: contributor

FWIW: I’ve verified that a disassembly of the bitcoind binary built on a Ubuntu 18.04 machine with this patch applied is identical to a disassembly of the bitcoind binary built against master (as expected).

laanwj commented at 10:41 am on April 10, 2019: member

utACK, ideally this kind of stuff would be handled by the cross-platform windowing system, so it’s a bit disappointing that it’s not, but also, this is only a few lines and increases user friendliness.

BTW: CTRL-W is also quite a common shortcut for closing windows in applications outside of OS-X, for example in browsers it closes the tab.

IPGlider commented at 10:54 am on April 10, 2019: contributor

I will do the suggested changes and add the functionality to all the windows.

Thanks for the review.

laanwj commented at 11:43 am on April 10, 2019: member

QShortcut has a Qt::ApplicationShortcut for application-global shortcuts (see https://doc.qt.io/Qt-5/qt.html#ShortcutContext-enum )—would this be useful here?

luke-jr commented at 6:45 pm on April 18, 2019: member

Any reason not to enable this on all platforms?

IPGlider force-pushed on Apr 19, 2019

IPGlider commented at 10:26 am on April 19, 2019: contributor

I have updated the PR with the given suggestions and added the shortcut to most windows.

The only one without this shortcut is the ‘About Qt’ as it is part of Qt and I have not been able to add the shortcut to it.

If I have missed any other window please let me know. @laanwj Qt::ApplicationShortcut does not work for the intended purpose. It makes the shortcut works even if it is not focused. @luke-jr CMD+W is usually only used on macOS, more information can be found in the macOS HID posted by @jonasschnelli and in Wikepedia’s Table of keyboard shortcuts.

luke-jr commented at 4:20 pm on April 19, 2019: member

I use Ctrl+W on Linux all the time to close browser tabs. My IRC client seems to use it for closing tabs too.

Furthermore, if macOS is using it, we can’t use it for anything else (eg, closing wallets?).

So overall, I’d say it makes sense to just support it on all platforms.

luke-jr commented at 8:59 pm on April 20, 2019: member

Note: When extending it to support other platforms, care needs to be taken that the main window is not actually hidden if there’s no system tray icon. Probably should minimise in that case? Not sure…

in src/qt/utilitydialog.cpp:164 in 263c3830f6 outdated

159@@ -158,6 +160,8 @@ ShutdownWindow::ShutdownWindow(QWidget *parent, Qt::WindowFlags f):
160         tr("%1 is shutting down...").arg(tr(PACKAGE_NAME)) + "<br /><br />" +
161         tr("Do not shut down the computer until this window disappears.")));
162     setLayout(layout);
163+
164+    GUIUtil::handleHideShortcut(this);

luke-jr commented at 9:03 pm on April 20, 2019:

Do we want this to be hide-able??

IPGlider commented at 7:54 pm on August 5, 2019:

The criteria I followed to make windows hide-able has been what I understand is the common usage in macOS, meaning that every window that has a close button can also be closed using the CMD+W shortcut.

luke-jr commented at 9:03 pm on April 20, 2019: member

Probably a number of windows ought to be closed normally, not merely hidden?

in src/qt/guiutil.cpp:377 in 263c3830f6 outdated

370@@ -370,6 +371,13 @@ void bringToFront(QWidget* w)
371     }
372 }
373 
374+void handleHideShortcut(QWidget* w)
375+{
376+#ifdef Q_OS_MAC
377+    w->connect(new QShortcut(QKeySequence(Qt::CTRL + Qt::Key_W), w), &QShortcut::activated, w, &QWidget::hide);

jonasschnelli commented at 6:55 am on April 29, 2019:

I haven’t looked closely at this,… but is there a proper deallocation of that QShortcut?

IPGlider commented at 7:56 pm on August 5, 2019:

I have followed the official documentation in this regard[0]. If you consider that this is not a correct application please let me know a more appropriate way.

[0] https://doc.qt.io/qt-5/qobject.html

hebasto commented at 9:59 am on August 1, 2019: member

@IPGlider Could you address reviewers’ comments?

fanquake added the label Up for grabs on Aug 2, 2019

fanquake closed this on Aug 2, 2019

IPGlider commented at 6:37 am on August 2, 2019: contributor

Is working in this PR still worth it? I would like to finish it. @hebasto I will address the comments in a few days.

fanquake removed the label Up for grabs on Aug 2, 2019

fanquake reopened this on Aug 2, 2019

hebasto commented at 11:25 am on August 3, 2019: member

@IPGlider

Is working in this PR still worth it?

Yes, IMO.

IPGlider commented at 7:58 pm on August 5, 2019: contributor

@luke-jr

I use Ctrl+W on Linux all the time to close browser tabs. My IRC client seems to use it for closing tabs too.

Furthermore, if macOS is using it, we can’t use it for anything else (eg, closing wallets?).

So overall, I’d say it makes sense to just support it on all platforms.

I wasn’t aware that this shortcut was also commonly used in other OSs other than macOS. Considering this I will remove the ifdef to support all OSs.

Probably a number of windows ought to be closed normally, not merely hidden?

I agree and after reading the QT documentation I will replace the hide call with a close call.

Should I continue working on this PR considering that the name of the branch is add_cmd_w_support_in_macos or would it be preferable to create another PR?

fanquake commented at 1:15 am on August 6, 2019: member

@IPGlider No need for a new PR. Just update the PR title, body etc if required.

IPGlider renamed this:
~~gui: Add CMD+W shortcut in macOS~~
gui: Add close window shortcut
on Aug 6, 2019

IPGlider force-pushed on Aug 6, 2019

IPGlider commented at 5:50 pm on August 6, 2019: contributor

Two jobs failed in Travis CI during the set up:

I have not found a way to force a re-run. Nevertheless the other jobs completed successfully.

hebasto commented at 12:13 pm on August 10, 2019: member

@IPGlider

Two jobs failed in Travis CI…

Restarted…

in src/qt/bitcoingui.cpp:215 in ab589f0db4 outdated

209@@ -210,6 +210,8 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
210 #ifdef Q_OS_MAC
211     m_app_nap_inhibitor = new CAppNapInhibitor;
212 #endif
213+
214+    GUIUtil::handleCloseWindowShortcut(this);

hebasto commented at 1:50 pm on August 10, 2019:

Ctrl+W should close a subwindow within an app but not the main window.

jonasschnelli commented at 10:10 am on October 9, 2019:

I think it should also close the main window (at least on macOS it is the case for almost all apps).

IPGlider commented at 8:18 am on October 13, 2019:

The CTRL+W/CMD+W behaviour in macOS is usually to close the focused window.

in src/qt/splashscreen.cpp:11 in ab589f0db4 outdated

 7@@ -8,6 +8,7 @@
 8 
 9 #include <qt/splashscreen.h>
10 
11+#include <qt/guiutil.h>

hebasto commented at 1:53 pm on August 10, 2019:

Could splash screen be untouched?

IPGlider commented at 8:18 am on October 13, 2019:

The rationale for adding the shortcut here is that on every window with an enabled close button the shortcut should be applicable.

It could be worth it taking into consideration disabling the close button or removing the bar all together.

in src/qt/utilitydialog.cpp:108 in ab589f0db4 outdated

114@@ -114,6 +115,8 @@ HelpMessageDialog::HelpMessageDialog(interfaces::Node& node, QWidget *parent, bo
115         ui->scrollArea->setVisible(false);
116         ui->aboutLogo->setVisible(false);
117     }
118+
119+    GUIUtil::handleCloseWindowShortcut(this);

hebasto commented at 1:57 pm on August 10, 2019:

The “About Qt” dialog does not accept Ctrl+W shortcut. So the “About Bitcoin Core” dialog should do for consistency.

IPGlider commented at 8:18 am on October 13, 2019:

Which of the following cases is preferable?

Two windows with their close behaviour consistent among them but inconsistent with the behaviour of the rest of them or keeping the inconsistency only on the QT window?

in src/qt/utilitydialog.cpp:149 in ab589f0db4 outdated

155@@ -153,6 +156,8 @@ ShutdownWindow::ShutdownWindow(QWidget *parent, Qt::WindowFlags f):
156         tr("%1 is shutting down...").arg(PACKAGE_NAME) + "<br /><br />" +
157         tr("Do not shut down the computer until this window disappears.")));
158     setLayout(layout);
159+
160+    GUIUtil::handleCloseWindowShortcut(this);

hebasto commented at 1:59 pm on August 10, 2019:

Could shutdown window be untouched?

IPGlider commented at 8:19 am on October 13, 2019:

Same comment as for the splash screen.

in src/qt/guiutil.cpp:384 in ab589f0db4 outdated

378@@ -378,6 +379,11 @@ void bringToFront(QWidget* w)
379     }
380 }
381 
382+void handleCloseWindowShortcut(QWidget* w)
383+{
384+    w->connect(new QShortcut(QKeySequence(Qt::CTRL + Qt::Key_W), w), &QShortcut::activated, w, &QWidget::close);

hebasto commented at 2:16 pm on August 10, 2019:

QObject::connect() is a static function of QObject:

0    QObject::connect(new QShortcut(QKeySequence(Qt::CTRL + Qt::Key_W), w), &QShortcut::activated, w, &QWidget::close);

hebasto changes_requested

DrahtBot commented at 7:32 pm on August 23, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17955 (gui: Paste button in Open URI dialog by emilengler)
#17597 (qt: Fix height of QR-less ReceiveRequestDialog by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Aug 29, 2019

laanwj added the label Feature on Sep 30, 2019

GChuf commented at 2:25 pm on October 4, 2019: contributor

@IPGlider are you still working on this?

jonasschnelli commented at 10:12 am on October 9, 2019: contributor

Needs rebase. I think this is almost done. Let’s finish it @IPGlider.

GChuf commented at 11:00 am on October 9, 2019: contributor

IPglider hasn’t been active since April I think - (when) is it okay if I try to pick it up and include hebasto’s suggestions?

IPGlider commented at 3:29 pm on October 9, 2019: contributor

Sorry for the delay, I will rebase and continue working on this PR this weekend.

fanquake added the label Waiting for author on Oct 9, 2019

GChuf commented at 3:52 pm on October 9, 2019: contributor

Excellent, good luck!

IPGlider force-pushed on Oct 13, 2019

DrahtBot removed the label Needs rebase on Oct 13, 2019

DrahtBot added the label Needs rebase on Oct 26, 2019

IPGlider force-pushed on Nov 10, 2019

DrahtBot removed the label Needs rebase on Nov 10, 2019

luke-jr referenced this in commit 186e2003e8 on Nov 15, 2019

MarkLTZ referenced this in commit 2b39e987ad on Nov 17, 2019

fanquake removed the label Waiting for author on Dec 9, 2019

DrahtBot added the label Needs rebase on Dec 9, 2019

IPGlider force-pushed on Dec 22, 2019

DrahtBot removed the label Needs rebase on Dec 22, 2019

luke-jr referenced this in commit c29a4ec823 on Jan 5, 2020

DrahtBot added the label Needs rebase on Jan 8, 2020

IPGlider force-pushed on Jan 24, 2020

DrahtBot removed the label Needs rebase on Jan 24, 2020

DrahtBot added the label Needs rebase on Feb 1, 2020

MarkLTZ referenced this in commit 136a27c84d on Feb 3, 2020

gui: Add close window shortcut

CMD+W/CTRL+W is the standard shortcut to close a window without
exiting the program.

f5a3a5b9ab

IPGlider force-pushed on Feb 5, 2020

DrahtBot removed the label Needs rebase on Feb 5, 2020

hebasto approved

hebasto commented at 5:49 am on May 4, 2020: member

ACK f5a3a5b9ab362c58fa424261f313aa9cf46d2a98, tested on Linux Mint 19.3 by manually opening available dialogs and sub-windows, and applying the Ctrl+W shortcut. Also tested with “Minimize on close” option enabled / disabled.

jonasschnelli commented at 9:52 am on May 4, 2020: contributor

Tested ACK f5a3a5b9ab362c58fa424261f313aa9cf46d2a98

jonasschnelli merged this on May 4, 2020

jonasschnelli closed this on May 4, 2020

sidhujag referenced this in commit e0e6a4ebd0 on May 4, 2020

luke-jr referenced this in commit 7d2ece8401 on Jun 9, 2020

Fabcien referenced this in commit 1e8c9d8760 on Jan 26, 2021

DrahtBot locked this on Aug 16, 2022

gui: Add close window shortcut #15768

Conflicts