home-assistant
Introduce PushProvider interface for push notification providers
Summary
This PR introduces a generic interface for push notification providers. Currently, the only provider is FCM, but an interface allows supporting additional push providers in the future, such as UnifiedPush.
The implementation is based on the SensorManager as only one instance of each provider is needed but they should still be able to be enabled for only a subset of servers. A PushManager class has also been added to coordinate the various push providers, which is currently just hard coded to use FCM but would provide the primary API for managing push providers.
Checklist
- [x] New or updated tests have been added to cover the changes following the testing guidelines.
- [x] The code follows the project's code style and best_practices.
- [x] The changes have been thoroughly tested, and edge cases have been considered.
- [x] Changes are backward compatible whenever feasible. Any breaking changes are documented in the changelog for users and/or in the code for developers depending on the relevance.
Any other notes
This came as a suggestion in this PR for adding UnifiedPush as an alternative push provider.
Multiple comments aren't addressed. Please take a look to them before asking for re-review. Also I would like @jpelgrom opinion on the architecture since he was the one initially thinking about introducing this concept. So take a look to my comments and ask us to review again your changes :)
How do you imagine UnifiedPush would work with this architecture for choosing between multiple distributors? The current architecture forces each push provider to have a separate class, while my first expectation would be that each distributor could be considered a push provider (for example, you get a list: none / FCM / ntfy / Sunup / ...).
The reason for each push provider having its own class is to support multiple protocols, as well as helping to separate things between minimal and full. Like @TimoPtr said, it's a good idea to keep any references to FCM out of minimal. And all of the UnifiedPush distributors (ntfy, Sunup, etc.) would be covered under one class since they all use the same protocol.
The PushProvider interface is primarily for translating the various protocols into a common interface. For example, Firebase messages are received as a map but UnifiedPush messages are received as raw bytes, and Firebase just provides a token while UnifiedPush provides a url and public key. The PushManager class would then be responsible for coordinating the various push providers. It will provide the list of available providers/distributors as well as handling enabling the selected one.
As for the UI aspect of choosing a distributor, I imagined a setting similar to what I had in my previous PR though more generic to include other push providers apart from just UnifiedPush. This architecture would also open up the possibility of using a different provider for each server.
I understand the advantages and was the one suggesting a generic interface on your previous PR. But how would this setup work for for choosing a distributor, which is a concept specific to UnifiedPush, if the other parts of the app don't know about them as the provider interface doesn't have a way to expose it?
(I tried do something similar for my UP proof of concept, though that has flaws in other ways, so I might simply be looking for something that you didn't add for a good reason and it hasn't clicked yet.)
Ok, I get what you're saying. I was planning on initially just using the default UnifiedPush distributor with UnifiedPush.tryUseCurrentOrDefaultDistributor(context) just to get things working at first. But there's a couple ways I could see for the user choosing a distributor. We could add a getDistributors() method to PushProvider or the UnifiedPush library includes its own dialog for choosing a distributor that we could use.
We could add a
getDistributors()method to PushProvider
That could be an option. What would you return for that function in the FCM push provider?
or the UnifiedPush library includes its own dialog for choosing a distributor
That would require specific UI work outside the provider so kind of defeats the point, I think?
That could be an option. What would you return for that function in the FCM push provider?
For the FCM provider, I'd either return null or "FCM" (or "Firebase"). If we allow null, then we could display it to the user as 'Provider (Distributor)' if not null, otherwise just 'Provider'. So for example, 'FCM' and 'UnifiedPush (ntfy)'. If getDistributor() always returns a list, so FCM returns "FCM", then we'd just display to the user each distributor.
That would require specific UI work outside the provider so kind of defeats the point, I think?
For UnifiedPush, the library handles the UI work, though for other providers, yeah it would require work outside of it. The library's UI probably doesn't align with Home Assistant's, so it's probably not the right choice, just wanted to mention it.
@lone-faerie Are you still interested in working on this? In case it wasn't obvious with the 👍 reaction to your latest comment, your suggestion sounded OK to me. If you have any questions, feel free to ask. I'll change this to draft for now, whenever you're ready you can mark it as ready for review.
Yeah, sorry I've been moving so haven't had any time to work on this. I'm finally settled in so I'll start up on this again soon