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

KeePassXC immediately minimizes to tray when restoring from tray icon #1595

Closed
Generator opened this issue Mar 2, 2018 · 12 comments · Fixed by #2402
Closed

KeePassXC immediately minimizes to tray when restoring from tray icon #1595

Generator opened this issue Mar 2, 2018 · 12 comments · Fixed by #2402

Comments

@Generator
Copy link

When starting KeePassXC with "Minimize window at application startup" and "Hide window system tray when minimizes" immediately minimizes to tray when restoring from tray icon for the first time.
This happens with XFCE with or without composite manager (xfwm4 or compton), didn't try with other desktop environment.

Expected Behavior

Main window should minimized when restored from system tray

Current Behavior

Window always minimizes immediately when restored for the first time
Issue video demo

Steps to Reproduce (for bugs)

  1. Go to Tools > Settings
  2. Enable "Minimize window at application startup" and "Hide window system tray when minimizes"
  3. Restart KeePassXC

Debug Info

KeePassXC - Version 2.3.0
Revision: 4c0ed74

Libraries:

  • Qt 5.10.1
  • libgcrypt 1.8.2

Operating system: Arch Linux
CPU architecture: x86_64
Kernel: linux 4.14.22-1-lts

Enabled extensions:

  • Auto-Type
  • Browser Integration
  • Legacy Browser Integration (KeePassHTTP)
  • SSH Agent
  • YubiKey

XFCE4 - 4.12

@joanbm
Copy link
Contributor

joanbm commented Mar 4, 2018

I’m observing the same behavior, also on Arch + XFCE. Basically, if I click repeatedly on the tray, I run into a flicker - restore - minimize cycle instead of a restore - minimize cycle.

I tried looking in the code, and I couldn’t figure out much, IIRC I saw that shortly after the toggleWindow call that restores the window, there’s a single changeEvent call after it where isMinimized() is still true (the calls following this return false) and this causes the code to send the window again to the tray (this is what causes the flicker). If I click the tray icon again, the isMinimized() call in changeEvent returns false correctly and the restore works.

Maybe there’s a chance this is something about Qt? I saw some similar bugs in their tracker from 2014 but they closed them without much explanation. Would be nice if we had more people try out on their desktop environment to narrow the issue.

@phoerious phoerious added the bug label Mar 4, 2018
@joanbm
Copy link
Contributor

joanbm commented Mar 5, 2018

I tested it today on Windows, and clicking on the tray icon restores the window (without the flicker effect), but clicking on it again doesn't seem to hide it. So it seems it's only useful for restoring the window on Windows... weird

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Mar 5, 2018

I'm using XFCE 4.12 as well but I can't reproduce this (clicking on the icon opens a menu with Toggle window, that works fine)

@Generator
Copy link
Author

The issue only happens if bot options are enabled ("Minimize window at application startup" and "Hide window system tray when minimizes") it's like the application tries to minimize after restoring the first time from tray.
If "Hide window system tray when minimizes" isn't enabled the same doesn't happen.

@phoerious
Copy link
Member

I can reproduce it on KDE. It's something I've been noticing for a while, but haven't had the time to fix.

@joanbm
Copy link
Contributor

joanbm commented Mar 7, 2018

As I said, I observe a spurious changeEvent() call with isMinimized()=true when restoring the window. This is followed later by another changeEvent() call with isMinimized()=false. I'm not sure what causes it, but this code seems to work around it:

diff --git a/src/gui/MainWindow.cpp b/src/gui/MainWindow.cpp
index 1822c48d..894cde7f 100644
--- a/src/gui/MainWindow.cpp
+++ b/src/gui/MainWindow.cpp
@@ -821,6 +821,8 @@ void MainWindow::closeEvent(QCloseEvent* event)
     }
 }
 
