Skip to content

Conversation

@promag
Copy link
Contributor

@promag promag commented Jul 31, 2019

Originally suggested by @emilengler.

@fanquake fanquake added the GUI label Jul 31, 2019
@cvengler
Copy link
Contributor

cvengler commented Jul 31, 2019

Tested it, works great.
Concept ACK
Approach ACK

@hebasto
Copy link
Member

hebasto commented Aug 1, 2019

Concept ACK and the code looks neat.
The CTRL+D shortcut is not common for Windows users. And the GUI should somehow inform a user about it.

@jonasschnelli
Copy link
Contributor

I guess this is OS specific.
On macOS, cmd-W does close windows within an app (works almost with all apps including strange java cross platform apps like "Arduino-IDE",... but not with Bitcoin-Qt).

However, concept ACK on working on shortcuts to close windows.

@laanwj
Copy link
Member

laanwj commented Aug 1, 2019

Concept ACK, going to test

(though I share with @jonasschnelli a slight preference for Ctrl-W as well for closing any subwindow, due to FF browser tabs, but have no problem with Ctrl-D doing the same here, there's already a PR for that anyhow #15768)

@laanwj
Copy link
Member

laanwj commented Aug 1, 2019

Tested ACK 5813b37

@cvengler
Copy link
Contributor

cvengler commented Aug 1, 2019

@jonasschnelli does ctrl+w closes any window?
The idea behind it was only to close a terminal window and the bitcoin-qt terminal window is one. I think the Ctrl+D shortcut is common for unix terminals

@promag
Copy link
Contributor Author

promag commented Aug 1, 2019

Correction, it sends EOF to the shell prompt, which then terminates, which can result in the window close.

@cvengler
Copy link
Contributor

cvengler commented Aug 1, 2019

Sure but this effectively closes a terminal (at least on unix(-like) systems)

@promag
Copy link
Contributor Author

promag commented Aug 1, 2019

Not quite:

cat
CTRL+D    # cat quits

@cvengler
Copy link
Contributor

cvengler commented Aug 1, 2019

IIRC someone said that this does not send a EOF to the terminal, it "only" closes the terminal.
I meant that your implementation is fine for this.
Sorry I sometimes have problems to justify myself :(

@jonasschnelli
Copy link
Contributor

@jonasschnelli does ctrl+w closes any window?

on macOS: yes. (Almost) every window (at least all well behaving apps close a window).


#15768 is related and I think we should have one PR adding the "close window shortcut functionality".

Since this is OS dependent, the shortcut should probably be defined in platformstyles.cpp/.h.

However, I think each window should be closeable with a default shortcut (including console, help, preferences, etc.).
Probably: ctrl-D on linux, cmd-W on macOS and alt F4 on windows.

I could not find, but it could be that Qt provide a such functionality already.

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

ACK 5813b37, built and tested manually, all tests pass.

The added C-d functionality works only with the GUI console window, and not with the info, network traffic, or peers windows.

On Linux Debian for me C-w closes desktop windows (Gnome) and browser tabs (Firefox), and C-d closes terminal windows.

(though I share with @jonasschnelli a slight preference for Ctrl-W as well for closing any subwindow, due to FF browser tabs, but have no problem with Ctrl-D doing the same here, there's already a PR for that anyhow #15768)

Agree.

@cvengler
Copy link
Contributor

cvengler commented Aug 2, 2019

On my Debian Linux MATE Ctrl-W also closes nearly every window except console windows.
Does Ctrl-W also closes the default Terminal.app on macOS?

@hebasto
Copy link
Member

hebasto commented Aug 3, 2019

I'd prefer to use Ctrl+W for closing any subwindow (like #15768). And using a special shortcut for closing the "Console" window seems an overkill:

Define new keyboard shortcuts only for things people do regularly. It’s hard for people to remember shortcuts they seldom use. Minimizing app-specific keyboard shortcuts also helps avoid potential conflicts with other system-wide shortcuts that may be in place.

Anyway, if reviewers will decide that the "Console" window needs its own shortcut to be closed, it seems the next patch could be an improvement of usability:

--- a/src/qt/rpcconsole.cpp
+++ b/src/qt/rpcconsole.cpp
@@ -802,19 +802,21 @@ void RPCConsole::clear(bool clearHistory)
                 ".secwarning { color: red; }"
                 "b { color: #006060; } "
             ).arg(fixedFontInfo.family(), QString("%1pt").arg(consoleFontSize))
         );
 
 #ifdef Q_OS_MAC
-    QString clsKey = "(⌘)-L";
+    QString clear_key = "⌘L";
+    QString close_key = "⌘D";
 #else
-    QString clsKey = "Ctrl-L";
+    QString clear_key = "Ctrl+L";
+    QString close_key = "Ctrl+D";
 #endif
 
     message(CMD_REPLY, (tr("Welcome to the %1 RPC console.").arg(PACKAGE_NAME) + "<br>" +
-                        tr("Use up and down arrows to navigate history, and %1 to clear screen.").arg("<b>"+clsKey+"</b>") + "<br>" +
+                        tr("Use up and down arrows to navigate history, %1 to clear screen and %2 to close this window.").arg("<b>" + clear_key + "</b>").arg("<b>" + close_key + "</b>") + "<br>" +
                         tr("Type %1 for an overview of available commands.").arg("<b>help</b>") + "<br>" +
                         tr("For more information on using this console type %1.").arg("<b>help-console</b>") +
                         "<br><span class=\"secwarning\"><br>" +
                         tr("WARNING: Scammers have been active, telling users to type commands here, stealing their wallet contents. Do not use this console without fully understanding the ramifications of a command.") +
                         "</span>"),
                         true);

@promag
Copy link
Contributor Author

promag commented Aug 19, 2019

meh

@promag promag closed this Aug 19, 2019
@promag promag deleted the 2019-07-console-ctrld branch August 19, 2019 00:28
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants