-
Notifications
You must be signed in to change notification settings - Fork 115
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
[Data Submission] Save timestamp when capturing current location #2719
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2719 +/- ##
============================================
- Coverage 60.91% 60.89% -0.03%
- Complexity 1164 1165 +1
============================================
Files 265 265
Lines 6046 6053 +7
Branches 839 839
============================================
+ Hits 3683 3686 +3
- Misses 1893 1897 +4
Partials 470 470
|
@@ -55,7 +56,7 @@ abstract class AbstractMapFragmentWithControls : AbstractMapContainerFragment() | |||
viewLifecycleOwner.lifecycleScope.launch { | |||
repeatOnLifecycle(Lifecycle.State.STARTED) { | |||
getMapViewModel().location.collect { | |||
val taskData = it?.toCaptureLocationResult() | |||
val taskData = it?.toCaptureLocationResult(timestampMillis = Date().time) |
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.
Why do we save the current time in two places, here and in the CaptureLocationTaskMapFragment
?
@kenstershiro My initial intuition was that we should store timestamp when the user clicks "Capture Location" button, not when the OS gives a location update, which would usually be slightly sooner. The degenerate case would be if the OS stops giving location updates (no GPS signal, location services disabled) - in that case, would we want to capture the time "Capture location" was clicked, or the time the last location was known? Can you ask @lecrabe and @jo-spek?
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.
We wanted the formatted lat/lng for the info card. Removed the dependency of CaptureLocationTaskData
from this class and directly created the formatted location text.
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.
Discussed with @jo-spek
His main concern is that no other task stores a timestamp. Putting this PR on hold until the discussion is finalized.
860fb55
to
371a7c1
Compare
371a7c1
to
6530ec6
Compare
Towards #2717
@gino-m PTAL?