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.

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 - Foreground Service #5595

Merged
merged 14 commits into from
Mar 28, 2022

Conversation

onurays
Copy link
Contributor

@onurays onurays commented Mar 21, 2022

We need to start a foreground service and show a sticky notification to be able to share live location if the application is in background and even if it is killed.

  • Start a foreground service when users start sharing live location
  • Show a sticky notification to inform users that the app continues to share location
  • Support sharing location in multiple rooms at the same time
  • We need to set a timer per room and stop foreground service once all times are up
  • Navigate to room list screen when users click on the notification (we will discuss showing room list in which live location is active)
  • Check if we can stop the location manager when the service is destroyed
  • Check if the service is not recreated when users share live location in another room

Live Location Sharing is disabled by a BuildConfig. We are creating small PRs to make them easily reviewable.

live_location_sharing_notification

Onuray Sahin added 8 commits March 18, 2022 15:26
* develop: (152 commits)
  Remove exhaustive.
  fixing the onboarding sanity test failing - adds tapping the new take me home button within the sanity test
  Fix lint issues on weblate sync
  fixing view model tests not collecting flow results - the switch from runBlockingTest to runTest means we need to provide a separate scope from the test in order to asynchronously collect the flow results
  Do not suggest collapse if there is only one section
  Translated using Weblate (Spanish)
  Translated using Weblate (Spanish)
  runBlocking -> runTest https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-test/MIGRATION.md
  runBlockingTest -> runTest https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-test/MIGRATION.md
  Small cleanup
  The `.exhaustive` trick is not needed anymore in Kotlin 1.6.0 https://kotlinlang.org/docs/whatsnew16.html#stable-exhaustive-when-statements-for-enum-sealed-and-boolean-subjects
  Also upgrade the coroutine lib
  Fix compilation warning (exhaustive when)
  Fix compilation warning (exhaustive when)
  Format file (no other change)
  Fix compilation warning (exhaustive when)
  Bump moshi from 1.12.0 to 1.13.0
  Bump kotlin-gradle-plugin from 1.5.31 to 1.6.0
  Code review fixes.
  fixing presence icon anchoring to the middle of the room icon - creates a secondary verification shield and aligns to the start of the room title when presence is present
  ...

# Conflicts:
#	vector/src/main/java/im/vector/app/features/location/LocationSharingFragment.kt
#	vector/src/main/java/im/vector/app/features/location/LocationSharingViewModel.kt
#	vector/src/main/res/values/strings.xml
@onurays onurays marked this pull request as ready for review March 23, 2022 13:30
@onurays onurays requested a review from ganfra March 23, 2022 13:30
@github-actions
Copy link

github-actions bot commented Mar 23, 2022

Unit Test Results

106 files  ±0  106 suites  ±0   1m 21s ⏱️ +13s
188 tests ±0  188 ✔️ ±0  0 💤 ±0  0 ±0 
622 runs  ±0  622 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 9b271e4. ± Comparison against base commit 6d0b823.

♻️ This comment has been updated with latest results.

@onurays onurays requested a review from ouchadam March 24, 2022 09:21
@@ -46,6 +46,7 @@
<uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION" />
<uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" />
<uses-permission android:name="android.permission.ACCESS_BACKGROUND_LOCATION" />
<uses-permission android:name="android.permission.FOREGROUND_SERVICE" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh no! Good catch, done.

@@ -176,6 +179,17 @@ class LocationSharingFragment @Inject constructor(
views.mapView.zoomToLocation(event.userLocation.latitude, event.userLocation.longitude)
}

private fun handleStartLiveLocationService(event: LocationSharingViewEvents.StartLiveLocationService) {
Intent(requireContext(), LocationSharingService::class.java)
.apply {
Copy link
Contributor

Choose a reason for hiding this comment

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

as the putX methods on Intent are builder style, we could avoid the apply

Intent()
  .putExtra(foo, bar)
  .also { ContextCompat.startForegroundService(requireContext(), it) }

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.

Exactly, forgot that puExtra returns Intent. Done.

@@ -221,7 +235,9 @@ class LocationSharingFragment @Inject constructor(
}

private fun startLiveLocationSharing() {
viewModel.handle(LocationSharingAction.StartLiveLocationSharing)
// TODO. Get duration from user
val duration = 30 * 1000L
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth a constant?

not for this PR - thinking out of loud about todos I'm wondering if they are helpful or add noise, would a User can set live location duration ticket that references the class provide the same information 🤔

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 already started implementing time-limiting UI. I will create another quick PR today.


private var hasGpsProviderLiveLocation = false

@RequiresPermission(anyOf = [Manifest.permission.ACCESS_COARSE_LOCATION, Manifest.permission.ACCESS_FINE_LOCATION])
fun start(callback: Callback?) {
fun start() {
Copy link
Contributor

@ouchadam ouchadam Mar 25, 2022

Choose a reason for hiding this comment

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

now that we're calling start from multiple places, do we need to guard the...

locationManager.requestLocationUpdates(
  provider,
  MIN_TIME_TO_UPDATE_LOCATION_MILLIS,
  MIN_DISTANCE_TO_UPDATE_LOCATION_METERS,
  this
)

to avoid registering duplicate updates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is protected internally and doesn't create multiple requests.

Copy link
Contributor

Choose a reason for hiding this comment

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

good to know, thanks!

}

fun addCallback(callback: Callback) {
synchronized(callbacks) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if we only call addCallback/removeCallback from the main thread, we could avoid needing to use synchronized

if we are calling from multiple threads, we'll probably want to make all the callback interactions synchronised 😢

Copy link
Contributor Author

@onurays onurays Mar 25, 2022

Choose a reason for hiding this comment

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

You are right. It is called in a single UI thread. Done.

}

private fun scheduleTimer(roomId: String, durationMillis: Long) {
Timer().schedule(object : TimerTask() {
Copy link
Contributor

Choose a reason for hiding this comment

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

we might also want to keep a reference to the timer and cancel it if it's still running when the service is destroyed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, done.

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

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

LGTM! Would be handy to see a screenshot of the notification in action It wasn't loading properly but now it is 🤦
(and a tiny comment about the timer instance maybe leaking~)

@onurays onurays merged commit 08476a9 into develop Mar 28, 2022
@onurays onurays deleted the feature/ons/live_location_service branch March 28, 2022 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants