Conversation
|
Thank you for contributing @anakin78z 👍 We will review this soon (will try today but i'm on vacation next week). |
|
Thanks. I'm also waiting to hear back about thstyl2000#1 since they have a massive refactor plan. I'm not sure what stage they're at with implementation though. |
|
and i oversaw it's "just" a draft for now. Just set the state when it's ready to review and let us know if you have questions. |
Switched call notification to NotificationCompat.CallStyle. This adds answer/decline buttons to the notification so that users can answer incoming calls. Adds caller avatar to notification. Signed-off-by: Jens Zalzala <jens@shakingearthdigital.com>
6f538bb to
dcde0be
Compare
There was a problem hiding this comment.
This is what was causing the lock screen to appear after answering a call.
nimishavijay
left a comment
There was a problem hiding this comment.
This is a great improvement! :) One design concern though is that there is not a very big difference between an audio or video call as the text says "Incoming call" for both. So some small changes to reflect that better:
- Could we change the text to "Incoming video call"?
- Could we add an icon to the the video button to indicate that the camera wold be on when you join? it could be the videocam icon
|
@nimishavijay Hi, thanks for the feedback. I think we would have to use a custom style to change the text or add icons, but maybe that could be a separate task, since it would involve design work? |
|
Updated description to note that it also fixes #5351 |
|
Thank you @anakin78z |
mahibi
left a comment
There was a problem hiding this comment.
great improvements @anakin78z 👍
just some minor comment and would like to have input how it's done on iOS..
| import androidx.core.app.NotificationManagerCompat | ||
| import com.nextcloud.talk.utils.bundle.BundleKeys.KEY_NOTIFICATION_TIMESTAMP | ||
|
|
||
| class CallNotificationActionReceiver : BroadcastReceiver() { |
There was a problem hiding this comment.
i suggest
DeclineCallReceiver
as this is the only usecase
There was a problem hiding this comment.
Initially I had it set up to add an additional action to the call notification to always give the option to answer with video or audio, but the way the extra action displayed I found it a bit cramped and inconsistent, so I removed it. We could, however, decide to add additional actions in the future, so I left this receiver more generic.
You can see how the action is being added on line 304 of NotificationWorker.kt: https://github.com/nextcloud/talk-android/pull/6015/changes/BASE..dcde0be993b7ee9733cdca5fa275c13b5b778caa#diff-b51bc8baa586c30bbd846a44639cba62c95cde0f586adb76db389f9e74635914R304
I can change the name if you don't think we'll add other actions. Or I could add some documentation to the Receiver explaining how it could be extended.
Edit: Actually, answering voice/video wouldn't have used this receiver. So it would have to be an unrelated action, and the only thing I can think of right now is maybe sending a text response instead of answering. But it's probably unlikely that we'd use this for anything other than declining calls for now.
There was a problem hiding this comment.
I guessed it was a leftover from other approaches.
I'd suggest to go with DeclineCallReceiver.
Then you could also delete the ACTION_DECLINE_CALL action.
| } | ||
| val callerPerson = callerPersonBuilder.build() | ||
|
|
||
| val isVideoCall = (conversation.callFlag and Participant.InCallFlags.WITH_VIDEO) > 0 |
There was a problem hiding this comment.
Afaik call flags are still like this:
It's not possible to know if the caller has video enabled. The video flag is set if there is the possibility to use video, regardless if it's enabled or not.
So this will most of the times result in showing the video button even when the caller has no video enabled.
Would be less annoying if #708 would be solved.
@SystemKeeper @Ivansss how is this done on iOS?
There was a problem hiding this comment.
In my testing it worked as intended:
If I initiate a video call, the video button shows, and if I initiate an audio call, it doesn't.
That may not handle edge cases, like the caller turning off their video, or switching from audio to video, but I think the most common use case is handled.
I'll wait to hear how it's handled on iOS though.
There was a problem hiding this comment.
For iOS it's the same behavior that a call is offered as videocall.
However on iOS the camera is not activated automatically in this case but you have to enable it yourself. We should do the same.
In the prepareCall() method you find that onCameraClick() is called.
Could you please wrap this with the following check so it's only automatically enabled when the callscreen is not opened via a notification?:
if (!isIncomingCallFromNotification) {
onCameraClick()
}
I think this is the expected behavior for most people.
There was a problem hiding this comment.
Fixed in be0bd8d
I will say, however, that I personally don't like this behavior. I'm not used to having to click an extra button to enable video for video calls. I think a setting would be nice for this. I can create an issue for it after this is merged.
There was a problem hiding this comment.
I understand for some people it's not the expected behavior but as it's privacy related we should disable it initially.
I asked my team and they also voted to disable it.
Some background: the call flag just tells that there is a device existing that could transmit video. It does not say video is enabled. So most of the time the called person will see "incoming video call", but actually the other participant does not have video enabled. In this case, most people will be confused to unveil their own video after accepting the call but don't the the other participants video.
| val notificationId = extras.getInt(KEY_NOTIFICATION_TIMESTAMP, 0) | ||
| if (notificationId != 0) { | ||
| // cancel the notification to stop the call ringing | ||
| NotificationManagerCompat.from(this).cancel(notificationId) |
There was a problem hiding this comment.
this should work and fix the issue with the ongoing ringing.
however it should have been covered by the call of cancelExistingNotificationsForRoom method in performCall, but there seems to be a bug that this is not triggered sometimes.
I will continue debugging in the evening or tomorrow to sort things out..
There was a problem hiding this comment.
i have some ideas what could go wrong, but it's not settled.
To simplify things, i suggest to keep your approach, but to remove the block
if (!TextUtils.isEmpty(roomToken)) {
cancelExistingNotificationsForRoom(
applicationContext,
conversationUser!!,
roomToken!!
)
}
from the function getRoomAndContinue completely.
So even when other things go wrong, the ringing should definitely stop.
Just let me know if you want to do this or if i should take over from here.
Signed-off-by: Jens Zalzala <jens@shakingearthdigital.com>
Signed-off-by: Jens Zalzala <jens@shakingearthdigital.com>
Do not cancel the notifications in getRoomnstead, but do it with NotificationManagerCompat.from(this).cancel(notificationId) in processExtras (see commit dcde0be). It seems that things can go wrong that cancelExistingNotificationsForRoom was not reached which caused an indefinite ringing. The underlying cause why this is not reached is NOT fixed with this commit, but instead dismissing the notification in processExtras seems to be like a more reliable approach. Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
|
Thanks again @anakin78z 👍 👍 |
|
/backport to stable-23.0 |
|
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.) |
Fixes some of the biggest annoyances with answering calls:
Also fixes #5351
🖼️ Screenshots
🏚️ Before

🏡 After

S-View case notification:
🚧 TODO
There are a lot of other issues with calls. This is meant as some small changes that can have a big impact on usability.
🏁 Checklist
/backport to stable-xx.x