-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Add tray icon when quake window is minimized #10179
Conversation
This comment has been minimized.
This comment has been minimized.
AMEN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only got questions really, but this looks good
Shell_NotifyIcon(NIM_DELETE, &_trayIconData.value()); | ||
_trayIconData.reset(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussion: should we
else if (!_window->IsQuakeWindow() && _logic.IsQuakeWindow())
{
_UpdateTrayIcon();
}
to show the icon as soon as the quake window is created? Or do we want the icon to only appear when the window is minimized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know what, I like that. The icon will kinda serve as an indicator that a quake window is alive (hidden or not), and the user can always rely on that icon to get focus the quake window.
In fact, right now the icon is still visible when the quake window is visible. So, it's actually a bit inconsistent because the first time you bring up the quake window, the icon isn't there yet (because it'll only add it on a minimize). So yeah I think I'll add the icon as soon as the window's created.
I guess another route that's possible is to only show the icon when the quake window is minimized, and if it's not (aka. shown/restored), remove the icon? Still kinda like having the icon there in all states though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still kinda like having the icon there in all states though.
me too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked through the spec in a while, but wasn't the plan to have the tray icon serve as a way to target windows with specific names and stuff like that? Like, if the tray icon is supposed to provide functionality outside of quake mode, shouldn't we always show it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[feature today] -> [feature tomorrow] -> [feature in 1 year] 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this iteration of the tray icon (which only works for quake mode), it makes sense to me to show it only if there's a quake window alive. Once I've gotten more of the minimize to tray/tray icon features down that warrant the "always show" behavior, it should be fairly simple to make that change.
This comment has been minimized.
This comment has been minimized.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
🎉 Handy links: |
This PR is a small start in a broader "Minimize to Tray" feature (#5727).
This particular change is scoped only to the scenario when a quake window
is minimized. Currently the only way to bring back the quake window
when it's minimized is to press the global hotkey again. This gives another
option - to press the terminal icon in the tray.
Eventually though, minimize to tray will be available for any window, and
I'd like more time to flesh out the general porpoise scenarios and context
menus. Having just a bit in this PR also helps reviewers by keeping it small!