-
Notifications
You must be signed in to change notification settings - Fork 43
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.
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
Allow launched process to spawn Consoles #1205
Conversation
…cepting into our own console. - Used Process directly instead of CliWrap since it wasn't possible through CLIWrap
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 don't think running a raw process instead of using CliWrap
for every game is the right choice. This solution will run a shell when you want to launch any game. Instead, RunGameTool
should have a property UseShell
or similar. This way, games can opt into this behavior.
I also believe we can still use CliWrap
and spawn a shell. Instead of launching the program directly, we launch cmd.exe
that then launches the program. This would be similar to how we open URLs on Windows.
If we have to use process, then we should add a new method to IProcessFactory
that takes in an unstarted Process
. The implementation of this method would be similar to the private RunProcessAsync
method from this PR.
@erri120 Something like this? Do you want more stuff to be moved to to ProcessFactory? |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1205 +/- ##
==========================================
- Coverage 53.06% 51.42% -1.65%
==========================================
Files 674 688 +14
Lines 23151 24099 +948
Branches 1764 1859 +95
==========================================
+ Hits 12285 12392 +107
- Misses 10465 11304 +839
- Partials 401 403 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Details:
Process
directly instead ofCliWrap
since CliWrap didn't expose any options to support this behaviorOpen to suggestions if there are better alternatives for awaiting the Process instead of
process.Exited
event