Clean up console code #4995

SteveL-MSFT · 2017-10-03T19:22:09Z

These are features we haven't been supporting already (for PowerShell Core) and no plans to support as they exist for legacy reasons only for Windows PowerShell:

removed sta/mta switches and code
removed psconsolefile switch and code
removed importsystemmodules switch and code
removed font changing code
merged #if CORECLR code

Fix #4987
Fix #4350

iSazonov · 2017-10-04T04:45:56Z

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

Do we use first parameter? Can we remove it?

The first param is RunspaceConfiguration. It's being removed as part of #4942

Thanks!

Closed.

PaulHigin · 2017-10-05T20:40:19Z

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

Since this method now only returns OEM_CHARSET, I think we should remove it and just set this value in the one place it is called (and add a comment as to why we use a fixed char set).

private static bool IsAvailableFarEastCodePage(uint codePage) { const uint OEM_CHARSET = 255; return IsAnyDBCSCharSet(OEM_CHARSET); }

Makes sense, will change

PaulHigin · 2017-10-05T21:01:44Z

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

It seems like we should also remove NativeMethods.GetStdHandle() declaration since it is only used here.

Will remove

You should verify the hang doesn't repro in Core. It looks like I did not add a test (probably because the simplest repro required installing Ruby on Windows), but with all the changes in the CLR, it seems prudent to verify we still don't need this code before removing.

In reply to: 143062595 [](ancestors = 143062595)

This code wasn't being used since it's in the #else statement.

@SteveL-MSFT - I know. That doesn't change my feedback. The code was originally added for Full CLR, but with the significant work that went into Core CLR 2.0, I think we need to revisit the issue in case the code is needed in Core now.

If you remove this code, you'll lose the context, and if it's still an issue, investigation will be much more difficult because nobody will remember the change I made that introduced the problem in the first place.

PaulHigin · 2017-10-05T21:12:43Z

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

Since we now always prompt using console we can get rid of this method since it will never be used.

PromptForCredential() can be simplified since it now always prompts from console. Also we can remove the HostUtilities.CredUIPromptForCredential() method since it also will no longer be used.

Agree, will remove

PaulHigin · 2017-10-05T21:15:13Z

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

We can remove this method and simply the conditional code in the two places it is called.

Will change

PaulHigin · 2017-10-05T21:18:50Z

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

We should remove all unused NativeMethods declarations (TranslateCharsetInfo, GetConsoleWindow, etc.).

Will remove

PaulHigin

LGTM

iSazonov · 2017-10-07T07:01:03Z

@SteveL-MSFT Is the PR ready to merge?

SteveL-MSFT · 2017-10-07T14:26:17Z

@iSazonov since this is a breaking change (from Windows PowerShell), let me get an ack from @PowerShell/powershell-committee

lzybkr · 2017-10-09T22:08:26Z

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

As I understand this code - somebody is screen scraping, there is a wide character (2 cells), and the font in use can't render the character, so conhost used 1 cell.
This seems like an extreme corner case, but the code seems correct to me and I don't any value in removing the code.
Also - the PR description should be improved - there is no link to an issue providing more detail.

I'll update the PR description as I did not originally intend for this PR to be so extensive. I simply started down the path of removing the -mta/-sta code since we weren't using it and then noticed the ConsoleFile code which wasn't being used, then the Font code... ugh...

lzybkr · 2017-10-09T22:13:55Z

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

I think we still need LengthInBufferCells - but if you want to make a simplifying assumption when you can't know the font, you can use this code from PSReadline.

I'll just use the code from PSReadline. I ended up removing all this code as the fundamental assumption in the code is that we are using a specific codePage so all the calculations ended up returning a width of 1 anyways as all the other supporting code only applied to Windows PowerShell.

lzybkr · 2017-10-09T22:21:02Z

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

Is this removal intentional? I think if that's not set, we create a new thread every time you execute a new command line.

Will put back the ReuseThread line

@lzybkr Actually, this line was only within the staMode which was not enabled for the user to set. So currently, we use the default which is to use a new thread for each pipeline. Do we want to change this behavior?

staMode = true was the default even if the user couldn't set it, right?
At any rate, I do think we want to reuse the thread.

No, we defaulted to MTA in PSCore

I see. We defaulted to STA on Windows for many years, so reusing the thread seems preferable.

@SteveL-MSFT - are you putting it back?

I need to verify something on Windows (was using a Mac for the changes) before I put it back. With my other uber PR to remove apartmentstate all-up, the WSMan provider didn't work correctly.

Looks good on my Windows box, will run through the tests and push the change.

lzybkr · 2017-10-09T22:28:23Z

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

This is wrong, plain and simple.

lzybkr · 2017-10-09T22:30:24Z

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

Deleting this code makes the comment above very confusing.
It also implies you are introducing a potential security risk.

Will remove the comment since it only applied to the GUI prompt.

lzybkr

There are several distinct changes in this PR, some of which don't feel like "clean up".

I think the commits should be cleaned up with better messages (reset and git add -p) and when merging, we should not squash this PR.

SteveL-MSFT · 2017-10-10T00:37:39Z

@lzybkr cleaned up the commits to be distinct groups of work

lzybkr · 2017-10-10T00:48:01Z

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

If I understand correctly - we default to STA and remove the option to use MTA, correct?

Assuming so, this line should probably stay, but I don't quite understand why it's here.

No, we defaulted to MTA and removing option to use STA

lzybkr · 2017-10-10T00:49:51Z

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

This condition only applies to PSReadline.

will remove

SteveL-MSFT · 2017-10-11T20:55:09Z

@lzybkr I believe your feedback has been addressed, can you take another look? Thanks

SteveL-MSFT · 2017-10-12T00:45:41Z

@PowerShell/powershell-committee discussed this and was fine with all the changes except for removing the STA code. Resolution is to leave the STA code #ifdef'd out in case we have valid use cases on Windows on the future requiring support.

removed -importsystemmodules switch from powershell.exe and related code removed -consolefile parameter from powershell.exe and related code

…ndows PowerShell replaced LengthInBufferCells() method with Unicode adapted code from PSReadline

removed code to show a GUI prompt for credentials as PSCore6 prompts in console

SteveL-MSFT · 2017-10-13T18:31:48Z

I believe all feedback has been addressed. @lzybkr can you take another look when you get a chance? Thanks.

lzybkr · 2017-10-16T23:31:37Z

This code is not called anymore, and is a slight variant of the version you added from PSReadLine below (which my version should be fine, it's more recent and has better comments.) :)

Refers to: src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleControl.cs:2736 in 6ca5d51. [](commit_id = 6ca5d51, deletion_comment = False)

SteveL-MSFT · 2017-10-16T23:56:25Z

@lzybkr removed that method

iSazonov · 2017-10-17T03:48:24Z

@SteveL-MSFT Is the PR ready to merge?

SteveL-MSFT · 2017-10-17T05:18:45Z

@iSazonov yes, please!

iSazonov · 2017-10-17T05:22:02Z

It seems here we should preserve commits by create a merge commit?

SteveL-MSFT · 2017-10-17T05:26:54Z

@iSazonov yes, I believe that is the case

SteveL-MSFT assigned iSazonov Oct 3, 2017

SteveL-MSFT requested review from iSazonov and lzybkr October 3, 2017 19:22

SteveL-MSFT requested review from BrucePay, anmenaga, daxian-dbw and vors as code owners October 3, 2017 19:22

SteveL-MSFT force-pushed the remove-apartmentstate branch from 9bc0001 to 183a164 Compare October 3, 2017 22:16

iSazonov reviewed Oct 4, 2017

View reviewed changes

PaulHigin suggested changes Oct 5, 2017

View reviewed changes

PaulHigin reviewed Oct 5, 2017

View reviewed changes

SteveL-MSFT force-pushed the remove-apartmentstate branch from e7369ed to 99559fe Compare October 6, 2017 20:14

PaulHigin approved these changes Oct 6, 2017

View reviewed changes

SteveL-MSFT added Breaking-Change breaking change that may affect users Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 7, 2017

lzybkr reviewed Oct 9, 2017

View reviewed changes

lzybkr suggested changes Oct 9, 2017

View reviewed changes

SteveL-MSFT force-pushed the remove-apartmentstate branch from ca8d4a8 to 940d58d Compare October 10, 2017 00:36

lzybkr reviewed Oct 10, 2017

View reviewed changes

SteveL-MSFT mentioned this pull request Oct 11, 2017

native commands doesn't return to prompt immediately sometimes #3941

Closed

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 12, 2017

SteveL-MSFT added 4 commits October 12, 2017 10:14

wrapped STA code sections in #if STAMODE for potential future use

856c2af

removed -importsystemmodules switch from powershell.exe and related code removed -consolefile parameter from powershell.exe and related code

removed font and codepage handling code that is only applicable to Wi…

46cdd7e

…ndows PowerShell replaced LengthInBufferCells() method with Unicode adapted code from PSReadline

removed #if CORECLR statements merging code base

5bd8dd3

[feature]

6ca5d51

removed code to show a GUI prompt for credentials as PSCore6 prompts in console

SteveL-MSFT force-pushed the remove-apartmentstate branch from 7652075 to 6ca5d51 Compare October 12, 2017 17:19

Remove unncessary method

173380b

lzybkr approved these changes Oct 17, 2017

View reviewed changes

iSazonov merged commit f604b0e into PowerShell:master Oct 17, 2017

SteveL-MSFT deleted the remove-apartmentstate branch November 12, 2017 05:17

iSazonov mentioned this pull request Jul 3, 2018

add back options for STA #7216

Closed

daxian-dbw mentioned this pull request Nov 4, 2019

Remove the unused setting key ConsolePrompting and avoid unnecessary string creation when querying ExecutionPolicy setting #10985

Merged

14 tasks

Clean up console code #4995

Clean up console code #4995

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Oct 7, 2017

Uh oh!

SteveL-MSFT commented Oct 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Oct 3, 2017 •

edited

Loading

SteveL-MSFT Oct 10, 2017 •

edited

Loading

SteveL-MSFT commented Oct 12, 2017 •

edited

Loading