Use platform interface abstraction to prepare for D-Bus impl

MatMaul commented 2 years ago

dependency on android-connector has been removed
the platform interface is pretty close to the spec, and user-oriented features like selecting distributor and instance abstraction are left to the common Dart code instead
only needed change for users would be an import in the receiver case, and none for the callback case

TODO:

[x] Implements dialog for distributor selection
[x] Test initializeWithCallback version (currently tested with fluffychat and the receiver way)
[x] Test multiple instances somehow

karmanyaahm commented 2 years ago

Implements dialog for distributor selection

~~I haven't looked at your code yet but~~ the following could help https://github.com/karmanyaahm/flutter-connector/blob/b7a09f5154bf11001ec701b94218111489f963e7/unifiedpush/lib/unifiedpush.dart#L219 https://github.com/karmanyaahm/flutter-connector/blob/platform_interface/unifiedpush/lib/dialogs.dart

karmanyaahm commented 2 years ago

Also

Test initializeWithCallback version

The example app does that, right?

karmanyaahm commented 2 years ago

Test multiple instances somehow

I made multi instance example. https://github.com/karmanyaahm/flutter-connector/tree/multi-instance-example

The primary issue I'm noticing with that is that the functions passed to initializeWithCallbackInstantiated

void Function(dynamic args) callbackOnNewEndpoint
void Function(dynamic args) callbackOnUnregistered
void Function(dynamic args) callbackOnMessage

that are called in the background have no way of knowing what instance they received information from.

It's possible that this issue existed before this PR, I'm not sure. @p1gp1g would know more about this.

MatMaul commented 2 years ago

I made multi instance example. https://github.com/karmanyaahm/flutter-connector/tree/multi-instance-example

Great thanks !

that are called in the background have no way of knowing what instance they received information from.

Indeed. I'll introduce an intermediary static callback function restored on cold launch to deal with that, a bit cumbersome but it should work.

p1gp1g commented 2 years ago

I have pushed the 4 examples : callback/recevier and default instance/multi instance.

@karmanyaahm the callback can get the instance with args["instance"]

MatMaul commented 2 years ago

Thanks a lot for the 4 examples, it eased my life a lot !

I have tested all of them, and fixed some code on the way :wink:

MatMaul commented 2 years ago

Hum so I tried to add the dialog using some of your code @karmanyaahm , but I miserably failed :grimacing: No dialog at all is shown.

Could you have a quick look please when/if you have some times @karmanyaahm (last commit) ?

The rest of the code looks in pretty good shape to me, feel free to make another round of review if needed.

karmanyaahm commented 2 years ago

I'll check in 2-4 hrs

p1gp1g commented 2 years ago

dependency on android-connector has been removed

I guess you did it to remove the need of implementation 'com.github.UnifiedPush:android-connector:{{VERSION}}' when init with a receiver.

Your code made me think of another solution. I'm pushing a version that doesn't need this importation tomorrow without having to copy/paste the android lib - which will be better for development. It may influence other parts of this PR, I try to be more explicit when I've time, it is late now :)

p1gp1g commented 2 years ago

I pushed it finally : https://github.com/UnifiedPush/flutter-connector/commit/23e0b9d97be4b50ea19efd7287f38ef1cdec8223

karmanyaahm commented 2 years ago

I have dialogs working, it's a few small changes but they're all mixed up and are going to be difficult to split into proper commits because it's really late rn, i'll send a pull request on your repo tomorrow

p1gp1g commented 2 years ago

It is a good thing some of the functions moved to the main part, but I think for maintenance, development, and probably other issues the platform library need to be used as possible. The need for app developper to import the library was indeed not a good thing but it is fixed with https://github.com/UnifiedPush/flutter-connector/commit/23e0b9d97be4b50ea19efd7287f38ef1cdec8223 (inspired from your work :) )

So, to make clear : if something is in the platform specs it should be done via the library. The main one should not have to deal with specifications but must be only an interace to other platform libraries. For instance, the registerAppWithDialog have entirely its place in the main part while registerApp need to be directly imported. If a platform need a token and the lib generate one, it is better to use this one.

This PR can easily follow this I think, mainly following the previously quoted commit.

MatMaul commented 2 years ago

Well right now I think that would mean using instance directly in the platform interface instead of token and I kind of strongly disagree with that, I think the platform interface should follow the specs more closely and keep this kind of details at the higher level code as in this PR. I don't think expecting that every platform will have a native implementation, and that they will follow the same instance logic, is a good expectation if this is not in the spec.

Also sometimes re-implementing directly in Dart if possible just make sense especially when it's pretty simple. For example I did have a look if they were some generic Broadcast Receiver flutter code before going the current way, so I can just not have to deal with native code at all. I found https://pub.dev/packages/flutter_broadcasts but after looking closer it doesn't support background receiving when the app is killed.

I can however split the android code to have a more layered codebase:

an advanced interface that match closely the spec using token
the current instance based interface that mostly call the first one and do some translation

I can then use the advanced interface in the flutter-connector.

MatMaul commented 2 years ago

If we could just ditch the thousand of lines of native code + various Dart glue used here and have simple working broadcast receiving using a lib, I think it would be a huge maintenance gain that greatly overcome the small duplication you would do against the native lib.

I am still contemplating adding background broadcast receiving (if possible, I haven't tried so not sure, it used a dynamically registered broadcast receiver and I don't know if it would work) to flutter_broadcasts and use that, that would reduce the codebase by a factor of 3 or 4 I believe.

karmanyaahm commented 2 years ago

The dialog code is here, https://github.com/karmanyaahm/flutter-connector/commits/platform-interface-dialog I also added a commit that deletes the saved distributor more aggressively so it's actually possible to see the dialog more often. The deletion commit doesn't have to be merged if you wouldn't like though. (I think the distributor should be deleted on unregister, but that discussion is out of scope of this PR).

p1gp1g commented 2 years ago

Well right now I think that would mean using instance directly in the platform interface instead of token and I kind of strongly disagree with that, I think the platform interface should follow the specs more closely and keep this kind of details at the higher level code as in this PR.

Well, the specifications are about calls between the connector and the distributor, not about functions exported by the connector libraries. There can be many different libs with very different logic still following the specifications. (Reminder: instance == token identifier :) )

Actually, like any app using unifiedpush, flutter-connector -at least the main part- can be written without even reading specifications.

I don't think expecting that every platform will have a native implementation, and that they will follow the same instance logic, is a good expectation if this is not in the spec.

Every platform don't have to have the native implementation but is is better to use it if it does the exactly what is needed and its integration is easy.

If a platform library doesn't have that instance logic, it must be done on dart side. For instance, if this library is directly asking for a token, it is possible to do a hashmap "instance" to "token" to follow the logic.

Also sometimes re-implementing directly in Dart if possible just make sense especially when it's pretty simple. For example I did have a look if they were some generic Broadcast Receiver flutter code before going the current way, so I can just not have to deal with native code at all. I found https://pub.dev/packages/flutter_broadcasts but after looking closer it doesn't support background receiving when the app is killed.

I agree ! If we find something that get rid of all the callback structure - used to receive message while the app is killed - then not using one implemented with it instead of the android-connector one is a good idea. If we don't gain anything, let's use the platform library :)

Separation platform specific / main implementation

I think platform specific code needs to be in platform side. This organization will make things easier to understand and to maintain.

For instance, the dialog used for registerAppWithDialog is platform agnostic, so it must be written in main part.

In contrast, every platform may not need a token to communicate with the distributor : it is used by android and linux so they both need it. But another version of the specification may not need the token (it could be public/private key for instance, or a version may force a UUID). Also if we implements APNS for iOS or webpush for web app - for ease of use, then the token won't be used there but it is a bit more hacky because it is not unifiedpush specs. There for it must stay in platform specific part and not the main.

Change suggestions for the next non-backward compatible release.

We are definitely going to have a non-backward compatible release.

I think it is time to do other changes :

I suggest removing non-instantiated functions and setting a default instance if not stated. It will be lighter. If an app use only one connection then they just have to ignore instance.
Having a single initialization function if possible (instead of WithCallback/WithReceiver). If we can't, then I suggest having initialize directly calling initializeWithReceiver or we may simply remove the function "WithCallback" ?
It is probably the moment to update the android spec to remove REGISTRATION_REFUSED.

There might be other things to change. It is time to, if you have any idea !

Structure

I think we must agree on functions offered by the flutter lib and platform interfaces (no matter yet if it is then done in native or dart).

I suggest the following

Main side

These are the functions that will be called on the applications :

initialize(...) calls platform initialize(...) *cf previous point

initialize(
    onNewEndpoint, // takes (String endpoint, String instance) in args
    onRegistrationFailed, // takes (String instance) in args
    onRegistrationRefused, // takes (String instance) in args // __Do we keep it ? It is discussed to be removed from specs__
    onUnregistered, // takes (String instance) in args
    onMessage, // takes (String message, String instance) in args
)

getDistributor() calls platform getDistributor, returns current distributor
saveDistributor(ditributor) calls platform saveDistributor
getDistributors() calls platform getDistributors, returns all available distributors
registerApp(instance) calls platform registerApp
registerAppWithDialog(instance) is implemented on the main part, calling other functions
unregisterApp(instance) calls platform unregisterApp

Platform interfaces

So, every platform interfaces will have the following functions so they can be called by the main lib:

initliaze *may have different init

initialize(
    onNewEndpoint, // takes (String endpoint, String instance) in args
    onRegistrationFailed, // takes (String instance) in args
    onRegistrationRefused, // takes (String instance) in args // __Do we keep it ? It is discussed to be removed from specs__
    onUnregistered, // takes (String instance) in args
    onMessage, // takes (String message, String instance) in args
)

getDistributor() returns current distributor
saveDistributor(ditributor)
getDistributors() returns all available distributors
registerApp(instance)
unregisterApp(instance)

All of these make a lot of words for a very few things to do, after what you have already done :). It is mostly moving some functions from main to platform part.

There is still the question about the initialization and about removing non-instantiated functions

MatMaul commented 2 years ago

I have spin up https://github.com/UnifiedPush/specifications/issues/19 as part of the follow up to your answer.

In contrast, every platform may not need a token to communicate with the distributor : it is used by android and linux so they both need it. But another version of the specification may not need the token (it could be public/private key for instance, or a version may force a UUID). Also if we implements APNS for iOS or webpush for web app - for ease of use, then the token won't be used there but it is a bit more hacky because it is not unifiedpush specs. There for it must stay in platform specific part and not the main.

I am more seeing the spec as an abstraction for the IPC mechanisms of the underlying platforms. I don't think iOS/Web would need to have a different interface at the platform level, the fact that we use or not the token looks more like an implementation detail for the APNS/webpush Distributor impl to deal with. My spec proposal also remove this ambiguity I believe.

MatMaul commented 2 years ago

And happy to remove non-instantiated functions. I need to put a bit more thought on how to merge the initialize functions.

I am a lot less convinced about keeping saveDistributor and getDistributor at the platform level interface, it looks like something that would be at the same level as registerWithDialog to me. But I'll survive if I have to keep them :)

p1gp1g commented 2 years ago

I've done some changes to:

continue to use the android library (without having to import at anytime)
generate the token on android-side
set the other functions (I agree those may also be written on the main lib, but having the distributor org on different may have side effect, and may miss functionalities. For instance, getDistributor checks if the distributor is still installed and remove it if not.)

Also, I have removed onRegistrationRemoved atm, it may change. I am waiting some review on https://github.com/UnifiedPush/specifications/pull/18

Commit: https://github.com/UnifiedPush/flutter-connector/commit/84d659bfe6de69522228e3b100e48a5d7074f987

What about renaming instance to tokenIdentifier as said on the spec issue ?

Else, is that ok on your side ?

MatMaul commented 2 years ago

Thanks, let's get this rolling, we can talk about the spec independently, it doesn't change that code much anyway. I am going to put your commit and @karmanyaahm one in this PR.

MatMaul commented 2 years ago

Right here it is. Can I have approval so the workflow run and I can fix stuffs from the CI if needed ?

MatMaul commented 2 years ago

This will still probably fail because I think we need to publish unifiedpush_platform_interface and unifiedpush_android to pub.dev first.

p1gp1g commented 2 years ago

Maybe we can use relative path for development and use the version on release.

For instance with android-example and android-connector: https://github.com/UnifiedPush/android-example/blob/main/app/build.gradle#L61-L62 https://github.com/UnifiedPush/android-connector/blob/main/.github/workflows/main.yml#L18

You can remove the auto flutter pub publish, it is broken. On each release I can do :

publish
make a github release draft (which will build with the published version)
undraft it if it is ok

MatMaul commented 2 years ago

fine for me :)

I've moved to relative paths and removed publish jobs.

MatMaul commented 2 years ago

Ah nevermind this doesn't work at all, we need to not have the relative paths on release. I am tired I'll see tomorrow, not sure how to do so yet.

p1gp1g commented 2 years ago

What do you think about https://github.com/UnifiedPush/flutter-connector/commit/ce64f7c5ae9eb0f2adadfe0c2b83536176e8ebda and https://github.com/UnifiedPush/flutter-connector/commit/c1958af409be66a3d6c108ffa6d22707c62e822c ?

During dev (workflow on commit) we run ./dev.sh first

MatMaul commented 2 years ago

Great :) I've pushed your commits on this PR.

p1gp1g commented 2 years ago

I've pushed 3 other commits on https://github.com/UnifiedPush/flutter-connector/commits/platform-interface :

keep a single example (cf next points)
keep only multi-instance functions
remove background callbacks :
- always use the receiver
- keep a single initialization method
use wip android-connector:2.0.0

Should I open a new PR with this branch for the review ? or you may prefer merge them here.

p1gp1g commented 2 years ago

We still need to rewrite the github workflows

MatMaul commented 2 years ago

Nice thanks :)

I have push it here with a small compilation fix, and also a potential fix for the GH workflows.

I am a bit unsure about how both receivers articulate together, if you have a small explanation I take it. It seems right now that they both hold the wakelock. It's probably not a trouble but just mentioning it in case of.

p1gp1g commented 2 years ago

The android-connector InternalReceiver (extending MessagingReceiver) need to run to receive external actions [1]
The android-connector InternalReceiver handle android connection workflow, with the connector tokens [2]
Events (message, new endpoint, registration failed and unregistration) are broadcasted internally to the app defined Receiver (extending UnifiedPushReceiver) [3] for internal processing. That way:
- This broadcast is exclusively an internal process. It does not depend on specifications (directly) and do not depend on android-connector.
- Supporting a new version of spec should have a bit less impact [4][5]

[1] Maybe renaming it ExternalReceiver instead of InternalReceiver makes sense :thinking: [2] For instance, https://github.com/UnifiedPush/android-connector/blob/main/connector/src/main/java/org/unifiedpush/android/connector/MessagingReceiver.kt#L41-L42 [3] Cf. [1] :smile: [4] For instance, applications won't have to add org.unifiedpush.android.connector.MESSAGEv2 to their manifest to support it. [5] I wrote this change before AND_2.0.0-beta2, and would already be useful. We still need to change onMessage(String message) to onMessage(ByteArray message) [6]

[6] We must change the onMessages from String to ByteArray and test if ByteArrays can be passed through the Plugin

emersion commented 2 years ago

I must admit, I'm not a very big fan of the new InternalReceiver concept. I'd much rather have flutter-connector spawn my callbacks in a separate dart Isolate, and handle the dispatching to my own code via SendPort/ReceivePort, just like every other library is doing (e.g. firebase_messaging, workmanager…). Forcing users to patch their Kotlin code isn't great.

p1gp1g commented 2 years ago

It is probably possible to do it without the kotlin part when using Flutter Android Embedding V2, like FCM. [0]

[0] https://firebase.flutter.dev/docs/messaging/overview/

p1gp1g commented 2 years ago

I have push some changes to the branch platform-interface:

it does not require any change android-side of the applications
it is still possible to have a single isolate if needed
it can require distributor features (needed for #64)

Review is welcome, I'd like to merge soon :)

emersion commented 2 years ago

Nice!

I'd suggest running flutter analyze. For instance, using upper-case names for constants is not idiomatic in Dart, instead lower-case should be used. Some function definitions are missing the return type.
UNKNOWN_INSTANCE seems unused (and is awkward in general, would be better to get rid of it if possible).
It shouldn't be necessary to JSON-encode List<String> features in getDistributors -- invokeMethod can support List<dynamic> (but not List<String>).
There's a stray debugPrint call, maybe should remove it before merging.

p1gp1g commented 2 years ago

I've cleaned the code.

flutter analyze didn't give anything.

I've tried to use a List\<String> for the features instead of a JSON encoded array, but there were some out of bound errors, that's why I have encoded it.

emersion commented 2 years ago

For flutter analyze to report anything, you need to:

Add flutter_lints to dev_dependencies in pubspec,yaml
Create a analysis_options.yaml file containing include: package:flutter_lints/flutter.yaml

in the package you run the command from. Maybe that's what missing?

More info at https://dart.dev/guides/language/analysis-options

p1gp1g commented 2 years ago

Thanks, it is done

p1gp1g commented 2 years ago

I've merged the branch platform-interface based on this PR. Thanks @MatMaul, and all other contributors

UnifiedPush / flutter-connector