Support element call widget (PSG-627) #6616

onurays · 2022-07-21T14:25:38Z

Type of change

Feature
Bugfix
Technical
Other :

Content

We implement Element Call as a widget with some extra functionalities.

Motivation and context

Since this is a call widget we need to make it run in the background, provide a picture-in-picture mode and also preserve the current native WebRTC call implementation for rooms which does not have an active Element Call widget.

This PR is a cherry-pick version of PR #6464 which additionally implements a Bluetooth connection with push-to-talk devices. Since it is an experimental PR with a hardcoded device name and service address without a good UX, we wanted to create this PR to be able to merge into develop.

Screenshots / GIFs

element_call_widget.mp4

Tests

Request an invite (from me?) to the room which has an active Element Call widget
Click voice call button and this should navigate you to the widget
Choose a DM room without EC widget and you should be able to make a regular voice call with the same button

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

Signed-off-by: Johannes Marbach <[email protected]>

mnaturel · 2022-07-21T14:40:51Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/widgets/model/WidgetType.kt

@@ -28,7 +28,8 @@ private val DEFINED_TYPES by lazy {
            WidgetType.StickerPicker,
            WidgetType.Grafana,
            WidgetType.Custom,
-            WidgetType.IntegrationManager
+            WidgetType.IntegrationManager,
+            WidgetType.ElementCall


Should we have a trailing comma for the last item?

Of course, done.

mnaturel · 2022-07-21T14:45:57Z

vector/src/main/java/im/vector/app/core/utils/PermissionsTools.kt

+ * @param request WebView permission request of onPermissionRequest function
+ * @return true if WebView permissions are already granted, false otherwise
+ */
+fun checkWebViewPermissions(activity: Activity, request: PermissionRequest): Boolean {


Maybe we could extract this method into a dedicated use case such as CheckWebViewPermissionsUseCase? This way, it may be possible to inject the ApplicationContext instead of passing the activity as an argument.

We could also add unit tests on it as bonus.

Good idea, done.

mnaturel · 2022-07-21T15:07:12Z

vector/src/main/java/im/vector/app/features/widgets/WidgetAction.kt

@@ -26,4 +26,5 @@ sealed class WidgetAction : VectorViewModelAction {
    object DeleteWidget : WidgetAction()
    object RevokeWidget : WidgetAction()
    object OnTermsReviewed : WidgetAction()
+    object HangupElementCall : WidgetAction()


Could we rename the action CloseWidget to avoid introducing too specific action?

mnaturel · 2022-07-21T15:37:01Z

vector/src/main/java/im/vector/app/features/widgets/WidgetActivity.kt

+    override fun onUserLeaveHint() {
+        super.onUserLeaveHint()
+        val widgetArgs: WidgetArgs? = intent?.extras?.getParcelable(Mavericks.KEY_ARG)
+        if (widgetArgs?.kind == WidgetKind.ELEMENT_CALL) {


I am wondering if we should move if (widgetArgs?.kind == WidgetKind.ELEMENT_CALL) inside the enterPictureInPicture. Like this:

override fun onUserLeaveHint() { super.onUserLeaveHint() enterPictureInPicture() } private fun enterPictureInPicture() { if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { val widgetArgs: WidgetArgs? = intent?.extras?.getParcelable(Mavericks.KEY_ARG) val params = when(widgetArgs?.kind) { WidgetKind.ELEMENT_CALL -> createElementCallPipParams() else -> null } params?.let { enterPictureInPictureMode(it) } } }

This way we could easily add picture in picture for other widget in the future.

Refactored.

mnaturel

Nice work! I have just added some suggestions. I have just one question about the picture in picture mode. Should/can we do something to have a better UI in this mode? Now, it looks like a reduced version of the current larger UI.

mnaturel · 2022-07-22T07:39:06Z

I have also seen there is one security hotspot to be reviewed in the Sonar analysis. It is about registering a broadcast receiver. I think it is worth checking if everything is okay.

mnaturel · 2022-07-22T12:25:26Z

vector/src/test/java/im/vector/app/core/utils/CheckWebViewPermissionsUseCaseTest.kt

+
+    @Before
+    fun setup() {
+        MockKAnnotations.init(this)


I am not sure this is needed. MockKAnnotations.init() is useful when we use the annotations for mockking objects like @MockK.

Yep, I started with using @Mockk then forgot to remove it. Done.

mnaturel · 2022-07-22T12:27:47Z

vector/src/test/java/im/vector/app/core/utils/CheckWebViewPermissionsUseCaseTest.kt

+
+    @Test
+    fun `given an audio permission is granted when the web client requests audio permission then use case returns true`() {
+        val permissionRequest = givenAPermissionRequest(arrayOf(PermissionRequest.RESOURCE_AUDIO_CAPTURE))


To ease the reading in tests, we should separate them in 3 blocks (given, when, then). We would have something like:

val permissionRequest = givenAPermissionRequest(arrayOf(PermissionRequest.RESOURCE_AUDIO_CAPTURE)) every { checkSelfPermission(activity.applicationContext, any()) } returns PackageManager.PERMISSION_GRANTED val result = checkWebViewPermissionsUseCase.execute(activity, permissionRequest) result shouldBe true

mnaturel · 2022-07-22T12:30:51Z

vector/src/test/java/im/vector/app/core/utils/CheckWebViewPermissionsUseCaseTest.kt

+
+        every { checkSelfPermission(activity.applicationContext, any()) } returns PackageManager.PERMISSION_GRANTED
+
+        checkWebViewPermissionsUseCase.execute(activity, permissionRequest) shouldBe true


I think we could add a verify bloc on the call to checkSelfPermission in the tests. For this test for example:

PERMISSIONS_FOR_AUDIO_IP_CALL.forEach { permission -> every { checkSelfPermission(activity.applicationContext, permission) } }

Good idea, done.

onurays · 2022-07-22T14:23:49Z

Nice work! I have just added some suggestions. I have just one question about the picture in picture mode. Should/can we do something to have a better UI in this mode? Now, it looks like a reduced version of the current larger UI.

Web team made a CSS trick to show the mic in the middle of the PIP screen but I think it is not in production yet.

onurays · 2022-07-22T14:25:27Z

I have also seen there is one security hotspot to be reviewed in the Sonar analysis. It is about registering a broadcast receiver. I think it is worth checking if everything is okay.

LocalBroadcastManager is not working with PIP. I think this is not a big issue? We are using it in several places.

mnaturel

Thanks for the updates!

sonarqubecloud · 2022-07-22T15:52:59Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
1 Security Hotspot
2 Code Smells

16.1% Coverage
0.0% Duplication

onurays and others added 15 commits July 21, 2022 12:26

Add element call widget type.

125135c

Create custom widget args for element call.

61f05e7

Add element call widget type.

b3a8052

Support picture-in-picture mode for element call widget.

85aba89

Skip widget permissions for element call.

a0eb2e7

Auto grant WebView permissions if they are already granted system level.

f5ec7a3

Open element call widget.

fd3b082

Suppress webview / checkbox permission dialog

5c55263

Signed-off-by: Johannes Marbach <[email protected]>

Stop javascript for non element call widgets.

9578350

Add a hangup button in pip mode.

85b5713

Fix picture in picture mode.

7dfe526

Show call icon if there is an active element call widget.

0d4697b

Open element call widget.

da780ac

Lint fixes.

81f3e4a

Changelog added.

b389117

onurays requested review from a team and mnaturel and removed request for a team July 21, 2022 14:28

mnaturel reviewed Jul 21, 2022

View reviewed changes

mnaturel reviewed Jul 22, 2022

View reviewed changes

onurays added 4 commits July 22, 2022 14:33

Create use case to check webview permissions.

d631c70

Write test for the use case.

242c14a

Rename widget action.

23a25cf

Code review fix.

792fca8

onurays requested a review from mnaturel July 22, 2022 11:50

mnaturel reviewed Jul 22, 2022

View reviewed changes

Code review fixes.

5c253bb

mnaturel approved these changes Jul 22, 2022

View reviewed changes

onurays merged commit 75de805 into develop Jul 22, 2022

onurays deleted the feature/ons/element_call_widget branch July 22, 2022 16:03

Johennes mentioned this pull request Jul 25, 2022

Put EC permission shortcuts behind labs flag (PSG-630) #6634

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support element call widget (PSG-627) #6616

Support element call widget (PSG-627) #6616

onurays commented Jul 21, 2022 •

edited

Loading

mnaturel Jul 21, 2022

onurays Jul 22, 2022

mnaturel Jul 21, 2022 •

edited

Loading

onurays Jul 22, 2022

mnaturel Jul 21, 2022

onurays Jul 22, 2022

mnaturel Jul 21, 2022

onurays Jul 22, 2022

mnaturel left a comment

mnaturel commented Jul 22, 2022

mnaturel Jul 22, 2022

onurays Jul 22, 2022

mnaturel Jul 22, 2022

onurays Jul 22, 2022

mnaturel Jul 22, 2022

onurays Jul 22, 2022

onurays commented Jul 22, 2022

onurays commented Jul 22, 2022

mnaturel left a comment

sonarqubecloud bot commented Jul 22, 2022


		every { checkSelfPermission(activity.applicationContext, any()) } returns PackageManager.PERMISSION_GRANTED

		checkWebViewPermissionsUseCase.execute(activity, permissionRequest) shouldBe true

Support element call widget (PSG-627) #6616

Support element call widget (PSG-627) #6616

Conversation

onurays commented Jul 21, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

mnaturel commented Jul 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurays commented Jul 22, 2022

onurays commented Jul 22, 2022

mnaturel left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 22, 2022

onurays commented Jul 21, 2022 •

edited

Loading

mnaturel Jul 21, 2022 •

edited

Loading