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.

Sign up for GitHub

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

Jump to bottom

Support the "file" URI scheme #7526

Merged
merged 8 commits into from
Feb 19, 2021
Merged

Support the "file" URI scheme #7526

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1429755
support the file URI scheme
PankajBhojwani Sep 3, 2020
7e66268
dictionary
PankajBhojwani Sep 3, 2020
2e7bc32
added helper function
PankajBhojwani Sep 4, 2020
15dc61e
nit
PankajBhojwani Sep 4, 2020
c09bbbd
resolve conflict
PankajBhojwani Sep 11, 2020
4ac7a85
fix comment
PankajBhojwani Oct 8, 2020
cf1a9b6
merge main
PankajBhojwani Feb 19, 2021
c294dfe
spell
PankajBhojwani Feb 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spell-check/dictionary/dictionary.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183437,6 +183437,7 @@ hostlership
hostlerwife
hostless
hostly
hostname
hostry
hosts
hostship
Expand Down
33 changes: 32 additions & 1 deletion src/cascadia/TerminalApp/TerminalPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1804,7 +1804,7 @@ namespace winrt::TerminalApp::implementation
try
{
auto parsed = winrt::Windows::Foundation::Uri(eventArgs.Uri().c_str());
if (parsed.SchemeName() == L"http" || parsed.SchemeName() == L"https")
if (_IsUriSupported(parsed))
{
ShellExecute(nullptr, L"open", eventArgs.Uri().c_str(), nullptr, nullptr, SW_SHOWNORMAL);
}
Comment on lines +2151 to 2154
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to use the "edit" verb here for file URLs? Perhaps having two separate conditions, like IsUriSupportedForOpen, and IsUriSupportedForEdit. Because if a user is clicking on a link to a .py file, I think it's more likely they would want the file edited rather than executed (which is what the open verb tends to do).

I don't know if there are any downsides to using the edit verb though. I did bring this up in the #7562 discussion thread, but I don't think anyone commented on the merits of the idea one way or the other.

Copy link
Member

@DHowett DHowett Jan 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not in love with the idea of using the edit verb. I just gave it a test on a couple of the files I had laying around:

extension open behavior edit behavior
ps1 notepad powershell ISE (?!)
txt notepad notepad
cpp visual studio preview, which is not my default VS (argh) notepad
no extension "how do you want to open this?" dialog an error message from ShellExecute
.gitconfig, .wslconifg, etc same as above same as above

It's great if somebody's registered for it, but I'm bothered that the OS doesn't give it the same "do what with?" treatment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extensions that concerned me were things like .py, .cmd, .bat, and .vbs. In all cases, the open verb executed the script, and the edit verb opened the file in notepad. Those all seem like files that would not be good to execute when you click on them.

If we're worried about the cases where a file extension doesn't have a registered handler, maybe we can handle that as a special case check. But wanting to click on a link to a file that you've never registered a handler for, seems a far less likely use case than clicking on something like a .py file, and expecting to be able to edit it.

Although maybe I'm the odd one out here. I just assumed nobody would want scripts to execute when clicking on their links.

Expand Down Expand Up @@ -1841,6 +1841,37 @@ namespace winrt::TerminalApp::implementation
}
}

// Method Description:
// - Determines if the given URI is currently supported
// Arguments:
// - The parsed URI
// Return value:
// - True if we support it, false otherwise
bool TerminalPage::_IsUriSupported(const winrt::Windows::Foundation::Uri& parsedUri)
{
if (parsedUri.SchemeName() == L"http" || parsedUri.SchemeName() == L"https")
{
return true;
}
if (parsedUri.SchemeName() == L"file")
{
const auto host = parsedUri.Host();
// If no hostname was provided or if the hostname was "localhost", Host() will return null
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved
// and we allow it
if (host == L"")
{
return true;
}
// TODO: by the OSC 8 spec, if a hostname (other than localhost) is provided, we _should_ be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be filing a follow-up for this? I'd imagine that this would involve returning a different URL that the one emitted. That would let the client emit file://localhost/foo/bar, and then let us ShellExecute file://foo/bar.

// comparing that value against what is returned by GetComputerNameExW and making sure they match.
// However, ShellExecute does not seem to be happy with file URIs of the form
// file://{hostname}/path/to/file.ext
// and so while we could do the hostname matching, we do not know how to actually open the URI
// if its given in that form. So for now we ignore all hostnames other than localhost
}
return false;
}

// Method Description:
// - Copy text from the focused terminal to the Windows Clipboard
// Arguments:
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,8 @@ namespace winrt::TerminalApp::implementation
const Microsoft::Terminal::TerminalControl::PasteFromClipboardEventArgs eventArgs);

void _OpenHyperlinkHandler(const IInspectable sender, const Microsoft::Terminal::TerminalControl::OpenHyperlinkEventArgs eventArgs);
bool _IsUriSupported(const winrt::Windows::Foundation::Uri& parsedUri);

void _ShowCouldNotOpenDialog(winrt::hstring reason, winrt::hstring uri);
bool _CopyText(const bool singleLine, const Windows::Foundation::IReference<Microsoft::Terminal::TerminalControl::CopyFormat>& formats);

Expand Down