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

New option for ss:// URL association #2855

Merged
merged 9 commits into from
Apr 22, 2020
Merged

New option for ss:// URL association #2855

merged 9 commits into from
Apr 22, 2020

Conversation

database64128
Copy link
Contributor

@database64128 database64128 commented Apr 18, 2020

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly

  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])

  • Use Preview tab to see how your pull request will actually look like

  • Searched for similar pull requests

  • Compiled the code with Visual Studio

  • Require translation update

  • Require document update (readme.md, wikipage, etc)

What is the purpose of your pull request?

  • Bug fix
  • Improvement
  • New feature

Description of your pull request and other information

Added an option to allow association with ss:// URLs.

This PR was made at the request of @studentmain . Credit goes to @studentmain for most of the code 👨‍💻!

@ghost ghost requested review from celeron533 and chenshaoju April 18, 2020 14:55
@chenshaoju
Copy link
Collaborator

The quick test is pass, Base appveyor version 4.1.10.4.

But there is a small issue: when ss:// URI is invalid, There is no error message.

RegistryKey ssURLAssociation = null;
try
{
ssURLAssociation = Registry.CurrentUser.CreateSubKey(@"SOFTWARE\Classes\ss", RegistryKeyPermissionCheck.ReadWriteSubTree);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps using full name "Shadowsocks" is better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps using full name "Shadowsocks" is better

By using "Shadowsocks" you are registering Shadowsocks:// instead of ss://.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @database64128 , I made a mistake...

Note:
Outline or other software may also register the protocol
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Outline or other software may also register the protocol

Just tested with Outline installed. The current behavior of our implementation is overwriting existing association, which is not perfect, but good enough. As far as I know, there is no way to associate a URI scheme with more than one Win32 application. The API for that is only available for UWP apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also note that Outline overwrites the URI association on boot. Apparently they are using the same implementation, with a more aggressive approach and no way to turn off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our implementation checks if ss:// is associated with itself on boot to determine the status of association, and shows it with MenuItem.Checked. It's up to the user to associate the URI scheme or not.

Copy link

Choose a reason for hiding this comment

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

with a more aggressive approach and no way to turn off

We need open an issue for it.

@database64128
Copy link
Contributor Author

The quick test is pass, Base appveyor version 4.1.10.4.

But there is a small issue: when ss:// URI is invalid, There is no error message.

Actually the original "Import URL from clipboard" also shows no error message. I added messages for both in a1e4245.

@celeron533
Copy link
Contributor

Name pipe or pipeserver may a bit confusion with existing StartPipe, PipeConnection... network related communications.

@database64128
Copy link
Contributor Author

Name pipe or pipeserver may a bit confusion with existing StartPipe, PipeConnection... network related communications.

Resolved in 4bd33db.

Copy link
Collaborator

@chenshaoju chenshaoju left a comment

Choose a reason for hiding this comment

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

The quick test is pass, Base appveyor version 4.1.10.7.
:+1 Looks good to me, but someone else must approve.

@database64128
Copy link
Contributor Author

Any plan to merge? I've been using the build on my PCs and everything seems good.

@ghost ghost added this to the v4.1.11 milestone Apr 22, 2020
@celeron533 celeron533 merged commit b706cdf into shadowsocks:master Apr 22, 2020
@celeron533
Copy link
Contributor

Merged. Thank you very much @database64128 !

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

Successfully merging this pull request may close these issues.

3 participants