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 Unifiedpush support #3

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add Unifiedpush support #3

wants to merge 5 commits into from

Conversation

p1gp1g
Copy link

@p1gp1g p1gp1g commented Oct 25, 2024

SDK

The SDK exposes the UnifiedPush dependency directly with a 'api' dependency so it can be used to un/register, select the default distributor etc.

The library exposes the UnifiedPush broadcast receiver by itself and forward events to a (non-exported) service the application must implement. That way, we reduce risk of any breaking change.
For that reason, I've hesitated to rewrite PushMessage, PushEndpoint, and FailedReason, to avoid passing classes defined in connector's library.

Any message that haven't been correctly decrypted is ignored. For information, the connector library doesn't do that by itself for backward compatibility, if the application doesn't use web push correctly.

Client sample

There is a MockApplicationServer that emulate a remote application server. Client<->Server calls are emulated with functions of MockApplicationServer.MockApi.

I have added some horizontal dividers to add some clarity on the UI.

I've added a new section for push messages:
if unregistered: a button to register
if registered: a button to unregister and a button to send a test notification

The application register to the distributor with VAPID enabled, the MockApplicationServer sends the push message with VAPID.

When receiving a push notification, a notification is displayed with the content of the test message.

@khvMX
Copy link

khvMX commented Oct 28, 2024

Hi @p1gp1g. I have questions which I hope you can shed some light on. I'm sorry if I'm asking things which have been clarified already.

  1. This PR would embed UnifiedPush directly in "COVESA SDK", without clear separation of its implementation and dependencies from other future "libraries". Have you considered some modularization approach to avoid mixing this with previous and future changes? I'm also thinking about versioning separate libraries within COVESA SDK separately.
  2. Should we have different sample apps for different libraries?
  3. Is "EventBus" dependency really necessary with current Kotlin and Jetpack state of the art capabilities?

@p1gp1g
Copy link
Author

p1gp1g commented Oct 29, 2024

Hi @khvMX,

  1. This PR would embed UnifiedPush directly in "COVESA SDK", without clear separation of its implementation and dependencies from other future "libraries". Have you considered some modularization approach to avoid mixing this with previous and future changes? I'm also thinking about versioning separate libraries within COVESA SDK separately.

Indeed, I think there are 2 ways to expose the feature: directly as an api dependency, and the service uses intent with the library classes (https://github.com/COVESA/covesa-aosp-sdk/pull/3/files#diff-dbdd43b7bfea8a29b76c088d80d8d17974ee18ddb299a354f0084cea5f853a49R9-R11) or write new classes that call the library ones. The first solution takes advantage of new features (if any) more easily, the second improves control on the updates.

I asked Maximilian, and he said that one or the other is fine to open the PR, then we can discuss about it.

Let me know if you prefer the 2nd option, it is very fast to do

  1. Should we have different sample apps for different libraries?

Both solution make sense. Having a different sample per feature would probably make it clearer. Let me know if you want to split the sample

  1. Is "EventBus" dependency really necessary with current Kotlin and Jetpack state of the art capabilities?

I've removed it, also it wasn't very clean how the PushUiState was updated, I've fixed that

@khvMX
Copy link

khvMX commented Oct 29, 2024

Indeed, I think there are 2 ways to expose the feature: directly as an api dependency, and the service uses intent with the library classes (https://github.com/COVESA/covesa-aosp-sdk/pull/3/files#diff-dbdd43b7bfea8a29b76c088d80d8d17974ee18ddb299a354f0084cea5f853a49R9-R11) or write new classes that call the library ones. The first solution takes advantage of new features (if any) more easily, the second improves control on the updates.

I asked Maximilian, and he said that one or the other is fine to open the PR, then we can discuss about it.

Let me know if you prefer the 2nd option, it is very fast to do

Good point, let's create an issue for that here? It would be great if others would join the discussion. On some of the meetings someone said that "COVESA SDK clients' efforts must be reduced to the absolute minimum", which I support too. Also, reducing API surface would allow for easier under-the-hood changes, albeit for the cost of client flexibility.

@khvMX
Copy link

khvMX commented Oct 29, 2024

3. Should we have different sample apps for different libraries?

Both solution make sense. Having a different sample per feature would probably make it clearer. Let me know if you want to split the sample

Similar to the above, we probably should create another issue for that, simply to come back to this question once we have at least 3 different SDK features to see any commonalities between them.

@p1gp1g
Copy link
Author

p1gp1g commented Oct 31, 2024

Do you prefer issues or github discussions ? and would you like me to open them?

@khvMX
Copy link

khvMX commented Nov 4, 2024

Do you prefer issues or github discussions ? and would you like me to open them?

Let's move these to (potentially separate) discussions. Could you please create them stating how you have done it for UnifiedPush for now?

As for this PR, it LGTM.

@p1gp1g
Copy link
Author

p1gp1g commented Nov 4, 2024

Discussions for the 2 subjects:
#4
#5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants