Change API to be closer to the Android/D-Bus spec ?

[unifiedpush]

Change API to be closer to the Android/D-Bus spec ?

[karmanyaahm]

I made a prototype of linux flutter without having to change this library, though I never completed my implementation so I might be missing something. Do you want to discuss in UnifiedPush/flutter-connector#45 first? I'll post some more info about my experiments there soon

[MatMaul]

Ahhh cool then we are basically in sync :)

Happy to keep that in the implementation for ease of use.
My interface is only the platform interface for the platform specific implementations, I can totally keep the instance mechanism, automatic token generation and distributor selection dialog at the user API level.

I'll ditch app indeed, I was already feeling that it was the right way to do it.

Thanks a lot for the feedback, you should have a PR pretty soon on flutter-connector and we can see from there I guess.

[MatMaul]

Ah and for multi distributors, I don't really know if there is a use case. I guess a fallback one ? But it would be a bit complicated since the app will have to deduplicate the pushes. As long as it stays a possibility in the spec in case of I am happy :)

[MatMaul]

We can also not break the API, but this is going to be ugly, with some weird names for the new methods.

[p1gp1g]

Well, it seems we did not write the spec for the instance extras - which I was pretty sure it was done. So we indeed made something that has been out of spec. :/ [edit: not true]

Actually the token identifies the a connection between the app and the distributor. The instances have been added to easily have multi-account with a single endpoint per account.

[edit: quote removed, irrelevant]

To argue in favour of updating the spec : * current spec with instances is backward-compatible since the instance extra is not mandatory ; * current android-connector and flutter-connector are not backward compatible if we remove the instance extra ; * the token is kept has an unique identifier between app and distributor.

[edit: that is an implementation discussion]

But, I am not against keeping the spec without the instance, and removing it in the connector. Is it worth it ?

[edit: I mixed up implementation and specifications in this comment]

[MatMaul]

I am not sure we should add in the spec another identifier to the connection mainly for ease of use, it feels to me like it's an implementation detail and mainly cosmetic. It will add complexity to it, while it's currently pretty minimal and I find that great.

In my flutter prototype, for the platform interface, I have ditch some other interfaces (saveDistributor, registerAppWithDialog, ...) that are not in the spec (cross-checked against D-Bus one) and can be abstract in the plugin API or dealt with in the user code instead, here is the end result:

  Future<List<String>> getDistributors() 

  Future<void> registerApp(String distributor, String token, String app)

  Future<void> unregister(String token)

  Future<void> initializeCallback({
    void Function(String token, String endpoint)? onNewEndpoint,
    void Function(String token, String? message)? onRegistrationFailed,
    void Function(String token, String? message)? onRegistrationRefused,
    void Function(String token)? onUnregistered,
    void Function(String token, String message)? onMessage,
  }) 

  Future<void> initializeBackgroundCallback({
    void Function(String token, String endpoint)? onNewEndpoint, //need to be static
    void Function(String token)? onUnregistered, //need to be static
    void Function(String token, String message)? onMessage, //need to be static
  })

With that you can even register on several distributors at the same time and all should work fine.

This is the platform interface only however, I am currently trying to mostly not break the actual API by adding support for instance and distributor selection/dialog in the Dart code instead, it's a bit painful to keep the compat but at least it can be converted in some helper functions if I can't retrofit properly. Also I am currently not depending on the android connector anymore, I have copy paste some code but not really much in the end.

We could also do the same in the Android code, separate the code in 2 layers for example.

[p1gp1g]

Sorry, I probably answered too late yesterday ! And I've got mixed up implementation and specifications.

So, the android-connector follows the specifications. The instance parameter is an implementation thing that identify a connection between the connector and the distributor. As we can see here, it is never sent to the distributor : https://github.com/UnifiedPush/android-connector/blob/main/connector/src/main/java/org/unifiedpush/android/connector/Registration.kt#L26-L31

Instance and token

There are 2 reasons we wanted the developers never have to deal with the token :

• it is easier to use (no token has to be generated) :

registerApp(context) // single instance
registerApp(context, account1) // multi instance

https://github.com/UnifiedPush/android-connector/blob/main/connector/src/main/java/org/unifiedpush/android/connector/Registration.kt#L15

• Developers won't do weak things, like using static token (I have seen a mastodon app using a static private key for webpush). It would allow a malicious app to interact with the app and we don't want it.

Therefore, token generation will definitely not be given to the user.

Nevertheless, what you are describing can be very easily tweak :

  Future<List<String>> getDistributors() 

  Future<void> registerApp(String distributor, String tokenIdentifier, String app)

  Future<void> unregister(String token)

  Future<void> initializeCallback({
    void Function(String tokenIdentifier, String endpoint)? onNewEndpoint,
    void Function(String tokenIdentifier, String? message)? onRegistrationFailed,
    void Function(String tokenIdentifier, String? message)? onRegistrationRefused,
    void Function(String tokenIdentifier)? onUnregistered,
    void Function(String tokenIdentifier, String message)? onMessage,
  }) 

  Future<void> initializeBackgroundCallback({
    void Function(String tokenIdentifier, String endpoint)? onNewEndpoint, //need to be static
    void Function(String tokenIdentifier)? onUnregistered, //need to be static
    void Function(String tokenIdentifier, String message)? onMessage, //need to be static
  })

Actually, that is what is already done, but we named the tokenIdentifier an instance to make it less technical. And the functions without tokenIdentifier are done for backward compatibility.

I have edited my previous post, I am sorry for the previous misleading answer. I hope it is more clear now.

Multi-Distributor

The instance/token said, there is still something different on your implementation :

## Today:
  UnifiedPush.saveDistributor(distributor);
  UnifiedPush.registerApp(tokenIdentifier); // instance=tokenIdentifier
## Your proposition
  UnifiedPush.registerApp(distributor, tokenIdentifier, app) // I have changed token to tokenIdentifier

So today saved a single distributor for the app and you suggest to set a distributor per connection. (app must be removed since it is the application package name on android).

That was also an implementation choice so end user won't have to choose the distributor for each account, to make it a bit more user friendly (which is not totally 😅 ). It wasn't obvious to do it this way, we discussed about it. Do you think we should allow multi distributor per app, and why ?

[p1gp1g]

Thanks a lot for your contribution, we will try to review it asap :)

[MatMaul]

And here it is:
UnifiedPush/flutter-connector#56

No rush, this is a sizable one, and thanks for the nice welcoming around !

[p1gp1g]

Can we close this issue (since it follows the specs 😄) and continue on the flutter-connector PR ?

[MatMaul]

Indeed let's close that :)