Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements a platform-dependent exit hotkey #395

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

data-man
Copy link
Contributor

@data-man data-man commented Jun 5, 2023

No description provided.

@dail8859
Copy link
Owner

dail8859 commented Jun 5, 2023

Hi @data-man thanks for the PR!

I know Ctrl+Q is much more common for Linux (and I'm assuming Mac). I took a look at QKeySequence and was hoping QKeySequence::Quit would handle the cross-platform differences, but for some reason it doesn't provide a default on Windows.

However Microsoft does specify that Alt+F4 should be used for quitting/exiting, so that seems a reasonable default. In the future I would like to have shortcut keys configurable, which would handle any situations where one would want it to be different.

So I think doing something similar to (untested):

#ifdef Q_OS_WIN
    ui->actionExit->setShortcut(QKeySequence("Alt+F4"));
#else
    ui->actionExit->setShortcut(QKeySequence::Quit);
#endif

would be preferrable rather than setting it to Ctrl+Q for all platforms.

@data-man
Copy link
Contributor Author

data-man commented Jun 5, 2023

However Microsoft does specify that Alt+F4 should be used

This is a built-in action in any Windows application, without a programmer having to do anything, if I'm not mistaken. :)
Similar to what is done in KDE Plasma.

Alt + F4 | Close the active item, or exit the active app.

@dail8859
Copy link
Owner

dail8859 commented Jun 5, 2023

This is a built-in action in any Windows application, without a programmer having to do anything

Correct. The only benefit of specifying it manually is so that it shows up in the menu as the assigned shortcut (which may or may not be beneficial to the user). So, to err on the side of simplicity, then yes you are correct and only ui->actionExit->setShortcut(QKeySequence::Quit); would be needed since it won't affect Windows, but the OS would still implement the shortcut.

@data-man
Copy link
Contributor Author

data-man commented Jun 5, 2023

#ifdef Q_OS_WIN
   ui->actionExit->setShortcut(QKeySequence("Alt+F4"));
#else
   ui->actionExit->setShortcut(QKeySequence::Quit);
#endif

Hmm, FocusWriter uses this:

window.cpp
   624:         m_document_cache_thread->quit();
  1311:         m_actions["Quit"] = file_menu->addAction(QIcon::fromTheme("application-exit"), tr("&Quit"), this, &Window::close);
  1312:         m_actions["Quit"]->setShortcut(keyBinding(QKeySequence::Quit, tr("Ctrl+Q")));
  1313:         m_actions["Quit"]->setMenuRole(QAction::QuitRole);

@dail8859
Copy link
Owner

dail8859 commented Jun 5, 2023

m_actions["Quit"]->setShortcut(keyBinding(QKeySequence::Quit, tr("Ctrl+Q")));

keyBinding() seems to allow a fallback shortcut of Ctrl+Q. Given that it is a non-standard Microsoft shortcut, and also Notepad++ uses Ctrl+Q for commenting, I think using Ctrl+Q on Windows would only cause confusion to Windows users.

@data-man
Copy link
Contributor Author

data-man commented Jun 5, 2023

ui->actionExit->setShortcut(QKeySequence::Quit);

Ok, it's work for me in Linux! :)

Should <string>Ctrl+Q</string> be removed from MainWindow.ui?

@dail8859
Copy link
Owner

dail8859 commented Jun 5, 2023

Should Ctrl+Q be removed from MainWindow.ui?

Yeah I think for clarity sake it can be removed to not cause any confusion later since it is being set outside of the ui file.

@data-man data-man changed the title Change Exit hotkey to Ctrl+Q Implements a platform-dependent exit hotkey Jun 5, 2023
@data-man
Copy link
Contributor Author

data-man commented Jun 5, 2023

It's ready, I hope.

@dail8859 dail8859 merged commit fbf6ecc into dail8859:master Jun 5, 2023
11 checks passed
@dail8859
Copy link
Owner

dail8859 commented Jun 5, 2023

Awesome. Thanks!

@data-man data-man deleted the exit_hotkey branch June 5, 2023 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants