Clean up ShellExecute code to use the .NET Core implementation by daxian-dbw · Pull Request #4523 · PowerShell/PowerShell · GitHub

This PR includes following major changes:

ProcessStartInfo.UseShellExecute is now supported in .NET Core on Windows. So we need to remove our implementation of ShellExecute and use the .NET Core API directly.
Enable NativeCommandProcessor to open files using default program associated with the shell.
Fix NativeCommandProcessor to properly clean up in case an exception is thrown in Prepare and ProcessRecord.

Note that, UseShellExecute works as full .NET on windows desktop, but it doesn't work properly on Unix and the improvement work is tracked by dotnet/corefx#19956. So on Linux/OSX, we still need to rely on xdg-open/open

…rocessor

… is thron

Maybe cache the value in a variable instead of looking it up twice in the same method.

Updated the code to cache the value in Platform.IsWindowsDesktop.

Dongbo Wang (@daxian-dbw) This does not seem to be fixed.

The value is cached in a static field in Platform.IsWindowsDesktop property.

Can we use Platform.IsWindowsDesktop here?

Sure. Will update the code.

We can use Platform.IsWindowsDesktop if it is made public.

Good point. Will update.

Use ShouldBeErroId. Example :

PowerShell/test/powershell/engine/Attributes.Tests.ps1

Line 22 in 0cd7a4a

{ & $testdrive/test.ps1 } | ShouldBeErrorId "Argument"

Fixed.

Should we consider making this public? This will be very useful in tests.

Done.

Polling for a file can be a common pattern. Consider implementing a generic file watcher function in HelpersCommon.psm1

Sounds reasonable. Opened #4524. Will address in a separate PR.

Maybe use quotas - "notepad"?

OK, will update.

Should we move this in AfterAll?

I think it should stay here, as it's specific to this test when running on full desktop.

I don't like commands after Should 😄

There are 2 failures in AppVeyor feature test run and both are related to Invoke-RestMethod, not related to the changes made in this PR. So the feature test run is good.

Aditya Patwardhan (@adityapatwardhan) Ilya (@iSazonov) Your comments have been addressed. Please take another look. Thanks!

Dongbo Wang (@daxian-dbw) This does not seem to be fixed.

LGTM

Since the PR is a cleanup we could add fix - we use "UseShellExecute" constant three times in Process.cs and could move it to private static variable.

It is obvious comment. Please add in the comment why we do this.

Fixed. More comment added to Cleanup() to explain what we are cleaning up.

Closed.

Again obvious comment. Please remove or enhance.

Fixed.

Closed.

Formatting - Can we use the one line pattern for if?

Yes, as long as the if block is a single simple statement like this one.

Closed.

We use "notepad" in three lines (66, 68, 69 and maybe 65) so maybe use a variable?

Fixed.

Closed.

The "Default" constant string is used in 9 places and should be cleaned too (change to const variable, instead of static). But I think they should be in separate PRs as they will introduce too many diffs and make it hard to see the focus of this PR.

LGTM

LGTM.

Ilya (@iSazonov) Aditya Patwardhan (@adityapatwardhan) Thanks for the review! Aditya, can you please merge this PR? My follow-up work on the file watcher test helper needs to update the tests in this PR (#4524).

…Shell#4523) * Clean up ShellExecuteHelper and enable ShellExecute in NativeCommandProcessor * Minor fix * [Feature] Fix NativeCommandProcessor to clean up in case an exception is thron * [Feature] Update tests * [Feature] Address comments * address one more comment * Address some more comments

Dongbo Wang (daxian-dbw) added 3 commits August 4, 2017 21:21

Clean up ShellExecuteHelper and enable ShellExecute in NativeCommandP…

82636e3

…rocessor

Minor fix

6ac56ab

[Feature] Fix NativeCommandProcessor to clean up in case an exception…

f99c35a

… is thron

Dongbo Wang (daxian-dbw) assigned Aditya Patwardhan (adityapatwardhan) Aug 7, 2017

Dongbo Wang (daxian-dbw) requested review from Steve Lee (SteveL-MSFT) and Aditya Patwardhan (adityapatwardhan) August 7, 2017 23:25

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Aug 7, 2017

Aditya Patwardhan (adityapatwardhan) requested changes Aug 8, 2017

View reviewed changes

[Feature] Update tests

7d375eb

Dongbo Wang (daxian-dbw) force-pushed the shellexecute branch from 1ffa36b to 7d375eb Compare August 8, 2017 00:43

Dongbo Wang (daxian-dbw) mentioned this pull request Aug 8, 2017

Add a generic file watcher function in HelpersCommon.psm1 #4524

Closed

[Feature] Address comments

5494c7f

Ilya (iSazonov) reviewed Aug 8, 2017

View reviewed changes

address one more comment

16f459e

Aditya Patwardhan (adityapatwardhan) requested changes Aug 8, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) approved these changes Aug 8, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) changed the title Shellexecute Clean up ShellExecute code to use the .NET Core implementation Aug 8, 2017

