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

Fix open::with() on Windows. (#80) #82

Merged
merged 2 commits into from
Jun 25, 2023

Conversation

igdswzcd
Copy link

On Windows OS, my colleagues and I conducted tests and found that:

  • cmd /c app command can't open apps which are not in the env variables.

  • However, cmd /c start app can open all apps exist in registry path HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\App Paths. This is why I added start to open::with() under the Windows branch. And I recommend adding the registry address that represents the available scope to the doc or README (on Mac and Linux it may be other files), so that developers can quickly determine appnames available.
    image

  • Commands like cmd /c "C:\app.exe" can also open the application, but cmd /c start "C:\app.exe" does not work. Finally I found that start "" "C:\app.exe" is effective which "" is said to open a new window, and start "" app also works. It is safe and consistent with the previous changes to open::that(), so I added "" as an arg.

  • To avoid the previous issue, I guess some people might pass start chrome as the second function param. After adding two args, it can still open the app but will also open a new cmd window, should this situation be taken into consideration?

At last, I modified the test code in main.rs. On Windows, you can call open::with() by using open.exe path -w app without specifying an env variable.

ZhuXiaojie and others added 2 commits June 25, 2023 11:02
Technically this is a breaking change as the way it communicates now looks different,
and error handling is different as well.
@Byron Byron merged commit ddf4842 into Byron:main Jun 25, 2023
@Byron
Copy link
Owner

Byron commented Jun 25, 2023

Thanks a lot for looking into this! I truly hope that this will be it now for Windows, and that it doesn't break in subtle, unforeseen ways.

I have created a new breaking release just to be sure this change doesn't catch anyone off-guard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants