Skip to content

Fix quit submenu on macOS tray icon#582

Merged
TheZ3ro merged 1 commit intokeepassxreboot:feature/macos-minimize-onclosefrom
weslly:feature/macos-minimize-onclose
May 19, 2017
Merged

Fix quit submenu on macOS tray icon#582
TheZ3ro merged 1 commit intokeepassxreboot:feature/macos-minimize-onclosefrom
weslly:feature/macos-minimize-onclose

Conversation

@weslly
Copy link
Copy Markdown
Contributor

@weslly weslly commented May 19, 2017

Description

Add the quit submenu to macOS tray icon and disable redundant toggle window action when clicking on the icon.

How has this been tested?

Local tests

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]

@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented May 19, 2017

I don't really like this workaround. I prefer finding out why m_ui->actionQuit doesn't work and fixing that.
Maybe is the Icon (line 188) that cause this problem? Can you test it after removing this line?

If there are no alternative I will accept this

@weslly
Copy link
Copy Markdown
Contributor Author

weslly commented May 19, 2017

@TheZ3ro it still doesn't work if I remove the icon, but when I remove the setMenuRole() at line 189 it works.

According to Qt docs the menu role can only be changed before the action is added to the menu bar so I don't think there's a way to reuse m_ui->actionQuit, even if I set it to QAction::NoRole when adding to the tray icon menu.

@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented May 19, 2017

Why don't you set the MenuRole inside a #ifdef Q_OS_MAC and use m_ui->actionQuit in the tray icon?

I mean, adding the #ifdef before line 188

@weslly
Copy link
Copy Markdown
Contributor Author

weslly commented May 19, 2017

Only macOS uses the MenuRole and this problem only happens on macOS so it would still get the MenuRole anyway.

The workaround only duplicates the quit action on macOS, other OSes still use m_ui->actionQuit at the tray icon menu.

I agree the workaround is ugly but I don't think there's a way to reuse m_ui->actionQuit at OSX :/

@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented May 19, 2017

Sorry, I was meaning something like:

diff --git a/src/gui/MainWindow.cpp b/src/gui/MainWindow.cpp
index 6a41173..43f6cf1 100644
--- a/src/gui/MainWindow.cpp
+++ b/src/gui/MainWindow.cpp
@@ -186,7 +186,9 @@ MainWindow::MainWindow()
     m_ui->actionChangeMasterKey->setIcon(filePath()->icon("actions", "database-change-key", false));
     m_ui->actionLockDatabases->setIcon(filePath()->icon("actions", "document-encrypt", false));
     m_ui->actionQuit->setIcon(filePath()->icon("actions", "application-exit"));
+#ifndef Q_OS_MAC
     m_ui->actionQuit->setMenuRole(QAction::QuitRole);
+#endif
 
     m_ui->actionEntryNew->setIcon(filePath()->icon("actions", "entry-new", false));
     m_ui->actionEntryClone->setIcon(filePath()->icon("actions", "entry-clone", false));

And using m_ui->actionQuit, that should work

@weslly
Copy link
Copy Markdown
Contributor Author

weslly commented May 19, 2017

That would work, but setMenuRole() is only useful (and required) at mac and not having it set breaks the quit menu on other languages:
#482 and #430 (comment)

@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented May 19, 2017

Ok, I was missing those Issues. So I think I can accept this

@TheZ3ro TheZ3ro merged commit 07d4668 into keepassxreboot:feature/macos-minimize-onclose May 19, 2017
@weslly weslly deleted the feature/macos-minimize-onclose branch May 19, 2017 12:32
@phoerious phoerious added pr: bugfix Pull request fixes a bug and removed bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants