Feat: Add UnifiedPush support#16526
Conversation
|
And I'll start by fixing some conflicts 👍 |
5cf0a27 to
84bf56d
Compare
| private fun appTypes(): List<String> = context.packageManager | ||
| .getLaunchIntentForPackage(APP_NEXTCLOUD_TALK)?.let { | ||
| listOf("all", "-talk") | ||
| } ?: listOf("all") |
There was a problem hiding this comment.
That is to get all notifications, except talk ones if talk is installed. But:
- We can decide to never receive talk notifications (if a user wants talk notif, they probably use Talk app)
- And it would be better to list the applications we want to receive notifications from
There was a problem hiding this comment.
What about talk notifications? Is it expected that the talk app will independently handle unifiedpush notifications too?
There was a problem hiding this comment.
I'm cleaning up the code to open the pull request
| @JvmStatic | ||
| fun registerCurrentPushConfiguration(activity: Activity, accountManager: UserAccountManager, preferences: AppPreferences) { | ||
| if ( | ||
| (!preferences.isPushInitialized && BuildConfig.DEFAULT_PUSH_UNIFIEDPUSH) |
There was a problem hiding this comment.
Instead of using a BuildConfig, we could add a function isPlayServiceAvailable to PushUtils (Implemented per flavor, dedicated to FCM and proxy-push) to check if the Play Services are available.
That way, a user using the gplay flavor on a phone without the Play Services can use by default UnifiedPush.
But it introduces a bit more change, and it is a bit less readable. As you want
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
Hi! Thanks for your contribution. Could you please resolve conflict? Then we can review. |
Signed-off-by: sim <git@sgougeon.fr>
The capability doesn't have an accountName, and therefore doesn't update the user cache Signed-off-by: sim <git@sgougeon.fr>
…e message Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
…ilable Signed-off-by: sim <git@sgougeon.fr>
To catch easily when we do UnifiedPush or Common push tasks Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
… a distributor Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
…nregistration Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
|
I'm doing it |
|
BTW, we need this PR too: nextcloud/android-library#1949 |
Signed-off-by: sim <git@sgougeon.fr>
|
The PR is rebased, and conflict free 👍 |
|
hmmm github still complains about conflicts 🤔 or is the webui out of date? |
| public String deviceIdentifierSignature; | ||
| public String userPublicKey; | ||
| public boolean shouldBeDeleted; | ||
| public boolean shouldBeDisabled = false; |
There was a problem hiding this comment.
Why do we need two boolean flag?
There was a problem hiding this comment.
- The first one,
shouldBeDisabledinforms the next push notification worker to disable (proxy) push (wheredisabledwill be set totrue) - the second one
disabledwhen we have successfully disabled (proxy) push notifications
It works like with shouldBeDeleted:
shouldBeDeletedinform the next push notification worker to delete push notification data (whereaccountPushDatawill be wiped)- once deleted (
accountPushData == null), it can't be used
|
|
||
| if (remoteOperationResult.getHttpCode() == HttpStatus.SC_ACCEPTED) { | ||
| int status = remoteOperationResult.getHttpCode(); | ||
| if (deleteLocalData && status == HttpStatus.SC_ACCEPTED) { |
There was a problem hiding this comment.
Why deleteLocalData needs to be true for UnregisterAccountDeviceForProxyOperation.
There was a problem hiding this comment.
deleteLocalData may not be the best name. It describes "delete user public key, and unsubscribe from the proxy".
The idea is that we can unregister for proxy push notifications in 3 cases:
- When we delete an account
- When we disable proxy-push
- When we hard-reset push subscriptions (and rotate encryption keys)
In the first and last case, we want either to wipe our current keys, and unregister on the proxy. In the 2nd case, that may simply be temporary, we don't need/want to unsubscribe
| ){ | ||
| tryUseUnifiedPush(activity, accountManager, preferences) {} | ||
| } else { | ||
| CoroutineScope(Dispatchers.IO).launch { |
There was a problem hiding this comment.
Please use lifecycle scope of passed activity.
There was a problem hiding this comment.
I think it's better to not be tied to the activity lifecycle for that. Is it OK for you if I set a private val scope = CoroutineScope(Dispatchers.IO) like we have for FileUploadHelper ?
There was a problem hiding this comment.
This function will be triggered with class TokenPushEvent Eventbus. And it is called from FileDisplayActivity.onStart.initFile and onRequestPermissionsResult.PermissionUtil.PERMISSIONS_EXTERNAL_STORAGE
Why it is not better to tied to the activity lifecycle?
FileUploadHelper.ioScope should also have cancellation function. We want to avoid this in new PRs.
There was a problem hiding this comment.
This job makes a request that isn't made for its response (which would have been used for the UI), but to change the state on the server. We don't want the job to finish if the user changes the activity
There was a problem hiding this comment.
Okay then so tied to the termination of the app?
-
Assign to a job current implementation and expose via cancel function then call cancel function termination of the app from MainApp.java
-
Internally add a mechanism to cancellation after n minutes and reassessing if its triggered.
What do you think?
There was a problem hiding this comment.
I'm thinking about a scope that forces the job to be run in a timeout. I'm pushing something to show, let me know what you think about after that
| accountManager: UserAccountManager, | ||
| account: OwnCloudAccount | ||
| ) { | ||
| CoroutineScope(Dispatchers.IO).launch { |
There was a problem hiding this comment.
Creates an unmanaged scope. Every call creates a new independent CoroutineScope that isn't tied to any lifecycle. If the Activity is destroyed or the operation should be cancelled, these coroutines keep running.
| ) { | ||
| CoroutineScope(Dispatchers.IO).launch { | ||
| val jobs = accountManager.accounts.map { account -> | ||
| CoroutineScope(Dispatchers.IO).launch { |
There was a problem hiding this comment.
Nested scopes are unnecessary and also needs to be managed scope.
There was a problem hiding this comment.
There is the jobs.joinAll() later, that's to avoid sending requests sequentially
There was a problem hiding this comment.
If registerUnifiedPushForAccount throws for one account rest will be affected as well.
You can use supervisorScope and also please handle cancelation of the scope (CoroutineScope(Dispatchers.IO)).
CoroutineScope(Dispatchers.IO).launch {
supervisorScope {
val jobs = accountManager.accounts.map { account ->
launch {
try {
val ocAccount = OwnCloudAccount(account, context)
val res = registerUnifiedPushForAccount(context, accountManager, ocAccount)
if (res) {
PushUtils.setRegistrationForAccountEnabled(account, false)
}
} catch (e: Exception) {
Log_OC.e(TAG, "Failed to register push for ${account.name}", e)
}
}
}
jobs.joinAll()
}
proxyPushToken?.let {
PushUtils.pushRegistrationToServer(accountManager, it)
}
}
There was a problem hiding this comment.
Thanks. I'm pushing the change, and I'll use the scope within timeout
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
p1gp1g
left a comment
There was a problem hiding this comment.
Thanks for the review. I'm pushing a few commits.
I tried to answer some of your comments. Do you have a matrix account, or anywhere to communicate directly ? It will be easier to discuss a few points.
|
|
||
| if (remoteOperationResult.getHttpCode() == HttpStatus.SC_ACCEPTED) { | ||
| int status = remoteOperationResult.getHttpCode(); | ||
| if (deleteLocalData && status == HttpStatus.SC_ACCEPTED) { |
There was a problem hiding this comment.
deleteLocalData may not be the best name. It describes "delete user public key, and unsubscribe from the proxy".
The idea is that we can unregister for proxy push notifications in 3 cases:
- When we delete an account
- When we disable proxy-push
- When we hard-reset push subscriptions (and rotate encryption keys)
In the first and last case, we want either to wipe our current keys, and unregister on the proxy. In the 2nd case, that may simply be temporary, we don't need/want to unsubscribe
| public String deviceIdentifierSignature; | ||
| public String userPublicKey; | ||
| public boolean shouldBeDeleted; | ||
| public boolean shouldBeDisabled = false; |
There was a problem hiding this comment.
- The first one,
shouldBeDisabledinforms the next push notification worker to disable (proxy) push (wheredisabledwill be set totrue) - the second one
disabledwhen we have successfully disabled (proxy) push notifications
It works like with shouldBeDeleted:
shouldBeDeletedinform the next push notification worker to delete push notification data (whereaccountPushDatawill be wiped)- once deleted (
accountPushData == null), it can't be used
| ){ | ||
| tryUseUnifiedPush(activity, accountManager, preferences) {} | ||
| } else { | ||
| CoroutineScope(Dispatchers.IO).launch { |
There was a problem hiding this comment.
I think it's better to not be tied to the activity lifecycle for that. Is it OK for you if I set a private val scope = CoroutineScope(Dispatchers.IO) like we have for FileUploadHelper ?
| ) { | ||
| CoroutineScope(Dispatchers.IO).launch { | ||
| val jobs = accountManager.accounts.map { account -> | ||
| CoroutineScope(Dispatchers.IO).launch { |
There was a problem hiding this comment.
There is the jobs.joinAll() later, that's to avoid sending requests sequentially
| ) { | ||
| CoroutineScope(Dispatchers.IO).launch { | ||
| val jobs = accountManager.accounts.map { account -> | ||
| CoroutineScope(Dispatchers.IO).launch { |
There was a problem hiding this comment.
If registerUnifiedPushForAccount throws for one account rest will be affected as well.
You can use supervisorScope and also please handle cancelation of the scope (CoroutineScope(Dispatchers.IO)).
CoroutineScope(Dispatchers.IO).launch {
supervisorScope {
val jobs = accountManager.accounts.map { account ->
launch {
try {
val ocAccount = OwnCloudAccount(account, context)
val res = registerUnifiedPushForAccount(context, accountManager, ocAccount)
if (res) {
PushUtils.setRegistrationForAccountEnabled(account, false)
}
} catch (e: Exception) {
Log_OC.e(TAG, "Failed to register push for ${account.name}", e)
}
}
}
jobs.joinAll()
}
proxyPushToken?.let {
PushUtils.pushRegistrationToServer(accountManager, it)
}
}
| fun startNotificationJob(subject: String, signature: String) | ||
| fun startDecryptedNotificationJob(accountName: String, message: String) | ||
|
|
||
| sealed class WebPushJobData() { |
There was a problem hiding this comment.
Please put WebPushJobData in owncloud/android/datamodel package.
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Following web push support in nextcloud/notifications, we can add UnifiedPush support to the Android application*, to get push notifications even without the Play Services.
The implementation follows this guide: https://unifiedpush.org/developers/ux/
This feature also gives the possibility to get Push Notifications with the Play Services, without the proxy (Nextcloud servers directly push to Google FCM servers), and without a proprietary library. So this is an accepted way to use FCM for application in F-Droid. => This is in a 2nd PR, already ready.
Fix #11898
Fix #5510
Fix #8684
Fix #3333 (with the 2nd PR)
* We could add it to the Linux app too, but I haven't, at least yet.
🖼️ Screenshots
Coming, I will update this description
🏁 Checklist