Update upgrade notification message by SteveL-MSFT · Pull Request #10777 · PowerShell/PowerShell

SteveL-MSFT · 2019-10-12T04:12:23Z

PR Summary

If terminal doesn't support VT, no colors are used. Also added aka.ms shortcut. Padding had to be calculated to make sure it looks nice which means
if the message line lengths change, the code may need to be updated as it expects the first line to be longest and the first and last line to include
the release tag. The stable version is the same but the word stable in place of preview.

Before:

After:

PR Context

PR Checklist

src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs

SteveL-MSFT · 2019-10-12T04:31:55Z

@PoshChan please retry static

PoshChan · 2019-10-12T04:32:19Z

@SteveL-MSFT, successfully started retry of PowerShell-CI-static-analysis

TylerLeonhardt

Due to feedback from another PR I strongly encourage not going with the hard-coded yellow

src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs

TylerLeonhardt · 2019-10-12T04:36:43Z

Also, this would be the second location the inverse escape sequence is hard coded in our code... We need to figure out what we're going to do about that in the future.

iSazonov · 2019-10-12T05:30:02Z

Coloring is very sensitive area. Windows has a special team for this. I'd prefer classic color scheme. This color kaleidoscope is distracting and annoying.
What will be next? Advertising banners? :-)

iSazonov · 2019-10-12T09:38:45Z

Coloring is nice for demo on a conference stand. For normal daily work, when the console is started dozens of times, it can tire your eyes.
Unix utilities have --color key but not pwsh.
I don’t like that we began to saturate the codewith color without a common approach.

SteveL-MSFT · 2019-10-12T14:25:27Z

@PoshChan please retry static

PoshChan · 2019-10-12T14:25:48Z

@SteveL-MSFT, successfully started retry of PowerShell-CI-static-analysis

SteveL-MSFT · 2019-10-12T14:26:21Z

@iSazonov I've started adding more color settings to $host.PrivateData (it's unfortunate that member is named that...) so users can opt out or customize.

src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs

iSazonov · 2019-10-12T14:42:41Z

@SteveL-MSFT I pulled #10778 to fix static CI.

iSazonov · 2019-10-12T14:46:47Z

I've started adding more color settings to $host.PrivateData (it's unfortunate that member is named that...) so users can opt out or customize.

Oh, it would be great to have modern design (RFC?) - PowerShell is power and I'd expect that we could do coloring gracefully, smart and powershelly :-)

vexx32 · 2019-10-12T14:49:44Z

Agreed. We shouldn't bury it in a $host API unless we also plan on exposing it via a cmdlet when it is reasonably feature-complete. 🙂

Couldn't we store color settings in the settings JSON file?

SteveL-MSFT · 2019-10-12T14:50:57Z

@vexx32 we should probably have a new issue to discuss that

iSazonov · 2019-10-12T14:59:44Z

#10780 new issue for coloring.

src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs

daxian-dbw · 2019-10-15T21:52:13Z

@joeyaiello Please review the user experience of the notification message.

daxian-dbw · 2019-10-15T21:52:47Z

@bergmeister Can you please also review this?

src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs

src/Microsoft.PowerShell.ConsoleHost/resources/ManagedEntranceStrings.resx

joeyaiello

String looks fine to me for now, we can easily iterate it over time. @SteveL-MSFT and I discussed possibly doing an "upgrade doc" that we can point to generically (/cc @sdwheeler) that would describe how updating should be done on each platform.

bergmeister

Lgtm, nice colour block

remove exclamations and make background yellow

d3bfe22

SteveL-MSFT requested a review from joeyaiello October 12, 2019 04:12

SteveL-MSFT requested review from anmenaga and daxian-dbw as code owners October 12, 2019 04:12

SteveL-MSFT assigned daxian-dbw Oct 12, 2019

SteveL-MSFT added this to the 7.0.0-preview.5 milestone Oct 12, 2019

PoshChan reviewed Oct 12, 2019

View reviewed changes

src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs Show resolved Hide resolved

TylerLeonhardt reviewed Oct 12, 2019

View reviewed changes

src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs Outdated Show resolved Hide resolved

PoshChan reviewed Oct 12, 2019

View reviewed changes

src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs Show resolved Hide resolved

remove color since it's not configurable

b322968

PoshChan reviewed Oct 12, 2019

View reviewed changes

src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs Show resolved Hide resolved

SteveL-MSFT changed the title ~~Update upgrade notification message to use color~~ Update upgrade notification message Oct 15, 2019

daxian-dbw reviewed Oct 15, 2019

View reviewed changes

src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs Outdated Show resolved Hide resolved

address Dongbo's feedback

c83e7e4

iSazonov reviewed Oct 16, 2019

View reviewed changes

src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs Outdated Show resolved Hide resolved

iSazonov reviewed Oct 16, 2019

View reviewed changes

src/Microsoft.PowerShell.ConsoleHost/resources/ManagedEntranceStrings.resx Show resolved Hide resolved

address Ilya's feedback

f0e09a0

joeyaiello approved these changes Oct 16, 2019

View reviewed changes

Remove and add new lines at a couple places

d370bd5

daxian-dbw approved these changes Oct 16, 2019

View reviewed changes

bergmeister approved these changes Oct 17, 2019

View reviewed changes

daxian-dbw merged commit cb66974 into PowerShell:master Oct 17, 2019

anmenaga added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Oct 22, 2019

kilasuit pushed a commit to kilasuit/PowerShell that referenced this pull request Nov 9, 2019

Update upgrade notification message (PowerShell#10777)

3c59b4e

SteveL-MSFT deleted the upgrade-message branch June 6, 2020 02:31

Conversation

SteveL-MSFT commented Oct 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Uh oh!

SteveL-MSFT commented Oct 12, 2019

Uh oh!

PoshChan commented Oct 12, 2019

Uh oh!

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TylerLeonhardt commented Oct 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Oct 12, 2019

Uh oh!

iSazonov commented Oct 12, 2019

Uh oh!

SteveL-MSFT commented Oct 12, 2019

Uh oh!

PoshChan commented Oct 12, 2019

Uh oh!

SteveL-MSFT commented Oct 12, 2019

Uh oh!

Uh oh!

iSazonov commented Oct 12, 2019

Uh oh!

iSazonov commented Oct 12, 2019

Uh oh!

vexx32 commented Oct 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT commented Oct 12, 2019

Uh oh!

iSazonov commented Oct 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

daxian-dbw commented Oct 15, 2019

Uh oh!

daxian-dbw commented Oct 15, 2019

Uh oh!

Uh oh!

Uh oh!

joeyaiello left a comment

Choose a reason for hiding this comment

Uh oh!

bergmeister left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

SteveL-MSFT commented Oct 12, 2019 •

edited

Loading

TylerLeonhardt commented Oct 12, 2019 •

edited

Loading

vexx32 commented Oct 12, 2019 •

edited

Loading

iSazonov commented Oct 12, 2019 •

edited

Loading