GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Clean up console code by SteveL-MSFT · Pull Request #4995 · PowerShell/PowerShell

Conversation

@SteveL-MSFT

@SteveL-MSFT Steve Lee (SteveL-MSFT) commented Oct 3, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT Steve Lee (SteveL-MSFT) assigned Ilya (iSazonov) Oct 3, 2017
@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested review from Ilya (iSazonov) and Jason Shirk (lzybkr) October 3, 2017 19:22
@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested review from Bruce Payette (BrucePay), Andrew (anmenaga), Dongbo Wang (daxian-dbw) and sergei (vors) as code owners October 3, 2017 19:22
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the remove-apartmentstate branch from 9bc0001 to 183a164 Compare October 3, 2017 22:16
iSazonov
Ilya (iSazonov) reviewed Oct 4, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleShell.cs Outdated

@iSazonov Ilya (iSazonov) Oct 4, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Do we use first parameter? Can we remove it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 4, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Oct 4, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks!

Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
PaulHigin
Paul Higinbotham (PaulHigin) suggested changes Oct 5, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleControl.cs Outdated

@PaulHigin Paul Higinbotham (PaulHigin) Oct 5, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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);
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 5, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Makes sense, will change

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs Outdated

@PaulHigin Paul Higinbotham (PaulHigin) Oct 5, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 5, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will remove

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lzybkr Jason Shirk (lzybkr) Oct 9, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 10, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lzybkr Jason Shirk (lzybkr) Oct 10, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Steve Lee (@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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceSecurity.cs Outdated

@PaulHigin Paul Higinbotham (PaulHigin) Oct 5, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@PaulHigin Paul Higinbotham (PaulHigin) Oct 5, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 5, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Agree, will remove

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceSecurity.cs Outdated

@PaulHigin Paul Higinbotham (PaulHigin) Oct 5, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 5, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will change

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
PaulHigin
Paul Higinbotham (PaulHigin) reviewed Oct 5, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleControl.cs Outdated

@PaulHigin Paul Higinbotham (PaulHigin) Oct 5, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 5, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will remove

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the remove-apartmentstate branch from e7369ed to 99559fe Compare October 6, 2017 20:14
PaulHigin
Paul Higinbotham (PaulHigin) approved these changes Oct 6, 2017
View reviewed changes

@PaulHigin Paul Higinbotham (PaulHigin) left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov

Ilya (iSazonov) commented Oct 7, 2017

Copy link
Copy Markdown
Collaborator

Steve Lee (@SteveL-MSFT) Is the PR ready to merge?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT Steve Lee (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
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Oct 7, 2017

Copy link
Copy Markdown
Member Author

Ilya (@iSazonov) since this is a breaking change (from Windows PowerShell), let me get an ack from @PowerShell/powershell-committee
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lzybkr
Jason Shirk (lzybkr) reviewed Oct 9, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleControl.cs Outdated

@lzybkr Jason Shirk (lzybkr) Oct 9, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 9, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lzybkr
Jason Shirk (lzybkr) reviewed Oct 9, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleControl.cs Outdated

@lzybkr Jason Shirk (lzybkr) Oct 9, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 9, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lzybkr
Jason Shirk (lzybkr) reviewed Oct 9, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs Outdated

@lzybkr Jason Shirk (lzybkr) Oct 9, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 9, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will put back the ReuseThread line

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 10, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Jason Shirk (@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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lzybkr Jason Shirk (lzybkr) Oct 10, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 10, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

No, we defaulted to MTA in PSCore

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lzybkr Jason Shirk (lzybkr) Oct 10, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lzybkr Jason Shirk (lzybkr) Oct 10, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Steve Lee (@SteveL-MSFT) - are you putting it back?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 10, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 10, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lzybkr
Jason Shirk (lzybkr) reviewed Oct 9, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostRawUserInterface.cs Outdated

@lzybkr Jason Shirk (lzybkr) Oct 9, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is wrong, plain and simple.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 10, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

WIll fix!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lzybkr
Jason Shirk (lzybkr) reviewed Oct 9, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfacePrompt.cs Outdated

@lzybkr Jason Shirk (lzybkr) Oct 9, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 10, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lzybkr
Jason Shirk (lzybkr) suggested changes Oct 9, 2017
View reviewed changes

@lzybkr Jason Shirk (lzybkr) left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the remove-apartmentstate branch from ca8d4a8 to 940d58d Compare October 10, 2017 00:36
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Oct 10, 2017

Copy link
Copy Markdown
Member Author

Jason Shirk (@lzybkr) cleaned up the commits to be distinct groups of work
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lzybkr
Jason Shirk (lzybkr) reviewed Oct 10, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs Outdated

@lzybkr Jason Shirk (lzybkr) Oct 10, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 10, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lzybkr
Jason Shirk (lzybkr) reviewed Oct 10, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleControl.cs Outdated

@lzybkr Jason Shirk (lzybkr) Oct 10, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This condition only applies to PSReadline.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 10, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

will remove

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT Steve Lee (SteveL-MSFT) mentioned this pull request Oct 11, 2017
native commands doesn't return to prompt immediately sometimes #3941
Closed
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Oct 11, 2017

Copy link
Copy Markdown
Member Author

Jason Shirk (@lzybkr) I believe your feedback has been addressed, can you take another look? Thanks
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Oct 12, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT Steve Lee (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
Steve Lee (SteveL-MSFT) added 4 commits October 12, 2017 10:14
@SteveL-MSFT
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
@SteveL-MSFT
removed font and codepage handling code that is only applicable to Wi…
46cdd7e 
…ndows PowerShell

replaced LengthInBufferCells() method with Unicode adapted code from PSReadline
@SteveL-MSFT
removed #if CORECLR statements merging code base
5bd8dd3
@SteveL-MSFT
[feature]
6ca5d51 
removed code to show a GUI prompt for credentials as PSCore6 prompts in console
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the remove-apartmentstate branch from 7652075 to 6ca5d51 Compare October 12, 2017 17:19
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Oct 13, 2017

Copy link
Copy Markdown
Member Author

I believe all feedback has been addressed. Jason Shirk (@lzybkr) can you take another look when you get a chance? Thanks.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lzybkr

Jason Shirk (lzybkr) commented Oct 16, 2017

Copy link
Copy Markdown
Contributor 
    {

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT
Remove unncessary method
173380b
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Oct 16, 2017

Copy link
Copy Markdown
Member Author

Jason Shirk (@lzybkr) removed that method
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lzybkr
Jason Shirk (lzybkr) approved these changes Oct 17, 2017
View reviewed changes
@iSazonov

Ilya (iSazonov) commented Oct 17, 2017

Copy link
Copy Markdown
Collaborator

Steve Lee (@SteveL-MSFT) Is the PR ready to merge?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Oct 17, 2017

Copy link
Copy Markdown
Member Author

Ilya (@iSazonov) yes, please!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Oct 17, 2017

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Oct 17, 2017

Copy link
Copy Markdown
Member Author

Ilya (@iSazonov) yes, I believe that is the case
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Ilya (iSazonov) merged commit f604b0e into PowerShell:master Oct 17, 2017
@SteveL-MSFT
Steve Lee (SteveL-MSFT) deleted the remove-apartmentstate branch November 12, 2017 05:17
@iSazonov Ilya (iSazonov) mentioned this pull request Jul 3, 2018
add back options for STA #7216
Closed
@daxian-dbw Dongbo Wang (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
8 of 14 tasks
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@iSazonov
Merge pull request PowerShell#4995 from SteveL-MSFT/remove-apartments…
15b048f 
…tate

* wrapped STA code sections in `#if STAMODE` for potential future use
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 Windows PowerShell
replaced LengthInBufferCells() method with Unicode adapted code from PSReadline

* removed `#if CORECLR` statements merging code base

* [feature]
removed code to show a GUI prompt for credentials as PSCore6 prompts in console

* Remove unncessary method 'LengthInBufferCellsFE'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@anmenaga Andrew (anmenaga) Awaiting requested review from anmenaga
@BrucePay Bruce Payette (BrucePay) Awaiting requested review from BrucePay
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw
@vors sergei (vors) Awaiting requested review from vors
+2 more reviewers
@lzybkr Jason Shirk (lzybkr) lzybkr approved these changes
@PaulHigin Paul Higinbotham (PaulHigin) PaulHigin approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@iSazonov Ilya (iSazonov)

Labels

Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@SteveL-MSFT @iSazonov @lzybkr @PaulHigin
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.