Skip to content

Support WindowStyle without UseShellExecute #82662

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 8, 2023
Merged

Support WindowStyle without UseShellExecute #82662

merged 6 commits into from
Mar 8, 2023

Conversation

@jborean93
Copy link
Contributor

@jborean93 jborean93 commented Feb 25, 2023

Supports specifying a custom ProcessStartInfo.WindowStyle without having to use UseShellExecute.

Fix #81681

@jborean93
Support WindowStyle without UseShellExecute
b06d7a6 
Supports specifying a custom ProcessStartInfo.WindowStyle without having
to use UseShellExecute.

Fix dotnet#81681
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 25, 2023
@jborean93

This comment was marked as resolved.

Sign in to view
@jborean93 jborean93 mentioned this pull request Feb 25, 2023
Closed
@build-analysis build-analysis bot mentioned this pull request Feb 25, 2023
Closed
@ghost
Copy link

ghost commented Feb 26, 2023

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

Issue Details

Supports specifying a custom ProcessStartInfo.WindowStyle without having to use UseShellExecute.

Fix #81681

Author: jborean93
Assignees: -
Labels:

area-System.Diagnostics.Process, community-contribution
Milestone: -

@jborean93 jborean93 marked this pull request as ready for review March 2, 2023 06:10
This was referenced Mar 2, 2023
Closed
Open
Closed
@runfoapp runfoapp bot mentioned this pull request Mar 2, 2023
Closed
jozkee
jozkee reviewed Mar 3, 2023
View reviewed changes
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs
startupInfo.dwFlags = Interop.Advapi32.StartupInfoOptions.STARTF_USESTDHANDLES;
}

if (startInfo.WindowStyle != ProcessWindowStyle.Normal)
Copy link
Member

@jozkee jozkee Mar 3, 2023

Choose a reason for hiding this comment

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

Is this meant to avoid a breaking change in the default path? Maybe it is a breaking change worth taking.

Copy link
Member

@jozkee jozkee Mar 3, 2023

Choose a reason for hiding this comment

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

Actually, the current behavior looks just like as if we were using ProcessWindowStyle.Normal. So, we can safely map ProcessWindowStyle.Normal => SW_SHOWNORMAL.

Copy link
Contributor Author

@jborean93 jborean93 Mar 4, 2023

Choose a reason for hiding this comment

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

So specifically this is done to preserve the existing behaviour where ProcessWindowStyle.Normal then no wShowWindow value is set alongside the STARTF_USESHOWWINDOW flag that tells Windows to use the value for wShowWindow. I'm not confident that explicitly setting the normal window style is the same as not setting it at all which is what Normal currently means so I kept it out. I can change the code so it always just sets this value alongside STARTF_USESHOWWINDOW if that's your wish but I was aiming for behaviour that was as close to what it was today.

Copy link
Member

@jozkee jozkee Mar 8, 2023

Choose a reason for hiding this comment

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

this is done to preserve the existing behaviour where ProcessWindowStyle.Normal then no wShowWindow value is set alongside the STARTF_USESHOWWINDOW flag.

@iSazonov would your scenario still be covered?

Copy link
Contributor Author

@jborean93 jborean93 Mar 8, 2023

Choose a reason for hiding this comment

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

PowerShell always sets the window so it would technically be a behaviour change on their end. It really depends on whether dotnet wants to be the one to change the behaviour or let PowerShell do it.

It would make sense for dotnet to align to the same behaviour as UseShellExecute = true but I'm just not confident enough to say whether this really matters or not.

Copy link
Member

@jozkee jozkee Mar 8, 2023

Choose a reason for hiding this comment

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

Let's keep it this way to avoid the breaking change. We can change it later based on user feedback ie: if someone needs this flag to be set for a specific use case.

@jborean93
Copy link
Contributor Author

jborean93 commented Mar 4, 2023

@jozkee thanks for the review and suggestions. I've pushed a new commit that implements them. The only thing I held off was the still unresolved comment around always setting wShowWindow and the STARTF_USESHOWWINDOW flag if ProcessWindowStyle.Normal was used. While I don't think it would break anything I'm not confident enough to say it won't and so just preserved the existing behaviour today. I can definitely change it to always set the flag if that's truly what is desired, on the plus side it will align with the ShellExecute behaviour so that could be a positive.

jozkee
jozkee approved these changes Mar 8, 2023
View reviewed changes
Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jborean93.

@jozkee jozkee merged commit 18c6ab6 into dotnet:main Mar 8, 2023
@jborean93 jborean93 deleted the WindowStyle-nonShell branch March 8, 2023 18:39
@jborean93
Copy link
Contributor Author

jborean93 commented Mar 8, 2023

Thanks @jozkee for your help in fixing this.

This was referenced Mar 27, 2023
Closed
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Apr 7, 2023
This was referenced Jun 23, 2023
Closed
Closed
@mxschmitt mxschmitt mentioned this pull request Jun 25, 2023
Merged
@rchauhan18 rchauhan18 mentioned this pull request Jul 28, 2023
Closed
@dotnet dotnet unlocked this conversation Aug 2, 2023
@jeffhandley jeffhandley mentioned this pull request Aug 3, 2023
Closed
@jeffhandley jeffhandley added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Aug 7, 2023
@ghost
Copy link

ghost commented Aug 7, 2023 •
edited by jozkee
Loading

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2023
@ericstj ericstj added this to the 8.0.0 milestone Oct 16, 2023
@jozkee
Copy link
Member

jozkee commented Oct 25, 2023

Breaking change doc created dotnet/docs#37739

@jozkee jozkee removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jozkee jozkee jozkee approved these changes

Assignees

@jozkee jozkee

Labels

area-System.Diagnostics.Process breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Support WindowStyle without UseShellExecute in ProcessStartInfo and Process implementation on Windows

5 participants

@jborean93 @jozkee @vcsjones @jeffhandley @ericstj