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

Add subscription support #2392

Merged
merged 15 commits into from Jan 11, 2020
Merged

Add subscription support #2392

merged 15 commits into from Jan 11, 2020

Conversation

@madeye
Copy link
Contributor

madeye commented Dec 30, 2019

  • Add animations
  • Fix Icons
  • Handle exceptions nicely
  • Check race conditions
@Mygod

This comment has been minimized.

Copy link
Contributor

Mygod commented Dec 30, 2019

This seems overly complicated. Why do we need multiple subscriptions anyway?

@madeye

This comment has been minimized.

Copy link
Contributor Author

madeye commented Dec 30, 2019

My understanding is that we need a separate fragment to manage the subscriptions.

Not sure if users would use multiple subscriptions. But I didn't see multiple subscriptions would bring much complexity.

@Mygod

This comment has been minimized.

Copy link
Contributor

Mygod commented Dec 30, 2019

I see. Sure then.

@madeye madeye changed the title Initial draft to add subscription support Add subscription support Dec 31, 2019
@madeye madeye marked this pull request as ready for review Dec 31, 2019
@madeye

This comment has been minimized.

Copy link
Contributor Author

madeye commented Dec 31, 2019

@Mygod I'm not sure if there would be a race condition that users try to modify profiles when downloading profiles from remote.

Or maybe we can prevent users switch to other fragment when downloading the profiles.

@Mygod

This comment has been minimized.

Copy link
Contributor

Mygod commented Dec 31, 2019

There will not be any races if you simply do everything in main. :)

@madeye

This comment has been minimized.

Copy link
Contributor Author

madeye commented Dec 31, 2019

I'm using GlobalScope.launch to fetch profiles and apply them, that's what I'm worry about...

madeye added 2 commits Dec 31, 2019
@madeye madeye requested a review from Mygod Dec 31, 2019
@madeye

This comment has been minimized.

Copy link
Contributor Author

madeye commented Jan 2, 2020

Any thoughts?

@Mygod

@Mygod

This comment has been minimized.

Copy link
Contributor

Mygod commented Jan 2, 2020

@madeye madeye added the enhancement label Jan 8, 2020
@madeye

This comment has been minimized.

Copy link
Contributor Author

madeye commented Jan 9, 2020

I plan to merge this change and schedule a pre-release soon.

Please comment if there's any concern.

@Mygod

This comment has been minimized.

Copy link
Contributor

Mygod commented Jan 9, 2020

My concern is that these subscriptions does not quite place nicely with profiles that are added manually. Some possible improvements:

  • Flag profiles that are added by subscriptions and make their server settings immutable (but copyable and deletable);
  • Respect existing (subscription) profile's feature settings when replacing; (priorities for choosing feature settings: existing not-necessarily-subscription profile -> JSON profile -> currently selected profile)
  • When syncing, hide (as opposed to removing) profiles that are no longer present. (in other words, the only way to re-sync feature settings is to remove them manually and re-sync)

@madeye Thoughts?

Copy link
Contributor

Mygod left a comment

I made some preliminary changes for now. Besides the high level comment above, I will make some changes (background sync, etc) during this weekend.

@madeye

This comment has been minimized.

Copy link
Contributor Author

madeye commented Jan 9, 2020

I agree that we can add a new flag to each profile, to mark it whether managed by subscription. If a profile is "managed", then it should be immutable and can be overwritten by each sync.

@madeye

This comment has been minimized.

Copy link
Contributor Author

madeye commented Jan 9, 2020

@Mygod Can you help add this "managed" profile feature?

I am not very familiar with the current database design, but the improvement here requires some change to Room related code.

@Mygod

This comment has been minimized.

Copy link
Contributor

Mygod commented Jan 9, 2020

Sure but I probably will not have time to test it until weekend. Room is very similar to ORMlite.

@Mygod Mygod force-pushed the subscription branch from 8e68148 to 02a8fbc Jan 9, 2020
@madeye

This comment has been minimized.

Copy link
Contributor Author

madeye commented Jan 9, 2020

I can help do the testing. Or you can just add the new field in profile, then I can implement all the other logic.

@Mygod

This comment has been minimized.

Copy link
Contributor

Mygod commented Jan 9, 2020

@madeye Done.

@madeye madeye merged commit 406edac into master Jan 11, 2020
2 of 3 checks passed
2 of 3 checks passed
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
License Compliance All checks passed.
Details
ci/circleci Your tests passed on CircleCI!
Details
@madeye madeye deleted the subscription branch Jan 11, 2020
@madeye

This comment has been minimized.

Copy link
Contributor Author

madeye commented Jan 11, 2020

Current design:

  1. Any profile synced from subscription is marked as "managed".
  2. All the server info fields in a managed profile is immutable.
  3. Managed profiles will be deleted from local if it's not present in the latest remote config.
@madeye

This comment has been minimized.

Copy link
Contributor Author

madeye commented Jan 11, 2020

Merged to the master branch now.

@Mygod

This comment has been minimized.

Copy link
Contributor

Mygod commented Jan 11, 2020

Okay. I'll check it out later today (am travelling).

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