Add a progress ring indicator to the tab header by PankajBhojwani · Pull Request #8133 · microsoft/terminal

PankajBhojwani · 2020-11-02T19:21:27Z

This commit adds a progress ring to the tab header when we receive an
OSC 9 sequence.

Adds an event handler in Tab.cpp for the event we raise when we get a
request to set the taskbar state/progress. This event handler updates
the tab header with the active control's state/progress.

When we want to show the progress ring, we hide the tab icon and place
the progress ring over it.

References #6700

…/pabhoj/taskbar_progress

…state and progress params

skyline75489 · 2020-11-03T03:09:43Z

Maybe we should extract some common pattern in this PR to support features like #8106 in the future.

zadjii-msft · 2020-11-09T11:55:02Z

Maybe we should extract some common pattern in this PR to support features like #8106 in the future.

Yea, i think we've put that off for far too long. I've filed #8201 to track that work ☺️

src/cascadia/TerminalControl/IControlSettings.idl

DHowett · 2020-12-09T22:25:38Z

src/cascadia/TerminalSettingsModel/GlobalAppSettings.h

        GETSET_SETTING(bool, AlwaysOnTop, false);
        GETSET_SETTING(Model::TabSwitcherMode, TabSwitcherMode, Model::TabSwitcherMode::InOrder);
        GETSET_SETTING(bool, DisableAnimations, false);
+        GETSET_SETTING(bool, DisableProgressRing, false);


quick check on: why is this a setting? is it too specific? should we have an enum for progressIndicatorStyle? tab|taskbar|all|none? I don't know!

That's... a fair point. Given that browsers/Visual Studio/other things that have progress indicators do not offer a way to disable them (as far as I know), I'm thinking we remove the setting.

Thoughts?

DHowett

Notes inline

PankajBhojwani · 2020-12-11T21:17:04Z

disableProgressRing setting has been removed!

DHowett · 2020-12-16T02:43:32Z

Moved from body:

Determinate: (30 -> 80 -> inactive)

Indeterminate:

ghost · 2020-12-16T02:45:06Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

skyline75489

Confession: I never really get it why this means "shipit".

DHowett · 2020-12-16T02:46:34Z

excellent question!

zadjii-msft · 2020-12-16T03:08:17Z

@skyline75489
https://www.quora.com/On-GitHub-what-is-the-significance-of-the-Ship-It-squirrel

skyline75489 · 2020-12-16T03:18:46Z

@zadjii-msft Thanks! Such a straightforward answer, which takes only ~4000 words if you include the background story.

Originally when I saw in some of PRs in the repo, I imagine the meaning of it is "huh you know what, the PR is good, but not that good, so I need to take a look, but from a distance, in a sneaky way, like a squirrel/mouse, for some reason". Yeah, my mind is weird, I know.

This commit adds a [progress ring] to the tab header when we receive an OSC 9 sequence. Adds an event handler in `Tab.cpp` for the event we raise when we get a request to set the taskbar state/progress. This event handler updates the tab header with the active control's state/progress. When we want to show the progress ring, we hide the tab icon and place the progress ring over it. [progress ring]: https://docs.microsoft.com/en-us/uwp/api/Microsoft.UI.Xaml.Controls.ProgressRing?view=winui-2.4 References microsoft#6700

It turns out that the negative margin for the progress ring is causing the clipping in case the tab title gets too long: https://github.com/microsoft/terminal/blob/43dbbd590fa4b46c37e9970415f8150d3c399598/src/cascadia/TerminalApp/TabHeaderControl.xaml#L18-L27 The negative margin was introduced in #8113 because the progress ring is supposed to replace the tab icon but the `TabView` still reserves space even if no icon is set (see #8133 (comment)). However, it is not actually the `TabView` reserving space even when there is no icon, but a workaround for a crash in the `IconPathConverter` that returns a `BitmapIconSource` with a `nullptr` source instead of a `nullptr` `IconSource`: https://github.com/microsoft/terminal/blob/43dbbd590fa4b46c37e9970415f8150d3c399598/src/cascadia/TerminalSettingsModel/IconPathConverter.cpp#L143-L154 The workaround in `IconPathConverter` could probably be removed as I did not find any instance where it is still used in a way that could trigger the mentioned crash, but I did not dare to just remove it as I do not know enough about the code by far. Hence, I opted to just locally instantiate the `IconSource` with a `nullptr` directly in `TerminalTab`. Fixes #8910

It turns out that the negative margin for the progress ring is causing the clipping in case the tab title gets too long: https://github.com/microsoft/terminal/blob/43dbbd590fa4b46c37e9970415f8150d3c399598/src/cascadia/TerminalApp/TabHeaderControl.xaml#L18-L27 The negative margin was introduced in #8113 because the progress ring is supposed to replace the tab icon but the `TabView` still reserves space even if no icon is set (see #8133 (comment)). However, it is not actually the `TabView` reserving space even when there is no icon, but a workaround for a crash in the `IconPathConverter` that returns a `BitmapIconSource` with a `nullptr` source instead of a `nullptr` `IconSource`: https://github.com/microsoft/terminal/blob/43dbbd590fa4b46c37e9970415f8150d3c399598/src/cascadia/TerminalSettingsModel/IconPathConverter.cpp#L143-L154 The workaround in `IconPathConverter` could probably be removed as I did not find any instance where it is still used in a way that could trigger the mentioned crash, but I did not dare to just remove it as I do not know enough about the code by far. Hence, I opted to just locally instantiate the `IconSource` with a `nullptr` directly in `TerminalTab`. Fixes #8910 (cherry picked from commit 21a9c55) Service-Card-Id: 86209056 Service-Version: 1.16

