Qt: Fixing restore from system tray behaviour of main window

vajdaz commented at 0:31 am on December 10, 2017: none

When restoring the main window from the system tray, it always was in minimized state (at least on Windows). This patch should fix this. The window should restore to the same state as it was at the moment of minimizing it into the tray.

May be related to issue #8225

To reproduce the bug do following under Windows:

In Options->Window enable “Minimize to the tray instead of the taskbar”
Optionally enable “Minimize on close”, too
Minimize the main window
Klick on the tray icon (or right click on the tray icon and select Show/Hide)

The main window’s taskbar icon appears, but the window itself is not restored to its original size.

Expected behaviour: the window should appear.

Fixing restore behaviour of main window from system tray

When restoring the main window from the system tray, it always was in minimized state (at least on Windows). This patch should fix this. The window should restore to the same state as it was  at the moment of minimizing it into the tray.

eb217dfc0e

fanquake added the label GUI on Dec 10, 2017

in src/qt/bitcoingui.cpp:952 in eb217dfc0e outdated

944@@ -946,12 +945,25 @@ void BitcoinGUI::changeEvent(QEvent *e)
945             QWindowStateChangeEvent *wsevt = static_cast<QWindowStateChangeEvent*>(e);
946             if(!(wsevt->oldState() & Qt::WindowMinimized) && isMinimized())
947             {
948+                if (wsevt->oldState() & Qt::WindowMaximized)
949+                {
950+                    QTimer::singleShot(0, this, SLOT(showMaximized()));
951+                }
952+                else if (wsevt->oldState() & Qt::WindowFullScreen)

promag commented at 1:46 am on December 10, 2017:

Remove fullscreen?

vajdaz commented at 10:58 am on December 10, 2017:

Removed.

in src/qt/bitcoingui.cpp:966 in eb217dfc0e outdated

961                 e->ignore();
962             }
963         }
964     }
965 #endif
966+    QMainWindow::changeEvent(e);

promag commented at 1:51 am on December 10, 2017:

Why?

vajdaz commented at 10:59 am on December 10, 2017:

I thought, it makes senses to reorder. But I see, it makes no sense. Fixed.

promag commented at 11:07 am on December 10, 2017:

You could have some reason, otherwise it’s unrelated to your goal.

vajdaz commented at 6:07 pm on December 11, 2017:

The idea was, that theoretically we don’t know what QMainWindow::changeEvent does. So if I want to add something “on top” of the default behaviour, I have to do it after that and not before. But this is kind of a academical thought. To be honest, I am not a Qt expert.

promag commented at 1:52 am on December 10, 2017: member

This must be tested in linux too. BTW see #941.

Adjustments on pull request #11859

* Removing unnecessary handling of full screen state handling
* Restoring original order of calling parent's changeEvent before own implementation

7856bb412d

vajdaz commented at 11:06 am on December 10, 2017: none

Besides Windows 10, I also have testetd on Kubuntu 16.04 (Plasma) and Ubuntu 16.04 (Gnome) . Seems to be fine to me. However, on Gnome there is no system tray. So if you minimize to tray, you loose visual reference to your application. But this is nothing that this patch introduces, it is a conceptual problem that we have already.

Failed CI test seems not to be related to my changes. Sporadic errors maybe? Could you please run the tests again?

laanwj commented at 2:35 pm on December 11, 2017: member

I’ve restarted the travis build.

promag commented at 2:37 pm on December 11, 2017: member

@laanwj me too, about an hour ago. Was it still failing?

laanwj commented at 4:28 pm on December 19, 2017: member

Was it still failing?

Yes.

This must be tested in linux too. BTW see #941.

Yes we’ve had so much problems with this feature in the past… If it turns out it’s just not possible to do this reliably with Qt it’d be better to remove it.

promag commented at 4:35 pm on December 19, 2017: member

it’d be better to remove it.

Only linux, or all?

vajdaz commented at 10:40 pm on December 20, 2017: none

Meanwhile I also read a about some past tray icon issues with different OSes. Seems that Qt can’t do it right everywhere.

Anyway, I have the impression, that minimizing to tray is a common thing to do on Windows. However, on Linux this is not the case (see GNOME). On Mac I don’t know.

What about keeping the minimize to tray feature only on Windows?

laanwj commented at 11:09 am on December 21, 2017: member

On Mac I don’t know.

It’s already disabled on MacOSX, along with the other window options:

0#ifdef Q_OS_MAC
1    /* remove Window tab on Mac */
2    ui->tabWidget->removeTab(ui->tabWidget->indexOf(ui->tabWindow));
3#endif

What about keeping the minimize to tray feature only on Windows?

Sounds good to me - but I’m not sure whether any Linux users use it, I don’t at least… Maybe we could hide it for a release first, see if anyone even notices?

jonasschnelli commented at 8:15 pm on December 22, 2017: contributor

Can reproduce the issue on Windows 8.1. Tested this PR and it works,… however there is a tiny visual glich when minimising to tray (it show the main window for a couple of ms maximised after minimising), which is not really problematic.

OSX does not do minimise to tray… needs Linux testing.

vajdaz commented at 2:35 pm on December 24, 2017: none

I have made an additional change that hides the tray icon for non-Windows systems, disables the “Hide tray icon” and “Minimize to tray” options on the Settings’ Winwow tab and sets the according settings in the OptionsModel (removing them from the configuration file).

On Windows, evertything works as before. On other systems, no tray icon is visible, and no tray icon settings are available. The “Minimize on close” option still exists on all OSes.

Should I create a new PR, or should I just add those changes to this one?

laanwj commented at 12:58 pm on December 27, 2017: member

Probably best to do so as a new PR, as the change is not directly pertinent to this one.

in src/qt/bitcoingui.cpp:951 in 7856bb412d

945@@ -946,6 +946,14 @@ void BitcoinGUI::changeEvent(QEvent *e)
946             QWindowStateChangeEvent *wsevt = static_cast<QWindowStateChangeEvent*>(e);
947             if(!(wsevt->oldState() & Qt::WindowMinimized) && isMinimized())
948             {
949+                if (wsevt->oldState() & Qt::WindowMaximized)
950+                {
951+                    QTimer::singleShot(0, this, SLOT(showMaximized()));

jonasschnelli commented at 7:21 pm on April 10, 2018:

Have you tried qApp->processEvents(); instead of the singleshot()?

jonasschnelli commented at 7:22 pm on April 10, 2018: contributor

Needs squash and linux testing.

laanwj added the label Up for grabs on May 14, 2018

laanwj commented at 12:32 pm on May 14, 2018: member

Last reply from author is some time last year, closing and adding “Up for grabs” (let me know if you start work againand I should reopen).

laanwj closed this on May 14, 2018

MarcoFalke removed the label Up for grabs on Sep 27, 2018

MarcoFalke locked this on Sep 8, 2021

Qt: Fixing restore from system tray behaviour of main window #11859