[PT Run] Helper for Execute Shells Process Calls #9538

davidegiacometti · 2021-02-06T20:00:29Z

Summary of the Pull Request

What is this about:
Common helper for execute shell across PT Run plugins.

What is include in the PR:
Removed most of the Process.Start used for invoking shell across PT Run plugins.
Still have 2 hardcorded explorer.exe in folder and indexer plugin used to focus the file when explorer is opened #7339 (comment)

How does someone test / validate:

Folder plugin
- Open file
- Open folder
- Open containing folder
Indexer plugin
- Open containing folder
- Click on index warning
- Open file
Program plugin
- Open containing folder UWP app
- Open containing folder Win32 app
URI plugin
- Open an URI
Registry plugin
- Open registry
Service plugin
- Open services.msc
System plugin
- Shutdown
- Reboot
Report Window
- Open GitHub URL

Quality Checklist

Contributor License Agreement (CLA)

A CLA must be signed. If not, go over here and sign the CLA.

src/modules/launcher/Plugins/Microsoft.Plugin.Program/Programs/Win32Program.cs

enricogior · 2021-02-08T08:55:38Z

src/modules/launcher/Plugins/Microsoft.Plugin.Indexer/Main.cs

-                                        Log.Exception($"Unable to launch Windows Search Settings: {ex.Message}", ex, GetType());
-                                    }
-
+                                    Helper.OpenInShell("ms-settings:cortana-windowssearch");


In some cases the returned value of Helper.OpenInShell is not checked and no error is shown.
I haven't check all the places where this is done, consider verifying why is not done and in case add a comment motivating why the error is not shown to the user.

Some plugins are checking the result and display a MessageBox.
Should we always display it in case of error? We can consider move the MessageBox inside Helper.OpenInShell.

enricogior · 2021-02-08T08:58:41Z

@davidegiacometti
thank you for the nice code clean up!

htcfreek · 2021-02-08T10:43:58Z

@davidegiacometti
Is it possible to use third party explorer apps with this helper?
I know we have/had an issue for this and possibly merged a PR to address this in the past.

src/modules/launcher/Plugins/Microsoft.Plugin.Folder/Sources/ShellAction.cs

src/modules/launcher/Wox.Infrastructure/Helper.cs

davidegiacometti · 2021-02-08T18:36:32Z

@davidegiacometti
Is it possible to use third party explorer apps with this helper?
I know we have/had an issue for this and possibly merged a PR to address this in the past.

Yes, It was already possible but in Program and Indexer plugin are invoking explorer.exe /select for "open containing folder". #7339 (comment)

mykhailopylyp · 2021-02-09T10:49:55Z

...Plugins/Microsoft.PowerToys.Run.Plugin.Service/Microsoft.PowerToys.Run.Plugin.Service.csproj

@@ -71,6 +71,7 @@

  <ItemGroup>
    <ProjectReference Include="..\..\Wox.Plugin\Wox.Plugin.csproj" />
+    <ProjectReference Include="..\..\Wox.Infrastructure\Wox.Infrastructure.csproj" />


Something to ponder about. It feels like plugins should be independent and reference to Wox.Infrastructure violates it. It is okay to have reference to a project that contains plugin interface. What do you think? Does it make sense?

Oh.. I missed this! Almost all plugin are referencing Wox.Infrastructure. Consider also that ISavable and PluginJsonStorage are in Wox.Infrastructure. Could be a refactoring. Should I open an issue?

We can open a separate issue for that and discuss the pros and cons there. It is definitely a low-priority one.

mykhailopylyp

LGTM!

davidegiacometti · 2021-02-15T19:20:02Z

List of places where result isn't checked and user isn't notified in case of error:

Indexer: open ms-settings:cortana-windowssearch
Registry: open regedit.exe
Services: open services.msc
System: open shudown
Report Window: open GitHub
URI: open URI
Program: open folder

@enricogior any thoughts on this? I think that URI and Program should display a message in case of error.
We can consider moving the MessageBox in Helper.OpenInShell and always display the error.

htcfreek · 2021-02-15T19:24:18Z

List of places where result isn't checked and user isn't notified in case of error:

Indexer: open ms-settings:cortana-windowssearch

Registry: open regedit.exe

Services: open services.msc

System: open shudown

Report Window: open GitHub

URI: open URI

Program: open folder

@enricogior any thoughts on this? I think that URI and Program should display a message in case of error.
We can consider moving the MessageBox in Helper.OpenInShell and always display the error.

What about creating a unified ui (I suggest toast notification) for error/additional info messages by creating a new common helper class for info and error messages?

Possible messages:

Error on opening uri in browser.
Error on stopping service "name"!
The service "name" has started successful.

enricogior · 2021-02-15T19:36:12Z

@davidegiacometti @htcfreek
we should show the error in the least intrusive way and not a toast notifications (we had so many problems with toast notifications that at this point we should avoid them all together). If we can show the error before closing the search box, we may consider showing the error as we do for the warning that tells the user that not all drives are indexed. Otherwise we show an error dialog.

htcfreek · 2021-02-15T19:46:52Z

@davidegiacometti @htcfreek
we should show the error in the least intrusive way and not a toast notifications (we had so many problems with toast notifications that at this point we should avoid them all together). If we can show the error before closing the search box, we may consider showing the error as we do for the warning that tells the user that not all drives are indexed. Otherwise we show an error dialog.

Then we should fix the bug that warning messages like DriveIndexWarning aren't shown as first result. (Don't know the issue at the moment.)

davidegiacometti · 2021-02-15T20:07:27Z

We should be fine with dialogs at the moment.
I think shutdown, regedit or services.msc not working is remote but possible with a messed path or Windows installation.
Should I move the error dialog inside Helper.OpenInShell?

enricogior · 2021-02-17T09:28:00Z

@davidegiacometti

Should I move the error dialog inside Helper.OpenInShell?

I let you make the call since you have a better understanding of the code.
Beside that, is there anything else that still need to be done for this PR or can we proceed with the final review?

davidegiacometti · 2021-02-17T19:48:13Z

@enricogior PR is ready for review.
I have added error handling for URI plugin since user can search for any URL.
Should't be a problem for other plugins since it hasn't been reported yet ans since result wasn't checked before this refactoring.

enricogior requested a review from mykhailopylyp February 8, 2021 08:42

enricogior reviewed Feb 8, 2021

View reviewed changes

src/modules/launcher/Plugins/Microsoft.Plugin.Program/Programs/Win32Program.cs Outdated Show resolved Hide resolved

enricogior reviewed Feb 8, 2021

View reviewed changes

enricogior added the Product-PowerToys Run Improved app launch PT Run (Win+R) Window label Feb 8, 2021

mykhailopylyp reviewed Feb 8, 2021

View reviewed changes

src/modules/launcher/Plugins/Microsoft.Plugin.Folder/Sources/ShellAction.cs Show resolved Hide resolved

mykhailopylyp reviewed Feb 8, 2021

View reviewed changes

src/modules/launcher/Wox.Infrastructure/Helper.cs Outdated Show resolved Hide resolved

mykhailopylyp reviewed Feb 9, 2021

View reviewed changes

mykhailopylyp approved these changes Feb 9, 2021

View reviewed changes

common helper for execute shell process from run plugins

650ce11

davidegiacometti force-pushed the users/davidegiacometti/issue-7339 branch from a695a9a to 650ce11 Compare February 17, 2021 19:44

davidegiacometti mentioned this pull request Feb 17, 2021

PT Run Plugin Referencing Wox.Infrastructure #9774

Open

enricogior approved these changes Feb 23, 2021

View reviewed changes

enricogior merged commit 571bceb into master Feb 23, 2021

enricogior deleted the users/davidegiacometti/issue-7339 branch February 23, 2021 08:53

enricogior mentioned this pull request Feb 23, 2021

[Run][File Plugin] Open in File Explorer does not honor Explorer replacements #4622

Closed

This was referenced Feb 23, 2021

[PT Run] Files not processed by URI plugin #8931

Closed

[PT Run] explorer.exe instead of execute shell #9865

Open

davidegiacometti mentioned this pull request Mar 2, 2021

[Run] URL plugin is false triggering with \\string #8364

Closed

This was referenced Mar 11, 2021

[Run] symbol “？” bug #9013

Closed

[Run] Exception shown when entering a file/folder path that doesn't exist #8891

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PT Run] Helper for Execute Shells Process Calls #9538

[PT Run] Helper for Execute Shells Process Calls #9538

davidegiacometti commented Feb 6, 2021

enricogior Feb 8, 2021

davidegiacometti Feb 8, 2021

enricogior commented Feb 8, 2021

htcfreek commented Feb 8, 2021

davidegiacometti commented Feb 8, 2021

mykhailopylyp Feb 9, 2021

davidegiacometti Feb 15, 2021

mykhailopylyp Feb 16, 2021

mykhailopylyp left a comment

davidegiacometti commented Feb 15, 2021

htcfreek commented Feb 15, 2021 •

edited

enricogior commented Feb 15, 2021

htcfreek commented Feb 15, 2021

davidegiacometti commented Feb 15, 2021

enricogior commented Feb 17, 2021

davidegiacometti commented Feb 17, 2021

[PT Run] Helper for Execute Shells Process Calls #9538

[PT Run] Helper for Execute Shells Process Calls #9538

Conversation

davidegiacometti commented Feb 6, 2021

Summary of the Pull Request

Quality Checklist

Contributor License Agreement (CLA)

enricogior Feb 8, 2021

Choose a reason for hiding this comment

davidegiacometti Feb 8, 2021

Choose a reason for hiding this comment

enricogior commented Feb 8, 2021

htcfreek commented Feb 8, 2021

davidegiacometti commented Feb 8, 2021

mykhailopylyp Feb 9, 2021

Choose a reason for hiding this comment

davidegiacometti Feb 15, 2021

Choose a reason for hiding this comment

mykhailopylyp Feb 16, 2021

Choose a reason for hiding this comment

mykhailopylyp left a comment

Choose a reason for hiding this comment

davidegiacometti commented Feb 15, 2021

htcfreek commented Feb 15, 2021 • edited

enricogior commented Feb 15, 2021

htcfreek commented Feb 15, 2021

davidegiacometti commented Feb 15, 2021

enricogior commented Feb 17, 2021

davidegiacometti commented Feb 17, 2021

htcfreek commented Feb 15, 2021 •

edited