Skip to content

Fix issue 3541, Can't complete polygon after getting an interception error popup #3606

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

Open
hassan-nsubuga wants to merge 8 commits into
base: master
Choose a base branch
from
+135 −22
Open

Fix issue 3541, Can't complete polygon after getting an interception error popup #3606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9556c93
Fix isse 3541, reset hasSelfIntersection to false in updateVertices a…
hassan-nsubuga Mar 11, 2026
5cb0e85
Add tests to DrawAreaTaskViewModelTest to convert resetting of hasSel…
hassan-nsubuga Mar 12, 2026
520a374
Bump com.google.maps.android:android-maps-utils from 4.1.0 to 4.1.1 (…
dependabot[bot] Mar 12, 2026
951c359
fix codecov failing dependabot CI (#3612)
andreia-ferreira Mar 13, 2026
6f8ee64
Migrate home screen drawer to Compose (#3554)
gino-m Mar 17, 2026
a2ed615
Refactor DrawAreaTaskViewModel, change showSelfIntersectionDialog an…
hassan-nsubuga Mar 19, 2026
8bc5523
Refactor DrawAreaTaskViewModelTest to add more tests to cover the cha…
hassan-nsubuga Mar 19, 2026
7f56a0f
Merge branch 'master' into bugfix/3541
hassan-nsubuga Mar 19, 2026
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
21 changes: 12 additions & 9 deletions ...n/java/org/groundplatform/android/ui/datacollection/tasks/polygon/DrawAreaTaskFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,15 +89,18 @@ class DrawAreaTaskFragment @Inject constructor() : AbstractTaskFragment<DrawArea
.show()
}
viewLifecycleOwner.lifecycleScope.launch {
viewModel.showSelfIntersectionDialog.collect {
renderComposableDialog {
ConfirmationDialog(
title = R.string.polygon_vertex_add_dialog_title,
description = R.string.polygon_vertex_add_dialog_message,
confirmButtonText = R.string.polygon_vertex_add_dialog_positive_button,
dismissButtonText = null,
onConfirmClicked = {},
)
viewModel.showSelfIntersectionDialog.collect { isVisible ->
if (isVisible) {
renderComposableDialog {
ConfirmationDialog(
title = R.string.polygon_vertex_add_dialog_title,
description = R.string.polygon_vertex_add_dialog_message,
confirmButtonText = R.string.polygon_vertex_add_dialog_positive_button,
dismissButtonText = null,
onConfirmClicked = { viewModel.dismissSelfIntersectionDialog() },
onDismiss = { viewModel.dismissSelfIntersectionDialog() },
)
}
}
}
}
Expand Down
40 changes: 27 additions & 13 deletions .../java/org/groundplatform/android/ui/datacollection/tasks/polygon/DrawAreaTaskViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,25 +139,30 @@ internal constructor(
private val _isTooClose = MutableStateFlow(false)
val isTooClose: StateFlow<Boolean> = _isTooClose.asStateFlow()

private val _showSelfIntersectionDialog = MutableSharedFlow<Unit>()
val showSelfIntersectionDialog = _showSelfIntersectionDialog.asSharedFlow()
private val _showSelfIntersectionDialog = MutableStateFlow(false)
val showSelfIntersectionDialog: StateFlow<Boolean> = _showSelfIntersectionDialog.asStateFlow()

var hasSelfIntersection: Boolean = false
private set
private val _hasSelfIntersection = MutableStateFlow(false)
val hasSelfIntersection: StateFlow<Boolean> = _hasSelfIntersection.asStateFlow()
// var hasSelfIntersection: Boolean = false
// private set
Comment on lines +147 to +148
Copy link
Collaborator

@andreia-ferreira andreia-ferreira Mar 19, 2026

Choose a reason for hiding this comment

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

unused code can be removed


private lateinit var featureStyle: Feature.Style
lateinit var measurementUnits: MeasurementUnits

override val taskActionButtonStates: StateFlow<List<ButtonActionState>> by lazy {
combine(taskTaskData, merge(draftArea, draftUpdates)) { taskData, currentFeature ->
combine(taskTaskData, merge(draftArea, draftUpdates), hasSelfIntersection) {
taskData,
currentFeature,
intersected ->
val isClosed = (currentFeature?.geometry as? LineString)?.isClosed() ?: false
listOfNotNull(
getPreviousButton(),
getSkipButton(taskData),
getUndoButton(taskData, true),
getRedoButton(taskData),
getAddPointButton(isClosed, isTooClose.value),
getCompleteButton(isClosed, isMarkedComplete.value, hasSelfIntersection),
getCompleteButton(isClosed, isMarkedComplete.value, intersected),
getNextButton(taskData).takeIf { isMarkedComplete() },
)
}
Expand Down Expand Up @@ -203,7 +208,12 @@ internal constructor(
@VisibleForTesting fun getLastVertex() = vertices.lastOrNull()

private fun onSelfIntersectionDetected() {
Copy link
Collaborator

@andreia-ferreira andreia-ferreira Mar 19, 2026

Choose a reason for hiding this comment

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

suggestion: not related to this PR, so feel free to resolve this commen, but maybe a clearer name for this method would be showSelfIntersectionDialog

viewModelScope.launch { _showSelfIntersectionDialog.emit(Unit) }
_showSelfIntersectionDialog.value = true
}

fun dismissSelfIntersectionDialog() {
_showSelfIntersectionDialog.value = false
resetHasSelfIntersection()
}

/**
Expand Down Expand Up @@ -319,12 +329,12 @@ internal constructor(
}

private fun checkVertexIntersection(): Boolean {
hasSelfIntersection = isSelfIntersecting(vertices)
if (hasSelfIntersection) {
vertices = vertices.dropLast(1)
_hasSelfIntersection.value = isSelfIntersecting(vertices)
if (_hasSelfIntersection.value) {
updateVertices(vertices.dropLast(1))
onSelfIntersectionDetected()
}
return hasSelfIntersection
return _hasSelfIntersection.value
}

private fun validatePolygonCompletion(): Boolean {
Expand All @@ -339,8 +349,8 @@ internal constructor(
vertices
}

hasSelfIntersection = isSelfIntersecting(ring)
if (hasSelfIntersection) {
_hasSelfIntersection.value = isSelfIntersecting(ring)
if (_hasSelfIntersection.value) {
onSelfIntersectionDetected()
return false
}
Expand All @@ -352,6 +362,10 @@ internal constructor(
refreshMap()
}

fun resetHasSelfIntersection() {
Copy link
Collaborator

@andreia-ferreira andreia-ferreira Mar 19, 2026

Choose a reason for hiding this comment

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

this is only used inside the VM so it can be private

_hasSelfIntersection.value = false
}

@VisibleForTesting
fun completePolygon() {
check(LineString(vertices).isClosed()) { "Polygon is not complete" }
Expand Down
96 changes: 96 additions & 0 deletions ...a/org/groundplatform/android/ui/datacollection/tasks/polygon/DrawAreaTaskViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,16 @@ import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertNotNull
import kotlin.test.assertTrue
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.filterNotNull
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.merge
import kotlinx.coroutines.test.UnconfinedTestDispatcher
import kotlinx.coroutines.test.advanceUntilIdle
import kotlinx.coroutines.test.runTest
import kotlinx.coroutines.test.setMain
import org.groundplatform.android.BaseHiltTest
import org.groundplatform.android.data.local.LocalValueStore
import org.groundplatform.android.model.job.Job
Expand Down Expand Up @@ -72,6 +76,7 @@ class DrawAreaTaskViewModelTest : BaseHiltTest() {

override fun setUp() {
super.setUp()
Dispatchers.setMain(UnconfinedTestDispatcher())
Copy link
Collaborator

@andreia-ferreira andreia-ferreira Mar 19, 2026

Choose a reason for hiding this comment

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

when overriding the main test dispatcher, it should be reset otherwise it may leak into other tests. I suggest adding the following to the test file to prevent it:

  @After
  fun tearDown() {
    Dispatchers.resetMain()
  }

mergedFeatureFlow = merge(viewModel.draftArea.filterNotNull(), viewModel.draftUpdates)

mergedFeatureLiveData = mergedFeatureFlow.asLiveData()
Expand Down Expand Up @@ -620,6 +625,95 @@ class DrawAreaTaskViewModelTest : BaseHiltTest() {
assertThat(viewModel.isMarkedComplete()).isTrue()
}

@Test
fun `checkVertexIntersection sets hasSelfIntersection and showSelfIntersectionDialog to true`() =
runWithTestDispatcher {
setupViewModel()

// Create a path: (0,0) -> (10,10) -> (0,10)
updateLastVertexAndAdd(COORDINATE_1)
updateLastVertexAndAdd(COORDINATE_2)
updateLastVertexAndAdd(COORDINATE_6)

// Move cursor to a point that crosses the first segment: (10,0)
updateLastVertex(COORDINATE_5)

// Trigger ADD_POINT which calls checkVertexIntersection
viewModel.onButtonClick(ButtonAction.ADD_POINT)
advanceUntilIdle()

assertThat(viewModel.hasSelfIntersection.value).isTrue()
assertThat(viewModel.showSelfIntersectionDialog.value).isTrue()
// offending vertex should be dropped
assertGeometry(4, isLineString = true)
}

@Test
fun `dismissSelfIntersectionDialog resets both flag and dialog visibility`() =
runWithTestDispatcher {
setupViewModel()

// Force an intersection state
updateLastVertexAndAdd(COORDINATE_1)
updateLastVertexAndAdd(COORDINATE_2)
updateLastVertexAndAdd(COORDINATE_6)
updateLastVertex(COORDINATE_5)
viewModel.onButtonClick(ButtonAction.ADD_POINT)
advanceUntilIdle()

viewModel.dismissSelfIntersectionDialog()
assertThat(viewModel.hasSelfIntersection.value).isFalse()
assertThat(viewModel.showSelfIntersectionDialog.value).isFalse()
}

@Test
fun `taskActionButtonStates re-enables COMPLETE button after intersection is dismissed`() =
runTest(UnconfinedTestDispatcher()) { // Use Unconfined to trigger emissions immediately
setupViewModel()

viewModel.taskActionButtonStates.test {
// 1. Initial State: Create a closed non-intersecting square
updateLastVertexAndAdd(COORDINATE_1)
updateLastVertexAndAdd(COORDINATE_2)
updateLastVertexAndAdd(COORDINATE_3)
// Set state to be near first vertex to enable COMPLETE
updateLastVertex(COORDINATE_1, isNearFirstVertex = true)

// 2. Trigger Intersection Logic
// Clear state (Simulated)
while (viewModel.getLastVertex() != null) {
viewModel.removeLastVertex()
}

updateLastVertexAndAdd(COORDINATE_1)
updateLastVertexAndAdd(COORDINATE_2)
updateLastVertexAndAdd(COORDINATE_6)
updateLastVertexAndAdd(COORDINATE_5)
updateLastVertex(COORDINATE_1, isNearFirstVertex = true)

// Trigger validation
viewModel.onButtonClick(ButtonAction.COMPLETE)

// 3. Assert: Button should eventually become disabled due to intersection
// We look for the emission where COMPLETE is disabled
expectMostRecentItem().let { latestStates ->
val completeAction = latestStates.find { it.action == ButtonAction.COMPLETE }
assertThat(completeAction?.isEnabled).isFalse()
}

// 4. Act: Dismiss and Verify
viewModel.dismissSelfIntersectionDialog()

// 5. Assert: Button is enabled again
expectMostRecentItem().let { latestStates ->
val completeAction = latestStates.find { it.action == ButtonAction.COMPLETE }
assertThat(completeAction?.isEnabled).isTrue()
}

cancelAndIgnoreRemainingEvents()
}
}

private fun assertGeometry(
expectedVerticesCount: Int,
isLineString: Boolean = false,
Expand Down Expand Up @@ -679,6 +773,8 @@ class DrawAreaTaskViewModelTest : BaseHiltTest() {
private val COORDINATE_2 = Coordinates(10.0, 10.0)
private val COORDINATE_3 = Coordinates(20.0, 20.0)
private val COORDINATE_4 = Coordinates(30.0, 30.0)
private val COORDINATE_5 = Coordinates(10.0, 0.0)
private val COORDINATE_6 = Coordinates(0.0, 10.0)

private val TASK =
Task(
Expand Down
Loading