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

Open up content dialogs for invalid URIs and unsupported schemes #7523

Merged
merged 7 commits into from
Sep 11, 2020
Merged

Open up content dialogs for invalid URIs and unsupported schemes #7523

merged 7 commits into from
Sep 11, 2020

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Sep 3, 2020 •
edited
Loading

Summary of the Pull Request

If a user clicks a link that is either invalid (cannot be parsed) or has a scheme we do not support (like file or mailto (for now)), we open up a dialog box telling them the issue.

References

#5001

PR Checklist

  • Closes #xxx
  • I work here
  • 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

dialog

DHowett
DHowett requested changes Sep 3, 2020
View reviewed changes
src/cascadia/TerminalApp/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.xaml Outdated
Comment on lines 73 to 75
x:Name="UnsupportedSchemeDialog"
x:Uid="UnsupportedSchemeDialog"
DefaultButton="Primary">
Copy link
Member

Choose a reason for hiding this comment

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

should the unsupported scheme dialog have an "Open Anyway" button that will allow the user an escape hatch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to say that makes me quite nervous - I am not sure what ShellExecute can do with other schemes and we do not have anything in place yet to check for safety of the link

Copy link

Choose a reason for hiding this comment

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

You could show the full link in the dialog box though and allow the user to copy it, so they can get the data. There could even be a button called "Copy link to clipboard".

Copy link
Member

Choose a reason for hiding this comment

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

I agree that there should be some way of letting the user take the URI out if we're not going to handle it ourselves for whatever reason.

Copy link
Contributor Author

@PankajBhojwani PankajBhojwani Sep 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The user can copy the URI from the dialog box that pops out! (The text is selectable)

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 3, 2020
@zadjii-msft zadjii-msft mentioned this pull request Sep 3, 2020
Open
22 tasks
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 3, 2020
@DHowett DHowett mentioned this pull request Sep 4, 2020
Merged
@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Sep 10, 2020
miniksa
miniksa reviewed Sep 10, 2020
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

I'm fine with this, but I don't want to sign until we find some way for the user to get the content out if they're really desperate to run that link by some other means.

@DHowett DHowett merged commit 1377dbc into master Sep 11, 2020
@DHowett DHowett deleted the dev/pabhoj/link_dialog branch September 11, 2020 00:55
@ghost
Copy link

ghost commented Sep 22, 2020

🎉Windows Terminal Preview v1.4.2652.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lo0oG Lo0oG Lo0oG left review comments

@cinnamon-msft cinnamon-msft cinnamon-msft left review comments

@miniksa miniksa miniksa left review comments

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@DHowett DHowett DHowett approved these changes

@zadjii-msft zadjii-msft Awaiting requested review from zadjii-msft

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

Assignees
No one assigned
Labels
Needs-Second It's a PR that needs another sign-off
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@PankajBhojwani @DHowett @carlos-zamora @miniksa @cinnamon-msft @Lo0oG