Skip to content
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

Merged
merged 4 commits into from
Apr 11, 2024
Merged

Allow launched process to spawn Consoles #1205

merged 4 commits into from
Apr 11, 2024

Conversation

Al12rs
Copy link
Contributor

@Al12rs Al12rs commented Apr 11, 2024

Details:

  • Used Process directly instead of CliWrap since CliWrap didn't expose any options to support this behavior

Open to suggestions if there are better alternatives for awaiting the Process instead of process.Exited event

…cepting into our own console.

- Used Process directly instead of CliWrap since it wasn't possible through CLIWrap
@Al12rs Al12rs requested a review from a team April 11, 2024 09:44
@Al12rs Al12rs self-assigned this Apr 11, 2024
@erri120 erri120 self-requested a review April 11, 2024 09:51
Copy link
Member

@erri120 erri120 left a 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.

@Al12rs
Copy link
Contributor Author

Al12rs commented Apr 11, 2024

@erri120 Something like this? Do you want more stuff to be moved to to ProcessFactory?
Do you want me to do anything for FakeProcessFactory?

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 67 lines in your changes are missing coverage. Please review.

Project coverage is 51.42%. Comparing base (72375fb) to head (48df418).
Report is 45 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
Linux 50.81% <0.00%> (-1.64%) ⬇️
Windows 50.72% <0.00%> (-1.62%) ⬇️
clean_environment_tests 51.41% <0.00%> (-1.65%) ⬇️
macOS 50.31% <0.00%> (-1.59%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...usMods.CrossPlatform/Process/FakeProcessFactory.cs 71.05% <0.00%> (-3.95%) ⬇️
...mes.StardewValley/RunGameTools/SmapiRunGameTool.cs 0.00% <0.00%> (ø)
.../NexusMods.CrossPlatform/Process/ProcessFactory.cs 16.00% <0.00%> (-34.00%) ⬇️
...ctions/NexusMods.Abstractions.Games/RunGameTool.cs 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

@Al12rs Al12rs requested a review from erri120 April 11, 2024 12:09
@erri120 erri120 merged commit bc426f5 into main Apr 11, 2024
6 checks passed
@erri120 erri120 deleted the smapi_console_fix branch April 11, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants