UnifiedPush / flutter-connector

Mirror of https://codeberg.org/UnifiedPush/flutter-connector
Apache License 2.0
31 stars 11 forks source link

Revert "Don't explicitly call _onUnregistered in unregisterApp()" #116

Closed p1gp1g closed 1 year ago

p1gp1g commented 1 year ago

Reverts UnifiedPush/flutter-connector#115

Ping @emersion


I was a little too quick to accept PR #115, so I open this one to discuss a bit what is the best behavior.

On the android lib, the connector set a onUnregistered method in a broadcast listener in case the distributor unregister it. And if the app sends an unregister request, the connector removes directly the connection token, so the onUnregistered after it is ignored.

On flutter, calling unregister does the same thing, but for ease of use it directly call _onUnregistered.

By removing the explicit call to _onUnregistered, the app need to write the unregistration logic after the unregister request. To illustrate:

With the explicit call to _onUnregistered:

[...]
void onUnregistered(String _instance) {
  myFunc(_instance);
}
[...]
UnifiedPush.unregister(instance);
// nothing
[...]

Without the explicit call:

[...]
void onUnregistered(String _instance) {
  myFunc(_instance);
}
[...]
UnifiedPush.unregister(instance);
myFunc(instance);
[...]

I think the flutter one is more efficient but it gives less liberty to the developers.

@emersion : What do you think of it ?


PS: Fortunately, the modification of this behavior seems to disturb only the example :)

emersion commented 1 year ago

At least ntfy will call onUnregistered asynchronously after it's done handling the unregister intent... Are you using a distributor which doesn't?

IOW, this explicit call shouldn't be needed for the onUnregistered callback to be called.

p1gp1g commented 1 year ago

This is how it works atm:

Application sends unregister

Step application Distributor
1 call unregister function
2 unregister function store the token in a local var
2 unregister function removes the token
3 send UNREGISTER to the distributor with the token
4 Store the token in a local var
5 Remove the connection and the token
6 Send UNREGISTERED with the token
7 Ignore since the token is not known

Distributor unregisters an application

Step application Distributor
1 Store the token in a local var
2 Remove the connection and the token
3 Send UNREGISTERED with the token
4 Remove the token
5 call onUnregistered

PS: I was waiting, thinking I sent this comment ^^ PPS: The more I think about it, the more I think it makes sense to ask the app to explicitly make its logic after the unregister request is sent, like it is done on android. I will probably just close that PR then

karmanyaahm commented 1 year ago

Are you using a distributor which doesn't?

Thinking out loud, @p1gp1g would be able to answer this better, but what if a distributor is uninstalled or force stopped or just plain buggy? The app should still move on with its life.

Apps should call their internal unregister logic (delete the endpoint from the server) after both an UNREGISTER request and UNREGISTERED notice. So, I like @emersion's commit, however, if an app runs its unregistered logic (delete endpoint from server) only inside onUnregistered, it would break with a buggy distributor. So technically, #115 should be a major version (maybe with #90?).

p1gp1g commented 1 year ago

Thinking out loud, @p1gp1g would be able to answer this better, but what if a distributor is uninstalled or force stopped or just plain buggy? The app should still move on with its life.

That is exactly why that is implemented that way

For what I've seen, there isn't any app that should suffer from the change ATM (there aren't a lot of flutter app using it ^^). Nevertheless, I agree with the major bump and I'm ok to do it

p1gp1g commented 1 year ago

Closing. The _onUnregistered change has been released in a major release.