+static QTimer *hideSingleShot = NULL;
+
 void MainWindow::changeEvent(QEvent* event)
 {
     if ((event->type() == QEvent::WindowStateChange) && isMinimized()) {
@@ -828,12 +830,22 @@ void MainWindow::changeEvent(QEvent* event)
                 && config()->get("GUI/MinimizeToTray").toBool())
         {
             event->ignore();
-            QTimer::singleShot(0, this, SLOT(hide()));
+
+            delete hideSingleShot;
+
+            hideSingleShot = new QTimer(this);
+            hideSingleShot->setSingleShot(true);
+            connect(hideSingleShot, SIGNAL(timeout()), this, SLOT(hide()));
+            hideSingleShot->start(0);
         }
 
         if (config()->get("security/lockdatabaseminimize").toBool()) {
             m_ui->tabWidget->lockDatabases();
         }
+    } else if ((event->type() == QEvent::WindowStateChange) && !isMinimized()) {
+        delete hideSingleShot;
+        hideSingleShot = NULL;
+
     }
     else {
         QMainWindow::changeEvent(event);

Code quality stuff aside, what this does is basically cancel the single shot timer event set by a changeEvent with isMinimized()=true if it is immediately followed by a changeEvent with isMinimized()=false.

I'm not sure if this kind of dirty workaround is the best we can aim for, or if there is a more clean way of solving this...

By the way, it's worth remarking again that for me at least, the bug happens even without the "Minimize at startup" option, and that I can reproduce over and over by repeatedly clicking the tray icon in order to minimize/restore the window repeatedly, while the original bug seems to point to other circumstances.

EDIT: Just installed LXDE in my system, and there I can reproduce the behavior pointed out in the original bug report... it only happens when both "Minimize at startup" and "Minimize to tray" are checked, and only when restoring the window for the first time. It doesn't happen any longer.

In this case, I detected two relevant behavior differences:

  • The patch above doesn't work, though the root cause it the same (spurious changeEvent calls with isMinimized()=true when restoring the window). However, changing the QTimer delay from 0 to e.g. 250 makes it work again.
  • In this case, it can also be solved by always calling mainWindow.show() at startup (note that this is not normally done under both "Minimize at startup" and "Minimize to tray"), but this causes another flicker to happen at startup, so it's no real solution...

@joanbm
Copy link
Contributor

joanbm commented Mar 8, 2018

qttest.zip
A simple demo app replicating the problem (with console output)

@joanbm
Copy link
Contributor

joanbm commented Mar 9, 2018

Got some more results, and what may be a pretty clean solution. Excuse my long write-ups :)

I set up some VMs, and I found this:

  • Local Arch Linux + XFCE: I experience the flicker-restore-minimize cycle sometimes, and sometimes only the flicker on the first time like in the original report. No idea on what makes the difference.
  • Local Arch Linux + LXDE: I experience the flicker like in the original report.
  • VM Ubuntu 17.10 + Unity,KDE,LXDE (Clean install): No bug experienced
  • VM Arch Linux + LXDE (Clean install): I experience the flicker like in the original report.
  • Local Windows: No flicker on restore, but the window cannot be hidden by clicking on the tray icon
  • VM Windows (Clean install): No flicker on restore, but the window cannot be hidden by clicking on the tray icon

Basically there are two separate issues at play there:

  • The Linux issue: I'm not sure why it only happens on Arch but not on Ubuntu, but I have found what seems to be the trigger of it: If one both minimizes and hides the window by code (or minimizes the window without showing it at all), it seems that a spurious changeEvent with isMinimized()=true is received when the window is restored, which causes the bug. I imagine that, internally on the Qt Linux implementation, hiding the window causes further events to be enqueued or something similar, and when restoring the window some events from when the window was minimized are notified.

This can happen both on start (this is the case of the original bug report), if minimizeOnStartup=true and minimizeToTray=true, then the window is minimized but not shown:

    // start minimized if configured
    bool minimizeOnStartup = config()->get("GUI/MinimizeOnStartup").toBool();
    bool minimizeToTray    = config()->get("GUI/MinimizeToTray").toBool();
    if (minimizeOnStartup) {
        mainWindow.setWindowState(Qt::WindowMinimized); // --> MINIMIZE
    }
    if (!(minimizeOnStartup && minimizeToTray)) {
        mainWindow.show(); // --> NO SHOW
}

Or when clicking on the tray icon to hide the window, the window is both minimized and hidden by code on hideWindow() (this is what is causing my flicker-restore-minimize cycle sometimes):

void MainWindow::hideWindow()
{
    saveWindowInformation();
#ifndef Q_OS_MAC
    setWindowState(windowState() | Qt::WindowMinimized); // --> MINIMIZE
#endif
    QTimer::singleShot(0, this, SLOT(hide())); // --> HIDE

    if (config()->get("security/lockdatabaseminimize").toBool()) {
        m_ui->tabWidget->lockDatabases();
    }
}
  • The Windows issue: On toggleWindow(), there's a QApplication::activeWindow() == this check:
void MainWindow::toggleWindow()
{
    if ((QApplication::activeWindow() == this) && isVisible() && !isMinimized()) {
        hideWindow();
    } else {

On Windows, when clicking on the tray icon when the window is visible, QApplication::activeWindow() = NULL. I think this is due to differences in window focus behavior between Windows and Linux (I believe Windows removes the focus from the window when clicking on the tray).

Tentative fix/workaround for all those issues:

diff --git a/src/gui/MainWindow.cpp b/src/gui/MainWindow.cpp
index 1822c48d..5486fc2d 100644
--- a/src/gui/MainWindow.cpp
+++ b/src/gui/MainWindow.cpp
@@ -971,9 +971,6 @@ void MainWindow::trayIconTriggered(QSystemTrayIcon::ActivationReason reason)
 void MainWindow::hideWindow()
 {
     saveWindowInformation();
-#ifndef Q_OS_MAC
-    setWindowState(windowState() | Qt::WindowMinimized);
-#endif
     QTimer::singleShot(0, this, SLOT(hide()));
 
     if (config()->get("security/lockdatabaseminimize").toBool()) {
@@ -983,7 +980,7 @@ void MainWindow::hideWindow()
 
 void MainWindow::toggleWindow()
 {
-    if ((QApplication::activeWindow() == this) && isVisible() && !isMinimized()) {
+    if (isVisible() && !isMinimized()) {
         hideWindow();
     } else {
         bringToFront();
diff --git a/src/main.cpp b/src/main.cpp
index a7fd2d76..12397339 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -125,7 +125,7 @@ int main(int argc, char** argv)
     // start minimized if configured
     bool minimizeOnStartup = config()->get("GUI/MinimizeOnStartup").toBool();
     bool minimizeToTray    = config()->get("GUI/MinimizeToTray").toBool();
-    if (minimizeOnStartup) {
+    if (minimizeOnStartup && !minimizeToTray) {
         mainWindow.setWindowState(Qt::WindowMinimized);
     }
     if (!(minimizeOnStartup && minimizeToTray)) {

For the Linux issue, I avoid both minimizing and hiding the window at the same time by code. It should suffice to only hide the window if the event is caused by a tray icon click.

For the Windows issue, I just removed the check for QApplication::activeWindow() == this, I think it is unreliable and the other checks in the same line (which were added later) should suffice alone.

I have done some tests on my local system and VMs and didn't experience any regression and all the three buggy cases disappeared... can someone review this patch and/or test it on his local system?

@phoerious
Copy link
Member

Thanks a lot for your thorough investigation. I think the minimize call on macOS is a relic of times when we had no tray icon on macOS.
Could you submit your patch as a pull request, please? That makes it easier for us to test and review your solution.

@joanbm
Copy link
Contributor

joanbm commented Mar 10, 2018

I have sent a PR, hopefully I didn't mess up in any of the PR process steps 😛.

TheZ3ro pushed a commit that referenced this issue Mar 12, 2018
Fix/work around KeePassXC flickering and not restoring from tray on some Linux systems, which happens if the window is hidden and minimized by code at the same time (see issue #1595).
TheZ3ro pushed a commit that referenced this issue Mar 12, 2018
Fix unreliable check on toggleWindow() which causes Windows systems to be unable to hide the window by clicking on the tray icon (see issue #1595).
@irgendsontyp
Copy link

Does not seem to be fixed, see #2088

jtl999 pushed a commit to jtl999/keepassxc that referenced this issue Jul 29, 2018
…oot#1595).

Fix/work around KeePassXC flickering and not restoring from tray on some Linux systems, which happens if the window is hidden and minimized by code at the same time (see issue keepassxreboot#1595).
jtl999 pushed a commit to jtl999/keepassxc that referenced this issue Jul 29, 2018
…xreboot#1595)

Fix unreliable check on toggleWindow() which causes Windows systems to be unable to hide the window by clicking on the tray icon (see issue keepassxreboot#1595).
jtl999 pushed a commit to jtl999/keepassxc that referenced this issue Jul 29, 2018
jtl999 pushed a commit to jtl999/keepassxc that referenced this issue Jul 29, 2018
jtl999 pushed a commit to jtl999/keepassxc that referenced this issue Jul 29, 2018
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 a pull request may close this issue.

6 participants
@Generator @joanbm @TheZ3ro @phoerious @irgendsontyp and others