-
Notifications
You must be signed in to change notification settings - Fork 741
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.
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
Live Location Sharing - Background permission #5565
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just small improvement suggestions we could do. Tested on different Android versions (9, 10, 11).
import android.app.Activity | ||
import im.vector.app.core.utils.openAppSettingsPage | ||
|
||
class DefaultLocationSharingSettingsNavigator constructor(val activity: Activity?) : LocationSharingSettingsNavigator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we name it DefaultLocationSharingNavigator
instead? In fact, it is intended to handle all navigation cases for the location sharing screen and not only for the settings case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, done.
|
||
package im.vector.app.features.location | ||
|
||
interface LocationSharingSettingsNavigator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments about the name, should we rename it to LocationSharingNavigator
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
PERMISSIONS_FOR_BACKGROUND_LOCATION_SHARING, | ||
requireActivity(), | ||
backgroundLocationResultLauncher, | ||
R.string.location_in_background_missing_permission_dialog_content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we could add a .setNegativeButton(R.string.action_not_now, null)
inside the implementation of the checkPermissions
method for the dialog which is displayed when a message should be displayed? Right now the user is forced to go to the permission settings screen and there is no way to disable the dialog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea, done.
override fun goToAppSettings() { | ||
activity?.let { | ||
goingToAppSettings = true | ||
openAppSettingsPage(it) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have seen we can use the ActivityResultLauncher
to go directly to the screen of the related missing permission which would be better than going to the app general settings. I would try to see and test if we can change the implementation of this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact ActivityResultLauncher
does not work as I expected on Android 10 for example. It requests permission as usual with dialog inside the app. So let's keep the current implementation.
Fixes #5536.
Background location permission behavior started to vary with API 29 as follows:
"<10" -> background location permission is automatically granted by the system
"=10" -> The system permission dialog includes "Allow all the time" to grant the background location permission
">10" -> The system permission dialog DOES NOT include "Allow all time". Users need to enable it in settings