Remove broadcastControl static variable - #15
Conversation
0bc3c1d to
96d2cc2
Compare
|
Thanks for the contribution, but unfortunately I do not entirely understand all the points. I am relatively new to Kotlin, so maybe you could help me understand some of your changes:
Also thanks for updating the library version, but I will have to skip this commit. I am currently running a public beta with the old versions and plan to release it soon. Unless there is something urgent in the new versions I would update the libraries when I start working on larger features again, which might be a while before I get around to this project again. I had some nasty surprises with blindly updating libraries in a different project... |
|
This PR does not introduce any new features or bug fixes, so please feel free to discard it if you prefer. I also fully agree with waiting until after your next release before merging anything like this.
for the boradcast constant is just to give the variable a scope, those variables are used inside that object, so is nice to have it inside the object. For the PreferencesKey values, the main reason is memory efficiency. When we create multiple instances of the Settings object, each instance also creates its own copy of those constants. This is unnecessary, since they are essentially just identifiers and can safely be shared. By putting them in a companion object (or even a top-level object), they are only allocated once.
This part is more a question of style. Personally, I try to avoid static variables because they make it harder to control when and how the value changes. Here it’s just a flag, but adding it to Regarding threading: you are right, What I don’t really get is the purpose of the flag itself: if I set up another app to triggers the intent, why would I want to block it instead of just letting it complete? |
|
Thanks, the reasoning behind putting these into a companion object makes sense, although it still looks somewhat wrong to me. But the companion object feels weird anyway as I am more used to static Java objects. About the blocking function, I agree that it should be nearly impossible to run into an issue here, but using a runBlocking here does not feel right. The point of the flag is security: Allowing any app to control α-Remote and therefore the camera through this leaves all users open to at least some annoying shenanigans from third parties. In practice it might not be relevant as α-Remote is a niche app with very few users, but leaving all users open to any app on their phone sending arbitrary commands to their camera just isn't right. So, since this feature is used by only very few users who then understand how intents work, the flag allows this to be disabled by default unless explicitly activated. Seems to be common practice for several apps that expose potentially disruptive operations through this (for example "Gadget Bridge" which allows controlling Bluetooth connections through their "Intent API"). The big difference to other Broadcast Intents is that this one is not filtered at all, as it should be open to any creative use case. So, thanks already, I learned a few new things and I will probably merge many of your suggestions next time I work on this project after the upcoming release. |
this pr: