Consolidate location capture tasks and add location lock support by anandwana001 · Pull Request #3341 · google/ground-android

anandwana001 · 2025-10-30T10:22:54Z

This PR consolidates the CaptureLocationTask functionality into the existing DropPinTask and adds configurable location lock behavior for both point and area drawing tasks. This reduces code duplication and provides more flexible location capture options based on task configuration.

Key Changes

Merged CaptureLocationTask into DropPinTask
Removed 3 separate CaptureLocationTask files 260 lines of code
Extended DropPinTaskFragment, DropPinTaskViewModel, and DropPinTaskMapFragment to support both modes:
Free placement mode: User can pan/zoom map to drop pin anywhere
GPS-only mode: User can only capture their current device location
Mode is determined by the new allowMovingPoint property on the Task model
Appropriate UI buttons shown based on mode DROP_PIN vs CAPTURE_LOCATION
Different instruction dialogs shown for each mode
Added Location Lock Support for DrawAreaTask
Added allowManualOverride property to Task model for area drawing tasks
When allowManualOverride is false, location lock cannot be disengaged and map gestures are disabled
When allowManualOverride is true default, location lock starts enabled but can be turned off by panning
Added location permission dialog flow for enforced location lock scenarios
Updated Task Model & Conversion
Added two new boolean properties to Task:
allowMovingPoint: Controls whether user can freely place points applies to DROP_PIN tasks
allowManualOverride: Controls whether location lock can be overridden applies to DRAW_AREA tasks
Updated TaskConverter to read these properties from Firebase proto schema
Updated ValueJsonConverter for proper JSON serialization of new properties
Data Submission Updates
Modified SubmitDataUseCase to handle both DROP_PIN and legacy CAPTURE_LOCATION task types
Both task types now submit point geometry data appropriately
Removed Unused Resources
Cleaned up unused string resources across all localization files es, fr, pt, vi
Removed CaptureLocationTaskViewModel from ViewModelModule

Impact

Code reduction: Net reduction of 150 lines of code 456 insertions, 311 deletions
Improved maintainability: Single implementation for point-based location capture
Enhanced flexibility: Task behavior now configurable via proto schema
Backward compatibility: Legacy CAPTURE_LOCATION tasks still supported in data submission

Testing

Updated CaptureLocationTaskFragmentTest to work with consolidated implementation
Updated FakeData to include new task properties

@shobhitagarwal1612 PTAL?

shobhitagarwal1612 · 2025-10-30T10:27:18Z

Please use a better issue title and update description to explain the changes being made and how are the changes verified. Thanks!

…-location

shobhitagarwal1612 · 2025-11-05T04:32:13Z

Can you please attach a screen recording of the new changes?

Also, I'm not able to follow the current PR description. Since the attached issue has a large thread, I would suggest adding the following details to the description.

Final proposed behavior changes
Final proposed UX changes
Will this also include merging the two tasks (CAPTURE_LOCATION & DROP_PIN) into a new task? If yes, what will happen to legacy surveys?

…-location

codecov · 2025-11-08T10:03:32Z

Codecov Report

❌ Patch coverage is 38.04348% with 114 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.35%. Comparing base (912767b) to head (c14b845).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
...tacollection/tasks/polygon/DrawAreaTaskFragment.kt	13.04%	18 Missing and 2 partials ⚠️
...datacollection/tasks/point/DropPinTaskViewModel.kt	52.50%	14 Missing and 5 partials ⚠️
...id/data/local/room/converter/ValueJsonConverter.kt	19.04%	15 Missing and 2 partials ⚠️
.../datacollection/tasks/point/DropPinTaskFragment.kt	50.00%	14 Missing and 3 partials ⚠️
...acollection/tasks/polygon/DrawAreaTaskViewModel.kt	15.00%	16 Missing and 1 partial ⚠️
...ollection/tasks/polygon/DrawAreaTaskMapFragment.kt	0.00%	10 Missing and 1 partial ⚠️
...tacollection/tasks/point/DropPinTaskMapFragment.kt	46.15%	7 Missing ⚠️
...m/android/usecases/submission/SubmitDataUseCase.kt	33.33%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3341      +/-   ##
============================================
- Coverage     67.94%   67.35%   -0.59%     
- Complexity     1518     1523       +5     
============================================
  Files           308      305       -3     
  Lines          8308     8382      +74     
  Branches        921      947      +26     
============================================
+ Hits           5645     5646       +1     
- Misses         2121     2187      +66     
- Partials        542      549       +7

Files with missing lines	Coverage Δ
...droid/data/remote/firebase/schema/TaskConverter.kt	`84.90% <100.00%> (+3.51%)`	⬆️
...java/org/groundplatform/android/model/task/Task.kt	`100.00% <100.00%> (ø)`
...droid/ui/datacollection/DataCollectionViewModel.kt	`79.04% <100.00%> (ø)`
...m/android/usecases/submission/SubmitDataUseCase.kt	`66.66% <33.33%> (-19.05%)`	⬇️
...tacollection/tasks/point/DropPinTaskMapFragment.kt	`27.58% <46.15%> (+15.82%)`	⬆️
...ollection/tasks/polygon/DrawAreaTaskMapFragment.kt	`28.20% <0.00%> (-11.09%)`	⬇️
...id/data/local/room/converter/ValueJsonConverter.kt	`50.74% <19.04%> (-13.41%)`	⬇️
.../datacollection/tasks/point/DropPinTaskFragment.kt	`67.24% <50.00%> (-29.32%)`	⬇️
...acollection/tasks/polygon/DrawAreaTaskViewModel.kt	`74.11% <15.00%> (-7.89%)`	⬇️
...datacollection/tasks/point/DropPinTaskViewModel.kt	`73.23% <52.50%> (-20.70%)`	⬇️
... and 1 more

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…-location # Conflicts: # gradle/libs.versions.toml

shobhitagarwal1612

Can you please attach screen recordings?

Legacy survey, once with capture location, then with drop a pin
New survey, all combinations of alllowMovingPoint and allowManualOverride with drop pin task

app/src/main/java/org/groundplatform/android/data/local/room/converter/ValueJsonConverter.kt

app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/TaskConverter.kt

...rc/main/java/org/groundplatform/android/ui/datacollection/tasks/point/DropPinTaskFragment.kt

…-location

…in-capture-location' into anandwana001/3148/combine-drop-pin-capture-location

shobhitagarwal1612 · 2025-11-24T12:31:53Z

Can you please attach the screen recordings as requested in previous comment?

shobhitagarwal1612 · 2025-11-24T15:21:30Z

@gino-m Can you please take a pass at this PR too?

gino-m

I made it half way through, but overall looking at the PR description I think it's clear this PR is trying to do too many things. Can it be split int two or three smaller targeted PRs please?

gino-m · 2025-11-24T15:25:29Z

app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/TaskConverter.kt

+      val allowMovingPoint = getAllowMovingPoint(taskType, task)
+
+      // Determine if user can manually override location lock for draw area tasks
+      val allowManualOverride = getAllowManualOverride(taskType, task)


Iiuc, this splits the condition into two, allowManualOverride for area, allowMovingPoint for point. Since we're calling both cases "manual location override", can we merge these into one condition, allowManualOverride?

gino-m · 2025-11-24T15:25:52Z

app/src/main/java/org/groundplatform/android/model/task/Task.kt

+   * Whether the user can pan/zoom the map to place a point anywhere (true), or can only capture
+   * their GPS location (false). Only applies to DROP_PIN tasks.
+   */
+  val allowMovingPoint: Boolean = true,


gino-m · 2025-11-24T15:25:58Z

app/src/main/java/org/groundplatform/android/data/remote/firebase/schema/TaskConverter.kt

+   * - For other tasks: Defaults to true (allow moving).
+   */
+  @Suppress("UnusedParameter")
+  private fun getAllowMovingPoint(taskType: Task.Type, task: TaskProto): Boolean =


gino-m · 2025-11-24T15:27:18Z

...rc/main/java/org/groundplatform/android/ui/datacollection/tasks/point/DropPinTaskFragment.kt

+    } else {
+      // Mode: GPS location only
+      addButton(ButtonAction.CAPTURE_LOCATION)
+        .setOnClickListener { viewModel.captureLocation() }


The viewModel entry point and button label can be the same ("Add point" --> addPoint())

gino-m · 2025-11-24T15:28:43Z

...rc/main/java/org/groundplatform/android/ui/datacollection/tasks/point/DropPinTaskFragment.kt

    }
+
+    if (!viewModel.task.allowMovingPoint) {
+      viewModel.enableLocationLock()


We always want to enable location lock to its previous state, even when manual override is allowed.

gino-m · 2025-11-24T15:32:02Z

...c/main/java/org/groundplatform/android/ui/datacollection/tasks/point/DropPinTaskViewModel.kt

+  /** The location lock was not already enabled. */
+  NEEDS_ENABLE,
+
+  /** Trigger to enable the location lock. */


Is this a state or an event? From the ktdoc comments here and above it seems like it might be mixing both?

gino-m · 2025-11-24T15:32:43Z

...c/main/java/org/groundplatform/android/ui/datacollection/tasks/point/DropPinTaskViewModel.kt

+import org.groundplatform.android.ui.map.gms.getAltitudeOrNull
+import org.groundplatform.android.ui.map.gms.toCoordinates
+
+/** Location lock states relevant for attempting to enable it or not. */


Can you please clarify what you mean here?

gino-m · 2025-11-24T15:33:15Z

...c/main/java/org/groundplatform/android/ui/datacollection/tasks/point/DropPinTaskViewModel.kt

+    when (taskData) {
+      is DropPinTaskData -> dropMarker(taskData.location)
+      is CaptureLocationTaskData -> dropMarker(taskData.location)
+      else -> {}


Does this represent an illegal state? If so should we log an error and exit?

…-location

shobhitagarwal1612 · 2025-11-27T04:23:18Z

Given the complexity and the number of touched files, I agree with the recommendation that we split this into smaller, incremental PRs to de-risk regressions.

This PR currently covers a lot of ground, including:

JSON to TaskData conversion.
Schema updates for the Task model.
Merging logic across CaptureLocationTaskFragment, DropPinTaskFragment, and DrawAreaTaskFragment.
Updating TaskType mappings and SubmitDataUseCase logic.

Proposed Strategy: We can introduce a temporary config flag (e.g., unify_capture_location_task) to implement the new logic alongside the old logic without breaking current functionality.

Suggested Rollout Plan:

PR 1: Introduce the unify_capture_location_task boolean and the new bit in the Task model. Introduce unify_capture_location_task and a task bit #3374
PR 2: Implement the merged logic in the Fragments/VMs, guarded by the boolean flag (keeping the old path active).
PR 3: Update SubmitDataUseCase and DataCollection mappings, still respecting the flag.
PR 4: Toggle the flag to true for QA/Verification.
PR 5: (Post-verification) Remove the flag and delete the legacy code/logic.

This allows us to review the logic changes in isolation without worrying about breaking the existing user flow immediately. WDYT?

combine droppin and capturelocation

9e21efb

auto-assign bot requested a review from scolsen October 30, 2025 10:22

anandwana001 changed the title ~~Combine droppin and capturelocation~~ Unify “Add a Point” & “Capture Location” into a Single Task with allowMovingPoint Support Oct 30, 2025

anandwana001 added 5 commits October 30, 2025 17:59

formatting fixes

3e31e57

Merge branch 'master' into anandwana001/3148/combine-drop-pin-capture…

04f8806

…-location

update as per proto

2ac3231

formatting fix

132a4c2

added location lock

6f380a7

anandwana001 added 3 commits November 5, 2025 15:29

Merge branch 'master' into anandwana001/3148/combine-drop-pin-capture…

402b735

…-location

Merge branch 'master' into anandwana001/3148/combine-drop-pin-capture…

92e3042

…-location

nit fix

e88b2b8

anandwana001 changed the title ~~Unify “Add a Point” & “Capture Location” into a Single Task with allowMovingPoint Support~~ Consolidate location capture tasks and add location lock support Nov 8, 2025

anandwana001 added 2 commits November 8, 2025 15:02

test fix

da6b4ef

formatting fix

82e1c83

Merge branch 'master' into anandwana001/3148/combine-drop-pin-capture…

9e2b518

…-location # Conflicts: # gradle/libs.versions.toml

shobhitagarwal1612 requested changes Nov 12, 2025

View reviewed changes

anandwana001 added 7 commits November 12, 2025 14:24

Merge branch 'master' into anandwana001/3148/combine-drop-pin-capture…

8691f07

…-location

Merge branch 'master' into anandwana001/3148/combine-drop-pin-capture…

3a188d9

…-location

Merge branch 'master' into anandwana001/3148/combine-drop-pin-capture…

24ed277

…-location

suggested fix

990dff6

Merge remote-tracking branch 'origin/anandwana001/3148/combine-drop-p…

4a66d2d

…in-capture-location' into anandwana001/3148/combine-drop-pin-capture-location

revert to true

8e0a183

format fix

f1c8604

gino-m removed the request for review from scolsen November 24, 2025 15:22

gino-m requested changes Nov 24, 2025

View reviewed changes

Merge branch 'master' into anandwana001/3148/combine-drop-pin-capture…

c14b845

…-location

This was referenced Dec 1, 2025

Introduce unify_capture_location_task and a task bit #3374

Merged

Added merged logic for Fragment and ViewModel #3375

Closed

anandwana001 closed this Dec 1, 2025

Conversation

anandwana001 commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key Changes

Impact

Testing

Uh oh!

shobhitagarwal1612 commented Oct 30, 2025

Uh oh!

shobhitagarwal1612 commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shobhitagarwal1612 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shobhitagarwal1612 commented Nov 24, 2025

Uh oh!

shobhitagarwal1612 commented Nov 24, 2025

Uh oh!

gino-m left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shobhitagarwal1612 commented Nov 27, 2025 • edited by anandwana001 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

anandwana001 commented Oct 30, 2025 •

edited

Loading

shobhitagarwal1612 commented Nov 5, 2025 •

edited

Loading

codecov bot commented Nov 8, 2025 •

edited

Loading

shobhitagarwal1612 commented Nov 27, 2025 •

edited by anandwana001

Loading