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

Change behavior when tray icon is clicked #19940

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thalieht
Copy link
Contributor

qBittorrent will be hidden only when it is in focus.

Closes #11424

@thalieht thalieht added the GUI GUI-related issues/changes label Nov 13, 2023
@glassez glassez requested a review from a team November 14, 2023 05:56
@glassez glassez added this to the 5.0 milestone Nov 14, 2023
@glassez
Copy link
Member

glassez commented Nov 14, 2023

The question is shouldn't we change behavior only for icon click and preserve the current one for context menu?

qBittorrent will be hidden only when it is in focus.
@thalieht
Copy link
Contributor Author

The question is shouldn't we change behavior only for icon click and preserve the current one for context menu?

Done.

Comment on lines +402 to +418
if (isActiveWindow())
{
hide();
}
else
{
if (m_uiLocked && !unlockUI()) // Ask for UI lock password
return;

// Make sure the window is not minimized
setWindowState((windowState() & ~Qt::WindowMinimized) | Qt::WindowActive);

// Then show it
show();
raise();
activateWindow();
}
Copy link
Member

Choose a reason for hiding this comment

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

I believe you shouldn't do setWindowState((windowState() & ~Qt::WindowMinimized) | Qt::WindowActive); manually. The next 3 methods will care about it. Or did your tests show the another result?
And if I'm right, then it can even be rewritten as the following:

Suggested change
if (isActiveWindow())
{
hide();
}
else
{
if (m_uiLocked && !unlockUI()) // Ask for UI lock password
return;
// Make sure the window is not minimized
setWindowState((windowState() & ~Qt::WindowMinimized) | Qt::WindowActive);
// Then show it
show();
raise();
activateWindow();
}
if (isActiveWindow())
hide();
else
activate();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't notice any problems at all.

I believe you shouldn't do setWindowState((windowState() & ~Qt::WindowMinimized) | Qt::WindowActive); manually.

Removing only that line or with your suggestion, when qBt is minimized (not to tray) clicking the tray icon doesn't restore the window.

Copy link
Member

Choose a reason for hiding this comment

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

Removing only that line or with your suggestion, when qBt is minimized (not to tray) clicking the tray icon doesn't restore the window.

OK.
Seems setWindowState(windowState() & ~Qt::WindowMinimized) is really required but setting Qt::WindowActive flag does exactly the same as activateWindow() so one of them can be omitted.

Unfortunately, this code does not work correctly (at least on Windows, where I managed to test it). It does not hide the main window when clicking on the tray icon. For some reason, isActiveWindow() always returns false.

Copy link
Contributor Author

@thalieht thalieht Nov 14, 2023

Choose a reason for hiding this comment

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

but setting Qt::WindowActive flag does exactly the same as activateWindow() so one of them can be omitted.

Ok i see no problems with removing Qt::WindowActive but i do with removing activateWindow():

  1. qBt out of focus
  2. Click tray icon and it brings qBt to the front
  3. Subsequent tray icon clicks do nothing (qBt is not in focus)

Unfortunately, this code does not work correctly (at least on Windows, where I managed to test it). It does not hide the main window when clicking on the tray icon. For some reason, isActiveWindow() always returns false.

Damn... i can't test in Windows, i mean i could with CI builds but it would be a chore.

Copy link
Member

@glassez glassez Nov 14, 2023

Choose a reason for hiding this comment

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

Ok i see no problems with removing Qt::WindowActive but i do with removing activateWindow()

In fact, the order of operations matters. show() has to go before activateWindow(), otherwise it doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

For some reason, isActiveWindow() always returns false.

It looks like main window becomes inactive at the moment when the user interacts with the tray icon.

@glassez
Copy link
Member

glassez commented Nov 22, 2023

In fact, I was always annoyed by the behavior of the qBittorrent tray icon, but it was a small thing for me, so I never even tried to delve into what exactly it does.
Now I tried to evaluate this problem.
I was always confused by the fact that when I clicked on the qBittorrent tray icon in the hope of seeing its window in front of my eyes, I eventually noticed that its window was previously open, but it was behind other windows or minimized to the taskbar, and at the moment of my click it simply hided into the tray, so I had to click again to see it.
Now I have checked the behavior when clicking on the icons of other applications that are constantly staying in my tray. And I found only three options:

  1. no reaction, the window can only be opened from the context menu, (inconvenient),
  2. duplicate the right click, i.e. the context menu is shown (unusual, but usable, its advantage is that you can avoid showing the window by accident),
  3. shows the application window or brings a previously opened window to the foreground.

Note that none of the icons (except qBittorrent) hides the opened window when clicked. This can only be done either through the context menu of the icon, or by closing the window itself in the usual way.

@thalieht
Copy link
Contributor Author

  1. shows the application window or brings a previously opened window to the foreground.

Isn't that what we want?
Anyway i'll say it again that i can't do anything about Windows (should i close the PR?).

Note that none of the icons (except qBittorrent) hides the opened window when clicked.

Mumble behaves like qbittorrent exactly (except no "Hide" from context menu), i have no opinion for this behavior.

@glassez
Copy link
Member

glassez commented Nov 23, 2023

5. shows the application window or brings a previously opened window to the foreground.

Isn't that what we want?

Yes.
But it no more hides window on Windows.
I am quite happy with this behavior if it was also proposed as a feature and would be approved by other members (and, accordingly, would work the same on all platforms). But now it is a regression. So I doubt it can be accepted in this form.
@Chocobo1, @qbittorrent/bug-handlers, what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI GUI-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Focus qBittorrent on tray icon click instead of minimizing it
2 participants