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

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

Closed
MatMaul opened this issue Dec 25, 2021 · 11 comments
Closed

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

MatMaul opened this issue Dec 25, 2021 · 11 comments

Comments

@MatMaul
Copy link

MatMaul commented Dec 25, 2021

I am working on adding support for D-Bus as a Linux backend to the flutter connector.

I am running into troubles because the Android API exposed here hides the token behind an instance chosen name instead, and store a mapping for that.
It complicated a lot of things when trying to abstract it, and I think staying closer to the specification and letting this to be handled by the user, or to some helpers instead, is perhaps not a bad idea.

What do you think ? The main problem is that we would break the API. I am however happy to propose something.

@MatMaul
Copy link
Author

MatMaul commented Dec 25, 2021

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

@karmanyaahm
Copy link
Member

karmanyaahm commented Dec 26, 2021

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

@p1gp1g
Copy link
Member

p1gp1g commented Dec 27, 2021

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
Copy link
Author

MatMaul commented Dec 27, 2021

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
Copy link
Member

p1gp1g commented Dec 27, 2021

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 ?

@MatMaul
Copy link
Author

MatMaul commented Dec 27, 2021

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
Copy link
Author

MatMaul commented Dec 27, 2021

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 :)

@p1gp1g
Copy link
Member

p1gp1g commented Dec 27, 2021

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

@MatMaul
Copy link
Author

MatMaul commented Dec 27, 2021

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

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

@p1gp1g
Copy link
Member

p1gp1g commented Dec 28, 2021

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

@MatMaul
Copy link
Author

MatMaul commented Dec 28, 2021

Indeed let's close that :)

@MatMaul MatMaul closed this as completed Dec 28, 2021
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

No branches or pull requests

3 participants