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

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

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@vajdaz

vajdaz commented Dec 10, 2017

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.

@fanquake fanquake added the GUI label Dec 10, 2017

@promag

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

{
QTimer::singleShot(0, this, SLOT(showMaximized()));
}
else if (wsevt->oldState() & Qt::WindowFullScreen)

This comment has been minimized.

@promag

promag Dec 10, 2017

Member

Remove fullscreen?

This comment has been minimized.

@vajdaz
QTimer::singleShot(0, this, SLOT(hide()));
e->ignore();
}
}
}
#endif
QMainWindow::changeEvent(e);

This comment has been minimized.

@promag

This comment has been minimized.

@vajdaz

vajdaz Dec 10, 2017

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

This comment has been minimized.

@promag

promag Dec 10, 2017

Member

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

This comment has been minimized.

@vajdaz

vajdaz Dec 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.

Adjustments on pull request #11859
* Removing unnecessary handling of full screen state handling
* Restoring original order of calling parent's changeEvent before own implementation
@vajdaz

This comment has been minimized.

vajdaz commented Dec 10, 2017

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

This comment has been minimized.

Member

laanwj commented Dec 11, 2017

I've restarted the travis build.

@promag

This comment has been minimized.

Member

promag commented Dec 11, 2017

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

@laanwj

This comment has been minimized.

Member

laanwj commented Dec 19, 2017

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

This comment has been minimized.

Member

promag commented Dec 19, 2017

it'd be better to remove it.

Only linux, or all?

@vajdaz

This comment has been minimized.

vajdaz commented Dec 20, 2017

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

This comment has been minimized.

Member

laanwj commented Dec 21, 2017

On Mac I don't know.

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

#ifdef Q_OS_MAC
    /* remove Window tab on Mac */
    ui->tabWidget->removeTab(ui->tabWidget->indexOf(ui->tabWindow));
#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

This comment has been minimized.

Member

jonasschnelli commented Dec 22, 2017

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

This comment has been minimized.

vajdaz commented Dec 24, 2017

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

This comment has been minimized.

Member

laanwj commented Dec 27, 2017

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Apr 10, 2018

Member

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

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Apr 10, 2018

Needs squash and linux testing.

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

@laanwj

This comment has been minimized.

Member

laanwj commented May 14, 2018

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment