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

Implement ConEmu's OSC 9;4 to set the taskbar progress indicator #8055

Merged
merged 25 commits into from Nov 18, 2020

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Oct 27, 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

@zadjii-msft
Copy link
Member

  1. Oh no, this looks like it's branched off the hyperlink branch, so the commit history is a mess. Just a heads up.

  2. re

    TODO: We need to figure out how to handle multiple tabs sending the sequence and overriding each other

    I'm imagining that this state is stored per-control, kinda like the application title. When we switch between panes/tabs, then we'll need to ask the control what its current progress state is, and bubble that state up.

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'm blocking mostly over the 3 thing, but there's still a bunch to do on this PR so I guess this block is mostly for show

src/terminal/parser/OutputStateMachineEngine.cpp Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/IslandWindow.cpp 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 Oct 27, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 27, 2020
@@ -29,6 +29,7 @@ Module Name:
#include <cstring>
#include <shellscalingapi.h>
#include <windowsx.h>
#include <ShObjIdl.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want to add this to pch.h? Personally I try to avoid adding things into PCH because the damage it does to compiling speed.

Copy link
Member

Choose a reason for hiding this comment

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

meh, this one's not so bad. We'll probably end up using it in other places across the project, and I'd rather eat the compile time once in the pch phase rather than many times across rebuilds.

It's the winrt headers that really chew up pch time 😑

@zadjii-msft zadjii-msft self-assigned this Oct 28, 2020
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.

Wow these are such nits for something that works so well and I'm so excited about

src/cascadia/TerminalControl/TermControl.h Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/AppLogic.idl Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/IslandWindow.h Outdated Show resolved Hide resolved
@@ -29,6 +29,7 @@ Module Name:
#include <cstring>
#include <shellscalingapi.h>
#include <windowsx.h>
#include <ShObjIdl.h>
Copy link
Member

Choose a reason for hiding this comment

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

meh, this one's not so bad. We'll probably end up using it in other places across the project, and I'd rather eat the compile time once in the pch phase rather than many times across rebuilds.

It's the winrt headers that really chew up pch time 😑

src/terminal/parser/OutputStateMachineEngine.cpp Outdated Show resolved Hide resolved
src/terminal/parser/OutputStateMachineEngine.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 Oct 28, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 28, 2020
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 REALLY want to sign off on this. I think it's ready to go, save for two things:

  • make sure the pfn isn't nullptr in TerminalApi.cpp
  • Maybe add tests to UnitTests_TerminalCore? We've got tests in ScreenBufferTests.cpp that write VT sequences to the buffer and make sure that the state of the SCREEN_INFORMATION in conhost is updated as we'd expect, maybe we should add something similar here?

src/cascadia/TerminalCore/TerminalApi.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 Oct 29, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 29, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 18, 2020
@DHowett DHowett changed the title Implementation for OSC 9;4 - setting the taskbar progress indicator Implement ConEmu's OSC 9;4 to set the taskbar progress indicator Nov 18, 2020
@ghost ghost added Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase labels Nov 18, 2020
@DHowett DHowett self-assigned this Nov 18, 2020
@DHowett
Copy link
Member

DHowett commented Nov 18, 2020

ohh noo there's another size_t <- uint64 conversion issue in AppHost.

Might be easiest to try to build the x86 version locally.

@DHowett DHowett merged commit 16e8a84 into main Nov 18, 2020
@DHowett DHowett deleted the dev/pabhoj/taskbar_progress branch November 18, 2020 22:24
DHowett pushed a commit that referenced this pull request Nov 20, 2020
## Summary of the Pull Request

Does what the title says. Now while you're building terminal projects,
the taskbar will show indeterminate progress. If the build fails, it'll
blink the error state for 500ms before returning to normal.

## References
* Made possible by #8055 _and viewers like you_

## PR Checklist
* [x] scratches an itch I've had since at least 2018
* [x] I work here
* [x] this is a build script

## Validation Steps Performed
tested manually
mpela81 pushed a commit to mpela81/terminal that referenced this pull request Nov 22, 2020
…8335)

## Summary of the Pull Request

Does what the title says. Now while you're building terminal projects,
the taskbar will show indeterminate progress. If the build fails, it'll
blink the error state for 500ms before returning to normal.

## References
* Made possible by microsoft#8055 _and viewers like you_

## PR Checklist
* [x] scratches an itch I've had since at least 2018
* [x] I work here
* [x] this is a build script

## Validation Steps Performed
tested manually
@sba923
Copy link

sba923 commented Jan 26, 2021

I must be missing something. I'm running Windows Terminal 1.4.3243.0, and this (PowerShell 7.1.1, FWIW) doesn't do anything:

Write-Host -NoNewline "`e]9;4;1;50`e\"

RTFM answers kindly accepted 😋

@KalleOlaviNiemitalo
Copy link

@sba923, this feature is on the release-1.6 branch, but not on the release-1.4 and release-1.5 branches.

@sba923
Copy link

sba923 commented Jan 26, 2021

@sba923, this feature is on the release-1.6 branch, but not on the release-1.4 and release-1.5 branches.

Phew. I haven't gone mad after all. I just need to be patient 😉 or build myself a 1.6 binary.

@sba923
Copy link

sba923 commented Jan 28, 2021

@sba923, this feature is on the release-1.6 branch, but not on the release-1.4 and release-1.5 branches.

I don't have to wait anymore thanks to v1.6.10272.0 Preview

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. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Taskbar progress indicator
9 participants