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 support for OSC777 - send notification #14425

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Nov 22, 2022

This adds support for the OSC 777 ; notify ; title ; body ST sequence. This allows client applications to send a notification to the Terminal. When this notification is clicked, it summons the terminal window that sent it.

This can be used in PowerShell as follows:

Write-Output "`e]777;notify;Hello From the Terminal;This is a notification sent by the client application`a"
 
# but do it with a sleep, because notifications while the Terminal is the FG window won't do anything
sleep 2; write-Output "`e]777;notify;Hello From the Terminal;This is a notification sent by the client </text><text>application`a"

other details

todo

@ghost ghost added Area-VT Virtual Terminal sequence support Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Nov 22, 2022
Comment on lines +2472 to +2479
<toast>\
<visual>\
<binding template=\"ToastGeneric\">\
<text></text>\
<text></text>\
</binding>\
</visual>\
</toast>" };
Copy link
Member

@lhecker lhecker Nov 22, 2022

Choose a reason for hiding this comment

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

Our notification system uses full blown XML instead of data transfer objects?
...and instead of making it use structured data we added a AppNotifications.Builder that still spits out XML?

Why is WinRT so awful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Lemme tell you, it's a disaster. At least the sample has like, 25% c++winrt code these days. The original sample was "Well, the API it hard to use, so go get this third party nuget package, and do some C#" 🤦

@WSLUser
Copy link
Contributor

WSLUser commented Nov 28, 2022

Is there going to be an optional WinToast notification as well users can set in settings.json or Settings UI (Similar to BEL where we could have visual, audio or both) where here we have "only within the Terminal, only in the Action Center or both"?)

@zadjii-msft
Copy link
Member Author

Is there going to be an optional WinToast notification as well users can set in settings.json or Settings UI (Similar to BEL where we could have visual, audio or both) where here we have "only within the Terminal, only in the Action Center or both"?)

I'd reckon probably not. Toasts have been harder to do right in the Terminal than it's been worth (see #8592).

@zadjii-msft zadjii-msft mentioned this pull request Nov 28, 2022
7 tasks
@WSLUser
Copy link
Contributor

WSLUser commented Nov 28, 2022

The issue you linked sounds a bit different than the guide I provided within 7718 (which this PR would close) but maybe they're the same thing.

@zadjii-msft
Copy link
Member Author

@WSLUser You linked to https://learn.microsoft.com/en-us/windows/apps/design/shell/tiles-and-notifications/scheduled-toast in #7718, which looks like a guide around using a C# wrapper around the same toast notification APIs I'm using here. Windows apparently just calls all these notifications "toast notifications", which kinda muddies the water when comparing to something like Android (where Toasts and Notifications are two very different concepts).

src/cascadia/TerminalApp/pch.h Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/pch.h Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/ControlCore.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
@zadjii-msft zadjii-msft self-assigned this Nov 30, 2022
@zadjii-msft
Copy link
Member Author

Elevated?

Oh hey, this is a problem. We might have to disable this entirely for elevated instances.

The notification is obviously running unelevated. When it calls back to our app, you guessed it, it calls into the unelevated instance. We can't trivially have a notification call back to an elevated instance. We'd need to have the unelevated ActivatedEventArgs handler take the args, then use them to invoke an elevated wt.exe with some commandline to try and summon the window. Something like wt summon -w {id} ala #13276. That's obviously a long way off - I'm just gonna disable this when elevated for now.

@zadjii-msft
Copy link
Member Author

sequenceDiagram
    participant OS
    participant WT.exe[1]
    participant WT.exe[2]
    activate WT.exe[1]
    WT.exe[1]->>-OS: Sends toast notification 
    note left of OS: some time later
    OS->>+WT.exe[2]: User clicks notification 
    WT.exe[2]->>-WT.exe[1]: Tell [1] to restore
    activate WT.exe[1]
    note left of WT.exe[1]: restores to bottom<br> of z-order?
    deactivate WT.exe[1]  

Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

One issue, otherwise it looks fine. But it needs to updated with the new non-multi-process model in mind right?

{
if (til::at(pairParts, 0) == L"window")
{
window = std::wcstoul(pairParts[1].data(), nullptr, 10);
Copy link
Member

Choose a reason for hiding this comment

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

The parts aren't null terminated so this won't work. You can use til::to_ulong instead.

@zadjii-msft
Copy link
Member Author

Note to self - pushed a merge of this with main to dev/migrie/fhl/7718-notifications-reboot. Still need to re-plumb the notification actually activating the window, but we're close. I'll merge that into here after I do that.

src/cascadia/TerminalApp/TerminalPage.cpp Fixed Show fixed Hide fixed
src/terminal/adapter/ITermDispatch.hpp Fixed Show fixed Hide fixed
src/cascadia/TerminalApp/TerminalPage.cpp Fixed Show fixed Hide fixed
src/terminal/adapter/adaptDispatch.cpp Fixed Show fixed Hide fixed
src/terminal/adapter/adaptDispatch.hpp Fixed Show fixed Hide fixed
src/terminal/adapter/termDispatch.hpp Fixed Show fixed Hide fixed

This comment has been minimized.

@zadjii-msft
Copy link
Member Author

Further notes from 4ab628d:

  • Testing this while debugging will not work. If you have a debugger attached to a process, Windows DNGAF about the FG rules and lets the terminal come to the FG. Without the debugger, this pretty much never works. At least, not on 26051.1000.amd64fre.ge_release_we_adept.240201-1823
  • Test case: sleep 2; write-Output "`e]777..., then minimize, then click another app, then click the notification
    • Terminal gets restored, and activated, but not brought to FG or the top
    • When I ask what the FG window is, it's a "New notification" window from ShellExperienceHost. NOT the Terminal
    • I tried AllowSetForeground(monarch.GetPID()) but that did nothing, and the HR was E_ACCESSDENIED
    • I tried to create a message window in the emperor of the toast wt.exe, but that didn't give us FG

Does this just... not work ever?

@zadjii-msft
Copy link
Member Author

At this point, I'm very tempted to stick this behind velocity and only enable for Dev & Canary builds, while we sort out the foreground issues

@DHowett
Copy link
Member

DHowett commented Feb 7, 2024

I'm very on the fence about this, even checked in behind velocity.

  • It doesn't work for unpackaged builds (it requires package identity)
  • It's broken if Terminal isn't the foreground application, since it can't activate us
    • Much of the utility of notifications is that they bring you to the thing that notified you
  • It doesn't work on Windows 10 ??
  • It doesn't work when you're elevated?
  • In the limited cases where it actually does work, it allows a text-mode application to spam users with system-wide toasts
    • However, there's no possible "action" response, so they can't gate actual complicated and meaningful features behind notifications (e.g. to perform a thing on click, or to receive a text response)

Frankly, I still don't see the value in offering this support! 😄

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

(Stake in the ground)

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 7, 2024
@zadjii-msft
Copy link
Member Author

  • lemme look into this more. There's surely got to be other ways of doing this unpackaged, or this would be a good thing to send to our WASDK friends
  • LMAO we have just discovered that this is a platform issue. We'll get that fixed internally. I don't think that's a reason to prevent it from merging.
  • Almost certainly the same as the above
  • I have an i d e a here which I'll be excited to talk about tomorrow
  • Yep. That's... kinda the whole point. I believe semantically, the idea is to be used for after a long-running command completes. I wouldn't be opposed to a per-profile disableNotifications setting or something, for more granular control than just Windows Settings
    • The original spec doesn't support actions, it literally just sends a message. There's no existing spec for "send a notification with buttons", this was just supposed to implement the existing osc777.

fite-me.emoji

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Attention The core contributors need to come back around and look at this ASAP. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send Desktop Notification via VT Sequence (OSC777)
6 participants