Ilya (iSazonov) reviewed Aug 9, 2017

View reviewed changes

Address some more comments

c7c9394

Aditya Patwardhan (adityapatwardhan) approved these changes Aug 9, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) merged commit 384a9fe into PowerShell:master Aug 10, 2017

Dongbo Wang (daxian-dbw) deleted the shellexecute branch August 10, 2017 20:47

Crystal Durham (CAD97) mentioned this pull request Sep 8, 2022

Nushell no longer respects file associations on Windows nushell/nushell#6440

Closed

               #else
-                                  ShellExecuteHelper.Start(invokeProcess.StartInfo);
+                              // Use ShellExecute when it's not a headless SKU
+                              invokeProcess.StartInfo.UseShellExecute = !Platform.IsNanoServer && !Platform.IsIoT;

+                          & $dllFile
+                          throw "No Exception!"
+                      } catch {
+                          $_.FullyQualifiedErrorId | should be "NativeCommandFailed"

+                          & $TestFile
+                          $startTime = Get-Date
+                          # It may take time for handler to start
+                          while (((Get-Date) - $startTime).TotalSeconds -lt 10 -and (-not (Test-Path "$HOME/nativeCommandProcessor.Success"))) {

+                              Get-Process -Name notepad | Stop-Process -Force
+                              Invoke-Item -Path $notepad
+                              $notepadProcess = Get-Process -Name notepad
+                              $notepadProcess.Name | Should Be notepad

+                              Invoke-Item -Path $notepad
+                              $notepadProcess = Get-Process -Name notepad
+                              $notepadProcess.Name | Should Be notepad
+                              Stop-Process -InputObject $notepadProcess

                           // -Verb and -WindowStyle are not supported on non-Windows platforms as well as Windows headless SKUs
-                          if (_isOnFullWinSku)
+                          if (Platform.IsWindowsDesktop)

+                              xdg-mime default nativeCommandProcessor.desktop text/plain
+                          }
+                      }
+                      elseif ($IsWindows) {

                           }
                       }
+                      internal static bool IsWindowsDesktop

+                          }
+                          catch (Exception)
+                          {
+                              // Do the cleanup and rethrow the exception

-                                  this.Command.Context.EngineHostInterface.NotifyEndApplication();
-                              }
-                              // Do the clean up...
+                              // Do the cleanup...

-              #else
-                          if (Platform.IsNanoServer)
-                              return false;
+                          if (!Platform.IsWindowsDesktop) { return false; }

Clean up ShellExecute code to use the .NET Core implementation by daxian-dbw · Pull Request #4523 · PowerShell/PowerShell · GitHub

Uh oh!

Clean up ShellExecute code to use the .NET Core implementation - #4523

Clean up ShellExecute code to use the .NET Core implementation#4523Aditya Patwardhan (adityapatwardhan) merged 7 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom daxian-dbw:shellexecutedaxian-dbw/PowerShell:shellexecuteCopy head branch name to clipboard

Conversation

Dongbo Wang (daxian-dbw) commented Aug 7, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) commented Aug 8, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aditya Patwardhan (adityapatwardhan) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) Aug 9, 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!

Clean up ShellExecute code to use the .NET Core implementation#4523
Aditya Patwardhan (adityapatwardhan) merged 7 commits into
PowerShell:masterfrom
daxian-dbw:shellexecute

Dongbo Wang (daxian-dbw) commented Aug 7, 2017 •
edited
Loading

Dongbo Wang (daxian-dbw) Aug 9, 2017 •
edited
Loading