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

Several Improvements (well... see description) #25

Closed
hybras opened this issue Feb 19, 2021 · 5 comments
Closed

Several Improvements (well... see description) #25

hybras opened this issue Feb 19, 2021 · 5 comments

Comments

@hybras
Copy link
Contributor

hybras commented Feb 19, 2021

I forked your project and made a bunch of changes. So far I've

  • created os specific modules
  • changed how cfg(target_os) was being used
  • Made the unix variant use which to check for the different open handlers.

I also plan to change the windows variant to call to explorer.exe using Command instead of winapi::um::shellapi::ShellExecuteW. This removes unsafe from windows::that and drastically simplifies the method.

I would like your feedback on my changes.

Whether you accept them or not, I love open! Previously I was using a shell function, which was very hacky not always portable.

@hoodie
Copy link
Contributor

hoodie commented Feb 19, 2021

question: isn't xdg-open always available?

@hybras
Copy link
Contributor Author

hybras commented Feb 20, 2021

question: isn't xdg-open always available?

You... are probably right. But by that logic, why are we checking for alternatives at all? Do u want me to special case xdg-open?

EDIT: On my wsl ubuntu install, xdg-utils is not installed by default.

@Byron
Copy link
Owner

Byron commented Feb 20, 2021

Will have a closer look later. In the meantime: Maybe also consult the ‘opener’ crate for reference.

@Byron
Copy link
Owner

Byron commented Feb 21, 2021

Would you be OK to send a draft PR so I can have a look and potentially leave some comments?

@hybras
Copy link
Contributor Author

hybras commented Feb 23, 2021

Yes I'll do that. At time of writing i still haven't finished the last point I mentioned

@hybras hybras closed this as completed Mar 1, 2021
matoous added a commit to matoous/open-rs that referenced this issue Feb 5, 2023
Use std `Command` instead of `ShellExecuteW` from windows sys crate.

This change was already attempted in: Byron#25
and later reverted in: Byron#27
and it it seems that it didn't work due to incorrect usage of
`explorer` instead of `cmd /c start`.
(see helix-editor/helix#5820 (comment)
for detailed explanation).

Related: helix-editor/helix#5820
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

No branches or pull requests

3 participants