Make opening the settings file more robust #1841

maru-sama · 2019-07-05T18:03:11Z

This fixes two issues.

Opens the assigned default application regardless of its configuration.
Gvim for example only reacts to the "edit" verb so when selected as default application won't open.
Using nullptr results in using the first specified application.
This fixes Settings do not open if json files are assigned to gvim #1789
If no application is assigned for json files fall back to notepad

See https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shellexecutea for more details
especially why the result code checking is so horrific.

Summary of the Pull Request

References

PR Checklist

Closes Settings do not open if json files are assigned to gvim #1789
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Set a default application and make sure it opens
Remove the default application and make sure that notepad gets opened

miniksa · 2019-07-05T18:51:28Z

src/cascadia/TerminalApp/App.cpp

@@ -445,7 +445,12 @@ namespace winrt::TerminalApp::implementation
        co_await winrt::resume_background();

        const auto settingsPath = CascadiaSettings::GetSettingsPath();
-        ShellExecute(nullptr, L"open", settingsPath.c_str(), nullptr, nullptr, SW_SHOW);
+
+        HINSTANCE res = ShellExecute(nullptr, nullptr, settingsPath.c_str(), nullptr, nullptr, SW_SHOW);


I get the "try something else when the first one fails," but why did you drop the "open" verb?

Also can you leave a comment back to this page https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shellexecutea that explains why the result code checking is so horrific for this API?

I think dropping the open verb is also documented there:

NULL
The default verb is used, if available. If not, the "open" verb is used. If neither verb is available, the system uses the first verb listed in the registry.

That seems strictly better. GVim registers the edit verb (which we learned in #1789!)

I tried to explain this in the commit message. Gvim only sets up the registry for the "edit" verb so it does not work with the "open" verb. Setting this to NULL solves the issue as well.

NULL
The default verb is used, if available. If not, the "open" verb is used. If neither verb is available, > the system uses the first verb listed in the registry.

@miniksa if you're concerned about the oddities of the ShellExecute return value, it might make sense to use ShellExecuteEx instead because that returns true or false.

As to why it's so odd, it made more sense in 16 bits.

This fixes two issues. * Opens the assigned default application regardless of its configuration. Gvim for example only reacts to the "edit" verb so when selected as default application won't open. Using nullptr results in using the first specified application. This fixes #1789 * If no application is assigned for json files fall back to notepad See https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shellexecutea for more details especially why the result code checking is so horrific.

DHowett-MSFT

Thanks for doing the work and investigation here!

DHowett-MSFT · 2019-07-05T19:16:48Z

src/cascadia/TerminalApp/App.cpp

-        ShellExecute(nullptr, L"open", settingsPath.c_str(), nullptr, nullptr, SW_SHOW);
+
+        HINSTANCE res = ShellExecute(nullptr, nullptr, settingsPath.c_str(), nullptr, nullptr, SW_SHOW);
+        if ((INT_PTR)res <= 32)


In additional to Michael's comment, I'd prefer a static_cast<intptr_t>() versus a C cast, but I'm otherwise totally willing to approve this.

Let me change this and push again.

Well apparently only

(reinterpret_cast<intptr_t>(res)

works, which I think does the same

Looking at this some more I think that reinterpret_cast just works since it does not check anything and the static cast fails, hence I will keep it as it is. Feel free to change it.

zadjii-msft

I agree with fixing the c-style cast, but otherwise this is a great fix, thanks!

maru-sama · 2019-07-08T13:53:27Z

I agree with fixing the c-style cast, but otherwise this is a great fix, thanks!

I tried that before and could not get static_cast to work at all. reinterpret_cast was working but I do not feel comfortable using it here.

maru-sama · 2019-07-08T15:05:13Z

I did find a "fix" for that. but it looks akward....

static_cast<int>(reinterpret_cast<uintptr_t>.....

This should get rid if the compiler warnings I get otherwise. Should I add this or leave it like it is right now?

maru-sama · 2019-07-09T07:38:59Z

Hi anything else that I can do to get this merged?

zadjii-msft

Looks fine to me. Thanks!

* Make opening the settings file more robust This fixes two issues. * Opens the assigned default application regardless of its configuration. Gvim for example only reacts to the "edit" verb so when selected as default application won't open. Using nullptr results in using the first specified application. This fixes microsoft#1789 * If no application is assigned for json files fall back to notepad See https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shellexecutea for more details especially why the result code checking is so horrific. * Fix c-style cast

ghost · 2019-08-03T01:48:41Z

🎉Windows Terminal Preview v0.3.2142.0 has been released which incorporates this pull request.:tada:

Handy links:

miniksa reviewed Jul 5, 2019

View reviewed changes

DHowett-MSFT approved these changes Jul 5, 2019

View reviewed changes

zadjii-msft reviewed Jul 8, 2019

View reviewed changes

Fix c-style cast

d7f7c2e

zadjii-msft approved these changes Jul 9, 2019

View reviewed changes

zadjii-msft merged commit bce8a79 into microsoft:master Jul 9, 2019

metathinker mentioned this pull request Jul 10, 2019

Settings doesn't open reliably when there's no association for .json files #1460

Closed

ghost mentioned this pull request Aug 3, 2019

Settings do not open if json files are assigned to gvim #1789

Closed

claudenobs mentioned this pull request Feb 7, 2020

Settings doesn't open reliably when there's no notepad.exe #4502

Closed

amoldeshpande mentioned this pull request Aug 20, 2020

#1841 not good enough #7355

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make opening the settings file more robust #1841

Make opening the settings file more robust #1841

maru-sama commented Jul 5, 2019 •

edited

Loading

miniksa Jul 5, 2019

DHowett-MSFT Jul 5, 2019

maru-sama Jul 5, 2019

metathinker Jul 5, 2019

DHowett-MSFT left a comment

DHowett-MSFT Jul 5, 2019

maru-sama Jul 5, 2019

maru-sama Jul 5, 2019

maru-sama Jul 5, 2019

zadjii-msft left a comment

maru-sama commented Jul 8, 2019 •

edited

Loading

maru-sama commented Jul 8, 2019 •

edited

Loading

maru-sama commented Jul 9, 2019

zadjii-msft left a comment

ghost commented Aug 3, 2019

Make opening the settings file more robust #1841

Make opening the settings file more robust #1841

Conversation

maru-sama commented Jul 5, 2019 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

maru-sama commented Jul 8, 2019 • edited Loading

maru-sama commented Jul 8, 2019 • edited Loading

maru-sama commented Jul 9, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

ghost commented Aug 3, 2019

maru-sama commented Jul 5, 2019 •

edited

Loading

maru-sama commented Jul 8, 2019 •

edited

Loading

maru-sama commented Jul 8, 2019 •

edited

Loading