Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make widget web view request system permissions for camera and microphone (PSF-1061) #6149

Merged
merged 14 commits into from
May 31, 2022
Merged

Make widget web view request system permissions for camera and microphone (PSF-1061) #6149

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
59c13bf
Make widget web view request system permissions for camera and microp…
Johennes May 25, 2022
9469027
Add changelog entry
Johennes May 25, 2022
580bbd6
Appease the linter
Johennes May 25, 2022
6ec6d41
Make permission utils instantiable and throw when permission request …
Johennes May 25, 2022
9e084ec
Inject permission utils
Johennes May 25, 2022
92a140b
Add unit tests for filter/map logic
Johennes May 25, 2022
73d7864
Turn test into normal unit test
Johennes May 30, 2022
d757914
Unsuppress lint rule
Johennes May 30, 2022
769b217
Remove unused imports
Johennes May 30, 2022
75da988
Move test to the right folder
Johennes May 30, 2022
5b64946
Remove empty default implementation
Johennes May 30, 2022
70bb2b7
Combine event listener interfaces
Johennes May 30, 2022
3739919
Add missing file
Johennes May 31, 2022
4ebb26d
Merge branch 'develop' into johannes/widget-system-permissions
Johennes May 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions vector/src/main/java/im/vector/app/features/widgets/WidgetFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ class WidgetFragment @Inject constructor() :

private val fragmentArgs: WidgetArgs by args()
private val viewModel: WidgetViewModel by activityViewModel()
private val permissionUtils = WebviewPermissionUtils()
Copy link
Contributor

@ouchadam ouchadam May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want to inject this via dagger (not really needed whilst the WebviewPermissionUtils has no dependencies of its own) we can include the instance via the injectable constructor of the fragment

WidgetFragment @Inject constructor()

becomes...

WidgetFragment @Inject constructor(
  private val permissionUtils: WebviewPermissionUtils
)

and the WebviewPermissionUtils itself will need to be marked as injectable

class WebviewPermissionUtils @Inject constructor()

Fragments in Element are injectable because of the fragment module

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nice! Thanks a lot for explaining. Made the change to be a good citizen and help remember for next time. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

future us will appreciate the injectable boilerplate being done 😄


override fun getBinding(inflater: LayoutInflater, container: ViewGroup?): FragmentRoomWidgetBinding {
return FragmentRoomWidgetBinding.inflate(inflater, container, false)
Expand Down Expand Up @@ -277,16 +278,17 @@ class WidgetFragment @Inject constructor() :
}

private val permissionResultLauncher = registerForActivityResult(ActivityResultContracts.RequestMultiplePermissions()) { result ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we could use existing methods in PermissionTools file to avoid having other logic for permissions handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The registration methods available there, sadly don't expose the map of permission results: https://github.com/vector-im/element-android/blob/4094a66f3ce65d5fa92ac31b7749c731b3f33cf5/vector/src/main/java/im/vector/app/core/utils/PermissionsTools.kt#L67

WebviewPermissionUtils.onPermissionResult(result)
permissionUtils.onPermissionResult(result)
}

override fun onPermissionRequest(request: PermissionRequest) {
WebviewPermissionUtils.promptForPermissions(
permissionUtils.promptForPermissions(
title = R.string.room_widget_resource_permission_title,
request = request,
context = requireContext(),
activity = requireActivity(),
activityResultLauncher = permissionResultLauncher)
activityResultLauncher = permissionResultLauncher
)
}

private fun displayTerms(displayTerms: WidgetViewEvents.DisplayTerms) {
Expand Down
30 changes: 16 additions & 14 deletions vector/src/main/java/im/vector/app/features/widgets/webview/WebviewPermissionUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ import androidx.fragment.app.FragmentActivity
import com.google.android.material.dialog.MaterialAlertDialogBuilder
import im.vector.app.R
import im.vector.app.core.utils.checkPermissions
import java.lang.NullPointerException

object WebviewPermissionUtils {
class WebviewPermissionUtils {

private var permissionRequest: PermissionRequest? = null
Copy link
Contributor

@ouchadam ouchadam May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little weary of including mutable static state in the object instance, would prefer for this class to become instantiable (and then injected where needed) or have the activity/fragment to hold on to the state instead

what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had also thought of this, yeah. I had previously decided against putting the state into the fragment because that way state and logic are split between the fragment and WebviewPermissionUtils which I found harder to reason about. I made the utils into a normal class now and used a private instance variable on the fragment. If this needs to be injectable, I might need some help with that. 🙈

private var selectedPermissions = listOf<String>()
Expand Down Expand Up @@ -73,20 +74,21 @@ object WebviewPermissionUtils {
}

fun onPermissionResult(result: Map<String, Boolean>) {
permissionRequest?.let { request ->
val grantedPermissions = selectedPermissions.filter { webPermission ->
val androidPermission = webPermissionToAndroidPermission(webPermission)
?: return@filter true // No corresponding Android permission exists
return@filter result[androidPermission]
?: return@filter true // Android permission already granted before
}
if (grantedPermissions.isNotEmpty()) {
request.grant(grantedPermissions.toTypedArray())
} else {
request.deny()
}
reset()
if (permissionRequest == null) {
throw NullPointerException("permissionRequest was null! Make sure to call promptForPermissions first.")
Johennes marked this conversation as resolved.
Show resolved Hide resolved
}
val grantedPermissions = selectedPermissions.filter { webPermission ->
val androidPermission = webPermissionToAndroidPermission(webPermission)
?: return@filter true // No corresponding Android permission exists
return@filter result[androidPermission]
?: return@filter true // Android permission already granted before
}
if (grantedPermissions.isNotEmpty()) {
permissionRequest?.grant(grantedPermissions.toTypedArray())
} else {
permissionRequest?.deny()
}
reset()
}

private fun reset() {
Expand Down