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

Refactor subscriptions #2402

Merged
merged 5 commits into from Jan 12, 2020
Merged

Refactor subscriptions #2402

merged 5 commits into from Jan 12, 2020

Conversation

@Mygod
Copy link
Contributor

Mygod commented Jan 11, 2020

Probably finish up tonight.

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Jan 11, 2020

Cool, hope we can have a pre-release next week.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Jan 11, 2020

@madeye Hmm wouldn't it make more sense to simply use an EditText for all the subscription URLs?

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Jan 11, 2020

I think it's much better to have a fragment to manage all subscriptions...

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Jan 11, 2020

EditText allows for copy/paste batch actions though.

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Jan 11, 2020

One concern is that it would be hard to validate each URL, like reporting insecure URLs.

Given we already have the subscription fragment, I don't think we should remove it. At least, we can extend subscription feature based on this in the future, e.g. subscription traffic statics.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Jan 11, 2020

Okay. Should we extend this feature to tv?

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Jan 11, 2020

On TV, I think one EditText should be good enough.

With subscription support on TV, I think we can improve the Google Play score a lot...

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Jan 12, 2020

Alright. Not sure what future extensions you are planning for but I will try to keep changes minimal. :)

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Jan 12, 2020

Hmm I'm currently getting java.lang.VerifyError: Verifier rejected class com.github.shadowsocks.subscription.SubscriptionService$onStartCommand$1$invokeSuspend$$inlined$map$lambda$1: java.lang.Object com.github.shadowsocks.subscription.SubscriptionService$onStartCommand$1$invokeSuspend$$inlined$map$lambda$1.invokeSuspend(java.lang.Object) failed to verify: java.lang.Object com.github.shadowsocks.subscription.SubscriptionService$onStartCommand$1$invokeSuspend$$inlined$map$lambda$1.invokeSuspend(java.lang.Object): [0x14A] 'this' argument 'Uninitialized Reference: com.github.shadowsocks.subscription.SubscriptionService$onStartCommand$1$invokeSuspend$$inlined$map$lambda$1$4 Allocation PC: 328' not instance of 'Precise Reference: com.github.shadowsocks.subscription.SubscriptionService$onStartCommand$1$invokeSuspend$$inlined$map$lambda$1$2' (declaration of 'com.github.shadowsocks.subscription.SubscriptionService$onStartCommand$1$invokeSuspend$$inlined$map$lambda$1' appears in /data/app/com.github.shadowsocks-MdKJd5Xmv6syOmQxl84ysw==/base.apk!classes4.dex). Seems like compiler bug.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Jan 12, 2020

I cannot be bothered to make a minimal example to reproduce this error and report to Kotlin.

@madeye Can you help test this?

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Jan 12, 2020

Sure.

@madeye madeye marked this pull request as ready for review Jan 12, 2020
@madeye
madeye approved these changes Jan 12, 2020
Copy link
Contributor

madeye left a comment

Tested and verified.

@madeye madeye merged commit 4126b31 into shadowsocks:master Jan 12, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci Your tests passed on CircleCI!
Details
@perqin

This comment has been minimized.

Copy link

perqin commented Jan 12, 2020

Will libev port implement this subscription mechanism? I am wondering how this feature will be implemented by those ports without GUI. I am using libev port on my Linux desktop, and it will be nice to have subscription support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.