PankajBhojwani added 15 commits October 27, 2020 13:03

osc 9;4 implementation

af9e4d5

spelling

606490f

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

0decbea

…/pabhoj/taskbar_progress

address comments

4e6f6fe

add paused state

da83748

use last focused tab's progress and state

3d0ed84

use split string

f08c962

use sender instead of args, some name changes

5f83dfd

remove all references to SetTaskbarProgressEventArgs, validation for …

ab99ed5

…state and progress params

tests

4125052

conflict with main

a900678

get the taskbar state and progress from the last active control

0aa5667

addressing comments, cleanups

e69d0af

com ptr

57d8e07

initial implementation

0f54cf9

PankajBhojwani changed the title ~~Progress ring indicator in tab header~~ [DRAFT] Progress ring indicator in tab header Nov 2, 2020

add setting to disable progress ring

a7e7186

PankajBhojwani mentioned this pull request Nov 2, 2020

[Scenario] Progress Bar Follow-ups #6700

Open

4 tasks

zadjii-msft mentioned this pull request Nov 9, 2020

Make the TabViewItemHeader a custom control #8201

Closed

Base automatically changed from dev/pabhoj/taskbar_progress to main November 18, 2020 22:24

PankajBhojwani added 6 commits December 3, 2020 16:14

conflict

c262d83

cleaning up remnants from before merge

31a4143

deleting more remnants

093d78b

wrong schema

5dfde59

tab catches control's event

435f44f

implement progress ring

705c07c

PankajBhojwani changed the title ~~[DRAFT] Progress ring indicator in tab header~~ Progress ring indicator in tab header Dec 4, 2020

PankajBhojwani marked this pull request as ready for review December 4, 2020 19:13

carlos-zamora approved these changes Dec 9, 2020

View reviewed changes

DHowett reviewed Dec 9, 2020

View reviewed changes

src/cascadia/TerminalControl/IControlSettings.idl Outdated Show resolved Hide resolved

DHowett reviewed Dec 9, 2020

View reviewed changes

DHowett requested changes Dec 9, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 9, 2020

remove setting from IControlSettings

bb0e4a0

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 10, 2020

remove setting

c8f301b

zadjii-msft approved these changes Dec 15, 2020

View reviewed changes

zadjii-msft assigned DHowett Dec 15, 2020

DHowett approved these changes Dec 16, 2020

View reviewed changes

DHowett changed the title ~~Progress ring indicator in tab header~~ Add a progress ring indicator to the tab header Dec 16, 2020

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 16, 2020

ghost merged commit 551cc9a into main Dec 16, 2020

ghost deleted the dev/pabhoj/ring_in_tab branch December 16, 2020 02:45

skyline75489 approved these changes Dec 16, 2020

View reviewed changes

ghost removed the Needs-Second It's a PR that needs another sign-off label Dec 16, 2020

DHowett mentioned this pull request Dec 16, 2020

Improve rendering of Progress PowerShell/PowerShell#14426

Closed

zadjii-msft mentioned this pull request Jan 15, 2021

Show indicator of bell in tab #8637

Merged

KalleOlaviNiemitalo mentioned this pull request Mar 13, 2021

Show error state in progress ring of tab #9481

Open

jonathan-meier mentioned this pull request Oct 9, 2022

Fix clipped progress ring in tab when tab title is too long #14167

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a progress ring indicator to the tab header#8133

Add a progress ring indicator to the tab header#8133
28 commits merged intomainfrom
dev/pabhoj/ring_in_tab

PankajBhojwani commented Nov 2, 2020 •

edited by DHowett

Loading

Uh oh!

skyline75489 commented Nov 3, 2020

Uh oh!

zadjii-msft commented Nov 9, 2020

Uh oh!

Uh oh!

DHowett Dec 9, 2020

Uh oh!

PankajBhojwani Dec 10, 2020

Uh oh!

DHowett left a comment

Uh oh!

PankajBhojwani commented Dec 11, 2020

Uh oh!

DHowett commented Dec 16, 2020

Uh oh!

ghost commented Dec 16, 2020

Uh oh!

skyline75489 left a comment

Uh oh!

DHowett commented Dec 16, 2020

Uh oh!

zadjii-msft commented Dec 16, 2020

Uh oh!

skyline75489 commented Dec 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

PankajBhojwani commented Nov 2, 2020 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skyline75489 commented Nov 3, 2020

Uh oh!

zadjii-msft commented Nov 9, 2020

Uh oh!

Uh oh!

DHowett Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

PankajBhojwani Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

PankajBhojwani commented Dec 11, 2020

Uh oh!

DHowett commented Dec 16, 2020

Uh oh!

ghost commented Dec 16, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

skyline75489 left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett commented Dec 16, 2020

Uh oh!

zadjii-msft commented Dec 16, 2020

Uh oh!

skyline75489 commented Dec 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

PankajBhojwani commented Nov 2, 2020 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.