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

Add tray icon when quake window is minimized #10179

Merged
merged 17 commits into from
Jul 8, 2021
Merged

Conversation

leonMSFT
Copy link
Contributor

@leonMSFT leonMSFT commented May 25, 2021

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!

@github-actions

This comment has been minimized.

@leonMSFT leonMSFT added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels May 25, 2021
@leonMSFT leonMSFT requested a review from zadjii-msft May 25, 2021 16:57
@zadjii-msft
Copy link
Member

Having just a bit in this PR also helps reviewers by keeping it small!

AMEN

Copy link
Member

@zadjii-msft zadjii-msft left a 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

src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
Shell_NotifyIcon(NIM_DELETE, &_trayIconData.value());
_trayIconData.reset();
}
}
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Member

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?

Copy link
Member

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] 😄

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

src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/AppHost.cpp Show resolved Hide resolved
src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

src/cascadia/WindowsTerminal/AppHost.cpp Show resolved Hide resolved
src/cascadia/WindowsTerminal/IslandWindow.h Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/icon.cpp Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/icon.cpp Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 28, 2021
@DHowett DHowett added Needs-Second It's a PR that needs another sign-off and removed Needs-Second It's a PR that needs another sign-off labels Jun 28, 2021
@ghost ghost requested a review from miniksa June 28, 2021 15:35
@ghost ghost requested review from PankajBhojwani and lhecker June 28, 2021 15:35
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 28, 2021
@leonMSFT
Copy link
Contributor Author

leonMSFT commented Jul 2, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@leonMSFT leonMSFT merged commit 96f4a9d into main Jul 8, 2021
@ghost
Copy link

ghost commented Jul 14, 2021

🎉Windows Terminal Preview v1.10.1933.0 has been released which incorporates this pull request.:tada:

Handy links:

@DHowett DHowett deleted the dev/lelian/notifyicon/proto branch October 26, 2021 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants