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

Feature Request: Taskbar progress indicator #3004

Closed
dim5 opened this issue Oct 1, 2019 · 26 comments · Fixed by #8055
Closed

Feature Request: Taskbar progress indicator #3004

dim5 opened this issue Oct 1, 2019 · 26 comments · Fixed by #8055
Assignees
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@dim5
Copy link

dim5 commented Oct 1, 2019

Description of the new feature/enhancement

Add a taskbar progress indicator for displaying the status of the current long running process (when possible).

For example, this can be helpful to users unpacking files with 7z or running python scripts with tqdm.

ConEmu's documentation.

@dim5 dim5 added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Oct 1, 2019
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Oct 1, 2019
@zadjii-msft
Copy link
Member

zadjii-msft commented Oct 1, 2019

I could have sworn we had an internal issue for this feature, but it never got migrated to github it appears....

FOUND IT! MSFT:18258406. Here are the relevant notes:

sequence description
ESC ] 9 ; 4 ; st ; pr ST Set progress state on Windows 7 taskbar and ConEmu title. When st is 0: remove progress. When st is 1: set progress value to pr (number, 0-100). When st is 2: set error state in progress on Windows 7 taskbar

Also related documentation: ITaskbarList3::SetProgressValue

We should probably also make sure that we support the TBPF_INDETERMINATE state - for something like our own bcz.cmd, where we don't know how long the operation will take.

In conpty mode, we should just pass these sequences through. There's a good amount of precedent for such sequences in the code already.

EDIT:
We should probably also have a couple settings for controlling this behavior:

  • showProgressInTaskbar (default true) to show the progress in the taskbar entry
  • showProgressInTab (default true) to show the progress in the tab itself.

showProgressInTab can probably be a follow up once microsoft/microsoft-ui-xaml#1386 is complete

@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Oct 1, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 1, 2019
@zadjii-msft zadjii-msft added Needs-Tag-Fix Doesn't match tag requirements and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Oct 1, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 1, 2019
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Oct 1, 2019
@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. labels Oct 1, 2019
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 23, 2019
@shmuelie
Copy link

Would this make a good "starter" issue?

@DHowett-MSFT
Copy link
Contributor

@SamuelEnglard

Perhaps! Anyone’s free to have a crack at any issue they find here unless the team’s actively assigned, and even then they just have to ask. It’s got a couple moving parts, but nothing impossible.

For anyone coming from Windows land: the reason the existing solution (calling GetConsoleWindow and using the Shell APIs directly to set progress on the window) doesn’t work is that GetConsoleWindow is an absolute abomination of an API and we never should have let a windowless application access the window it just happened to be hosted in. Circumventing the console and telling the shell to paint progress isn’t even something we can detect; we need applications to move to a standards-compliant or, at the least, quasi- standards-compliant, mechanism for setting progress.

@zadjii-msft
Copy link
Member

More to the point, the work that I think is involved here is roughly:

  • Change the OutputStateMachineEngine to be able to parse these sequences and pass them through to the connected terminal, when the console is operating as a conpty. (this might already work).
  • When the state machine isn't in conpty mode, it'll need to call methods on ITerminalDispatch. These should be left unimplemented for conhost. I believe that means no-op'ing them in adaptDispatch, or probably just returning false.
  • For the Windows Terminal, these ITerminalDispatch methods (implemented in TerminalDispatch) should call (new) methods on ITerminalApi, implemented in Terminal.
    • The Terminal should probably stash whatever the progress bar state is inside itself, and expose methods for querying that state.
  • These Terminal methods should probably trigger some sort of callback, that TermControl can listen for.
  • TermControl's callback should merely raise an event that can be used to bubble the event up to TerminalPage.
  • TerminalPage should add listeners to TermControls as they're created, and use those listeners to bubble it's own progress event that the AppHost can listen for.
    • Note that the page should only bubble the event of the currently focused tab/pane, and when the tab/pane focus changes, it should raise a new event to match the state of the focused control. (this is why we needed the "methods for querying that state" on Terminal). This would be much like the events we have for the tab title currently (though maybe a little simpler).
  • AppHost should listen for events from the TerminalPage to know when it should update the progress bar on the title. AppHost is actually the object that knows about the HWND that's hosting the terminal.

This is kinda the bottom-up approach I'd take to implementing this myself for the Terminal. I'd probably want to start by looking at the last bullet point, actually. That way I'd know what shape the event that's going to get bubbled all the way through all these layers would need. I'd reckon that will line up pretty close with the VT sequence we'll be parsing at the start.

This would probably be a lot easier if you just wanted to implement it for conhost. Not nearly as much bubbling (but not no bubbling).

The events for setting the title will be really similar actually. There's a bit of extra logic for handling the title, depending on if the user has suppressApplicationTitle or any of those related settings, but you could probably ignore that for now.

@j4james
Copy link
Collaborator

j4james commented Oct 27, 2020

We should probably also make sure that we support the TBPF_INDETERMINATE state - for something like our own bcz.cmd, where we don't know how long the operation will take.

Note that ConEmu does already support the indeterminate state, by setting the st parameter to 3. It's just not documented.

@mintty
Copy link

mintty commented Oct 27, 2020

Why would the "green" and "red" options be supported but not the "yellow"?
Since OSC sequence also support text parameters, this could be done in a compatible way like:
ESC ] 9 ; 4 ; yellow ; pr ST

@j4james
Copy link
Collaborator

j4james commented Oct 27, 2020

Why would the "green" and "red" options be supported but not the "yellow"?

The best place to ask this would be on on the ConEmu repo.

@zadjii-msft
Copy link
Member

Thanks for the quick reply @Maximus5!

Maximus5 added a commit to Maximus5/ConEmu that referenced this issue Nov 1, 2020
… Windows TaskBar).

  ANSI sequence `ESC ] 9 ; 4 ; 4 ; pr ST` or GuiMacro `Progress(4, pr)` where `pr` is a percentage 0 .. 100.

  ref: microsoft/terminal#3004
@ghost ghost added the In-PR This issue has a related PR label Nov 18, 2020
DHowett pushed a commit that referenced this issue Nov 18, 2020
This commit implements the OSC 9;4 sequence per the [ConEmu style].

| sequence                   | description                                       |
| ------------               | ------------                                      |
| `ESC ] 9 ; 4 ; st ; pr ST` | Set progress state on taskbar and tab.            |
|                            | When `st` is:                                     |
|                            |                                                   |
|                            | `0`: remove progress.                             |
|                            | `1`: set progress value to `pr` (number, 0-100).  |
|                            | `2`: set the taskbar to the "Error" state         |
|                            | `3`: set the taskbar to the "Indeterminate" state |
|                            | `4`: set the taskbar to the "Warning" state       |

We've also extended this with:
* st 3: set indeterminate state
* st 4: set paused state

We handle multiple tabs sending the sequence by using the the last focused
control's taskbar state/progress.

Upon receiving the sequence in `TerminalApi`, we send an event that gets caught
by `TerminalPage`. `TerminalPage` then fires another event that gets caught by
`AppHost` and that's where we set the taskbar progress. 

Closes #3004 

[ConEmu style]: https://conemu.github.io/en/AnsiEscapeCodes.html#ConEmu_specific_OSC
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Needs-Tag-Fix Doesn't match tag requirements and removed In-PR This issue has a related PR labels Nov 18, 2020
@mixmastamyk
Copy link

mixmastamyk commented Feb 2, 2021

This unfortunately conflicts with iterm2 and kitty use of OSC 9. Not the end of the world since neither supports the others notification, but definitely problematic.

@mintty
Copy link

mintty commented Feb 2, 2021

Both could coexist as long as messages do not begin with "4;"...

@j4james
Copy link
Collaborator

j4james commented Feb 2, 2021

This unfortunately conflicts with iterm2 and kitty use of OSC 9.

This was discussed in the PR and the terminal-wg issue that you linked. We reached the conclusion that WT probably wouldn't need to support iterm2's use of OSC 9, because if we did decide to implement notifications, we'd be more likely to go with OSC 777, since that's more widely supported.

@mixmastamyk
Copy link

Ok, but what about from the app developer's point of view? I just implemented the progress reporting in my library, and it's nice, thanks.

However, when I run my python script on kitty (and presumably iterm and others?) I get a ton of bogus desktop notifications "4;0;10", "4;0;20", etc.... instead. I had to put this BS in my code to work around it:

if os.name = 'nt':
    # impl 1 ...        
else:
    # impl 2 ...

The least bad solution was probably using another number. As I read the discussion I realized Mr. Kovid is difficult, but I think he was right, we're not running out of integers any time soon.

It's a shame that iterm and conemu chose conflicting sequences ~ten years ago, and there's nothing we can do about that. But now that folks are at least in some communication there's no reason to double down on them. ;-)

@j4james
Copy link
Collaborator

j4james commented Feb 2, 2021

As I read the discussion I realized Mr. Kovid is difficult, but I think he was right, we're not running out of integers any time soon.

If he really cared about the conflict, though, he didn't have to implement OSC 9 as a notification sequence - kitty already supports notification through another OSC sequence. And while I can sympathise with iTerm2, gnachman didn't seem particular attached to OSC 9 when the subject of notifications was being discussed, so it's quite possible he might drop it one day.

@DHowett

This comment has been minimized.

@mintty
Copy link

mintty commented Feb 2, 2021

mintty also implementes CSIN%q as an alternative sequence;
see https://github.com/mintty/mintty/wiki/CtrlSeqs#progress-bar but note the different numbering

@mixmastamyk
Copy link

As an application developer, I'd like one sequence that works as many places as possible without conflicts…

@zadjii-msft
Copy link
Member

If you want consistency, I don't think the interpretation of escape sequences is a good place to be 😆

There's just already so much fragmentation in this space, especially with OSC sequences. No one really standardized them in the first place, so everyone just went wild west claiming whatever they wanted. Not much we can do about it now unfortunately.

Curious to me that iTerm2 chose OSC 9. Looks like the sequence must be very old in iTerm2: kovidgoyal/kitty#1474 (comment). Plus, ConEmu has been using OSC 9 as their prefix for years. So it really doesn't make sense to me to double down on OSC9 -> notification. It seems like it was an oversight when it was authored, the current author isn't attached to it, and there are better (non-conflicting alternatives).

We could also support the mintty version of the sequence if that doesn't conflict too bad. Don't see why not.

@j4james
Copy link
Collaborator

j4james commented Feb 4, 2021

We could also support the mintty version of the sequence

If the point is to have a sequence that won't break kitty, then that probably isn't going to help. The last version I tested had issues with intermediates (at least some of them), which caused the sequences to be misinterpreted as something else. You may not get a notification popup, but you'll probably get some other weird breakage.

@shmuelie
Copy link

shmuelie commented Feb 4, 2021

When in doubt, offer the option to change which OSC is used in settings?

@DHowett
Copy link
Member

DHowett commented Feb 4, 2021

That only complicates the feature matrix, though. For @mixmastamyk's "application developer" case, now it's just unreliable rather than "consistently different from iTerm2."

@shmuelie
Copy link

shmuelie commented Feb 4, 2021

That is true, would need a way to tell Terminal "this application uses OSC 9 for notifications, please do..."

@zadjii-msft
Copy link
Member

Man if only there was some sort of database that an app could use to look up "this is how you emit progress bar updates, this is how you send notifications, this is how you set the colors" for various different terminals 😝

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 Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants