-
Notifications
You must be signed in to change notification settings - Fork 72
Move to use futures #146
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
base: master
Are you sure you want to change the base?
Move to use futures #146
Conversation
|
Smaller diff can be seen with "hide whitespace" diff mode. |
|
I appreciate the work that went into this, but I have two problems with merging it. One is that it drops support for a version of Android without providing benefits to the user. The other is that I don't understand the code anymore, and I don't have the time to learn how Futures work enough to satisfy myself that your changes are correct. |
|
The benefit is that the service will have higher priority and last longer. Regarding futures, I can try and rewrite without them, it just make the code harder to read. Generally speaking, the idea is as follows: When adding a watcher do the following:
When removing the last watcher, stop the service. Since the service has high priority it needs a notification icon, so it is always created with the service. The idea behind futures is that they allow to easily wait for stuff to complete instead of dealing with all the callback hell. I tried to keep the changes minimal, I might be able to split them to smaller PR so that it will be easier to read. |
|
Please let me know how you would like to proceed, I'm good with what feels right for you. |
|
Can you link me to anything explaining why the lazy service initialisation improves reliability? If you think you can rewrite it without futures, that might be best. I am reluctant to drop support for SDK 23, because that will affect many apps using this plugin. |
|
I couldn't find an article about why futures improves readability, I'm just used to the concept because this is how promises work in javascript and it's easier to recon with. |
|
Hello, just passing by while doing my round of capacitor plugin updates. If that can be of any help, Android SDK 23 support has been dropped in Capacitor 8. |
|
I've moved to maintain capgo's geolocation plugin if that's interesting to anyone. Feel free to close this PR or merge it, I'm good either way. Good luck! |
This will require minsdk to be 24 in order to support Future, but I find this code a lot more readable.
Let me know what you think.
Keeping in mind that I want to add the post notification permission check, this opens the way for it to be added more easily.
I've changed the code so that notification is never null...
The service is started when adding the first watcher and stoped when removing the last watcher.