rust-lang / rustup

The Rust toolchain installer
https://rust-lang.github.io/rustup/
Apache License 2.0
6.17k stars 888 forks source link

Use ShellExecute to open docs on windows #499

Closed mattico closed 7 years ago

mattico commented 8 years ago

rustup 0.1.12 (c6e430a 2016-05-12) Windows 10 x64 Insider Preview build 14342

Running any of the doc subcommands opens a new command prompt, but does not display the docs. I've tested this from cmd.exe, powershell.exe, and the MSYS2 shell.

Let me know if you need any other info to help debug this issue.

Edit: I've tried running the command as found in https://github.com/rust-lang-nursery/rustup.rs/blob/master/src/rustup-utils/src/raw.rs#L542, which works fine.

cmd /C start https://github.com

peschkaj commented 8 years ago

Interesting. On Windows 10 (10.0.10586), I see different behavior.

With my default set to either nightly or stable, rustup doc --book first prompted me which program should be used to open the file and then subsequent runs use the browser I chose.

What do you have set in "Default Apps by Protocol"? (Settings -> System -> Default Apps -> Choose default apps by protocol)

mattico commented 8 years ago

@peschkaj I'd assume this would only happen if you don't have a default browser set? Otherwise that is strange. If you run, for example, start chrome https://github.com or start iexplore https://github.com you can specify the browser, FWIW.

peschkaj commented 8 years ago

I would've assumed that I was being prompted because I didn't have a default browser set, too, but this has been my daily driver system since October of last year, so I'm really confused why this particular action bothered to prompt me. :confused:

I wonder if you're running into something blocking you on from being on the Insider Preview.

peschkaj commented 8 years ago

I couldn't duplicate this on 10.0.14295, but I'm waiting for the VM to notice that it's on the fast ring.

luser commented 8 years ago

I just hit this, also running a Windows 10 insider build (14342). I wouldn't be surprised if this is a Windows bug. If I open cmd.exe:

mattico commented 8 years ago

@luser Interesting. I've submitted it to the insider hub, lets hope they actually see it.

feedback-hub:?contextid=268&feedbackid=b2091da1-3e92-4c8e-bd5c-fadae6f29412&form=1&src=2

Edit: Interestingly, that feedback-hub link also does not work with the start command.

brson commented 8 years ago

There must be a smarter way to open the web browser than running cmd.exe.

luser commented 8 years ago

I believe start is just calling ShellExecute under the hood, like: https://support.microsoft.com/en-us/kb/224816

Diggsey commented 7 years ago

Let's re-purpose this issue for the ShellExecute feature. I'm not sure if it will fix the problem, but that seems to be a windows bug, and likely specific to the insider preview.

Eh2406 commented 7 years ago

I don't see ShellExecute in the winapi 0.2. I don't think @retep998 is taking pr to the 0.2 version any more and It is going to be some time before the 0.3 branch is ready let alone before we switch to it. I'd love to be wrong on any of those points. :-)

mattico commented 7 years ago

This is no longer an issue in W10 build 15063 AKA Creator's Update. I assume that this isn't specific to insider builds (considering that 15063 was an insider build before general availability). It might still be a good idea to use ShellExecute.

We don't necessarily need winapi to use ShellExecute. It's a pretty simple API. We'd just need one extern function, one constant, and make sure that the types of LPCSTR, etc. are correct.

~Edit: hm. Unicode doc paths are extremely uncommon, I assume, but this is Rust and we must do the Right Thing:tm:. To support unicode we'd need to use ShellExecuteExW, which is a bit more complicated.~ Nevermind, just use ShellExecuteW.

Eh2406 commented 7 years ago

That is a great point. This code worked in one of my projects.

    use winapi;
    extern "system" {
        pub fn ShellExecuteW(
            hwnd: winapi::HWND,
            lpOperation: winapi::LPCWSTR,
            lpFile: winapi::LPCWSTR,
            lpParameters: winapi::LPCWSTR,
            lpDirectory: winapi::LPCWSTR,
            nShowCmd: winapi::c_int,
        ) -> winapi::HINSTANCE;
    }

oops, did not see your edit.