Skip to content

Fix problems with the hide/restore functionality on the tray icon (issue #1595).#1700

Closed
joanbm wants to merge 22 commits intokeepassxreboot:release/2.3.2from
joanbm:develop
Closed

Fix problems with the hide/restore functionality on the tray icon (issue #1595).#1700
joanbm wants to merge 22 commits intokeepassxreboot:release/2.3.2from
joanbm:develop

Conversation

@joanbm
Copy link
Copy Markdown
Contributor

@joanbm joanbm commented Mar 10, 2018

Fix/work around tray issues on some Linux systems such as Arch Linux, and Windows. Fixes all cases explained in issue #1595.

Description

Basically there are two separate issues at play there:

  • The Linux issue: Sometimes instead of restoring, the KeePassXC window just flickers when restoring the window from the tray. This mostly happens if both the "minimize to tray" and "minimize at startup" options are enabled, and only on the first restore, though sometimes it's enough to set the "minimize to tray" option is enough and the flicker happens once per every restore, thus the tray having a flicker-restore-hide cycle instead of a restore-hide cycle. I'm not sure why it only happens on Arch but not on Ubuntu, and why it only happens sometimes, but I have found what seems to be the trigger of the issue: 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 sometimes when the main 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: This is a completely separate issue. The flicker on restore issue explained previously doesn't happen on Windows, but on the other hand it seems impossible to hide the window by clicking on the tray icon on Windows. 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).

Motivation and context

This change is required in order to avoid issues when hiding and restoring the KeePassXC windows from tray. One of those issues happen on Windows, and the other in some Linux systems (what exactly causes some systems and desktop environments to be affected and some not to be affected is not known, though it probably depends on the Qt version, but must also depend on some environment condition since on my system the bug only happens sometimes).

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.

Fixes issue #1595 (#1595).

How has this been tested?

I set up some VMs for manual testing of the issue, and I found this:

  • Local Arch Linux + XFCE (Updated): 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. After applying the changes, the bug disappears and no regressions are found.

Environment details:

Libraries:
- Qt 5.10.1
- libgcrypt 1.8.2

Operating system: Arch Linux
CPU architecture: x86_64
Kernel: linux 4.15.7-1-ARCH

Enabled extensions:
- Auto-Type
- Browser Integration
- Legacy Browser Integration (KeePassHTTP)
- SSH Agent
- YubiKey
  • Local Arch Linux + LXDE (Updated): I experience the flicker like in the original report. After applying the changes, the bug disappears and no regressions are found.
  • VM Ubuntu 17.10 + Unity,KDE,LXDE (Updated clean install): No bug experienced. After applying the changes, no regressions are found.
  • VM Arch Linux + LXDE (Updated clean install): I experience the flicker like in the original report. After applying the changes, the bug disappears and no regressions are found.
  • Local Windows 10 (Updated): No flicker on restore, but the window cannot be hidden by clicking on the tray icon. After applying the changes, the bug disappears and no regressions are found.
  • VM Windows 7 (Updated clean install): No flicker on restore, but the window cannot be hidden by clicking on the tray icon. After applying the changes, the bug disappears and no regressions are found.

I also ran all the automatic tests on my local Arch Linux system and they passed.

Screenshots (if appropriate):

Windows issue: https://imgur.com/fJW62v7
Linux issue (flicker on first restore): https://imgur.com/cES8olj

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]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

phoerious and others added 21 commits February 4, 2018 14:10
Correct compile time issues with 2.3.0
* Fix build on NetBSD.
Building with -flto caught the fact that we were ignoring the return
value of readMagicNumbers(), which potentially left the value of 'sig2'
uninitialized.

Signed-off-by: Steven Noonan <steven@uplinklabs.net>
KdbxReader::readDatabase: abort if reading magic numbers fails
* Tests: Speed up AutoType testing

Decrease default autotype delay to 1 to improve test suite speed by
seconds. This shaves multiple seconds off the whole test suite. In some
cases, the largest part.

Also, initialize config just creating the test instance, just in case
that it ever depends on the configuration values at that point already.

* Tests: Speed up Kdbx4 testing

This speeds up the Kdbx4 tests by using parameters optimized for speed
for the key derivation functions. On an i7-6700K the tests run close to
50% faster with this change (about 1.5s vs. 3s).
@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented Mar 10, 2018

This seems ok, can someone test on MacOS? @weslly maybe

@weslly weslly self-requested a review March 10, 2018 21:21
@droidmonkey droidmonkey added this to the v2.3.2 milestone Mar 11, 2018
@droidmonkey
Copy link
Copy Markdown
Member

Please rebase onto release/2.3.2 branch

src/main.cpp Outdated
bool minimizeOnStartup = config()->get("GUI/MinimizeOnStartup").toBool();
bool minimizeToTray = config()->get("GUI/MinimizeToTray").toBool();
if (minimizeOnStartup) {
if (minimizeOnStartup && !minimizeToTray) {
Copy link
Copy Markdown
Contributor

@weslly weslly Mar 11, 2018

Choose a reason for hiding this comment

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

If this particular issue only affects Linux, the extra condition shouldn't be enforced on other OSes.

Also, the "minimize on startup" checkbox should be disabled when "minimize to tray" is active at the settings window on Linux.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why? I want KeePassXC to start minimized in the tray.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@weslly It shouldn't hurt if the condition is enforced in other platforms, though I will add an #ifdef for it if it makes the reason clearer. Also a comment referencing it, now that I think of it.

I concur with @phoerious, "minimize on startup" + "minimize to tray" should be a valid combination, it's specially useful if you have set KeePassXC to auto unlock on boot (and want it to be on the tray upon boot).

Copy link
Copy Markdown
Contributor

@weslly weslly Mar 11, 2018

Choose a reason for hiding this comment

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

@phoerious Whoops, I just noticed this extra condition doesn't matter since the window is only shown on the next if

@joanbm
Copy link
Copy Markdown
Contributor Author

joanbm commented Mar 11, 2018

@droidmonkey You're right, I missed the git flow branch structure and did the PR over develop, will fix.

@joanbm joanbm changed the base branch from develop to release/2.3.2 March 11, 2018 11:37
@droidmonkey
Copy link
Copy Markdown
Member

You need to put your changes into their own branch. It would be easiest for you to make a new branch off 2.3.2 and cherry pick your commits into that new branch. Push it, then fix which branches this PR is pointing to.

@joanbm
Copy link
Copy Markdown
Contributor Author

joanbm commented Mar 11, 2018

@droidmonkey Unfortunately I can't change the source branch of the PR, only the destination branch... not sure if there is any solution. Sorry for the annoyance, I will make sure to do it right the next time.

image

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.

8 participants