fix: multiple undo tap for drawing, unnecessary recompositions (WPB-8…

…810) (#3032)
wireapp · May 24, 2024 · f5db657 · f5db657
1 parent b782d08
commit f5db657
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 12 deletions.
diff --git a/features/sketch/src/main/java/com/wire/android/feature/sketch/DrawingCanvasViewModel.kt b/features/sketch/src/main/java/com/wire/android/feature/sketch/DrawingCanvasViewModel.kt
@@ -36,6 +36,8 @@ import androidx.lifecycle.viewModelScope
 import com.wire.android.feature.sketch.model.DrawingMotionEvent
 import com.wire.android.feature.sketch.model.DrawingPathProperties
 import com.wire.android.feature.sketch.model.DrawingState
+import kotlinx.collections.immutable.persistentListOf
+import kotlinx.collections.immutable.toPersistentList
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.withContext
@@ -89,7 +91,7 @@ class DrawingCanvasViewModel : ViewModel() {
      * Stores the initial point of the drawing.
      */
     fun onStartDrawingEvent() {
-        state = state.copy(paths = state.paths + state.currentPath).apply {
+        state = state.copy(paths = (state.paths + state.currentPath).toPersistentList()).apply {
             currentPath.path.moveTo(state.currentPosition.x, state.currentPosition.y)
         }
     }
@@ -111,7 +113,8 @@ class DrawingCanvasViewModel : ViewModel() {
                 strokeWidth = state.currentPath.strokeWidth
                 color = state.currentPath.color
                 drawMode = state.currentPath.drawMode
-            }, pathsUndone = emptyList(),
+            },
+            pathsUndone = persistentListOf(),
             currentPosition = Offset.Unspecified,
             drawingMotionEvent = DrawingMotionEvent.Idle
         )
@@ -130,8 +133,8 @@ class DrawingCanvasViewModel : ViewModel() {
     fun onUndoLastStroke() {
         if (state.paths.isNotEmpty()) {
             state = state.copy(
-                paths = state.paths.dropLast(1),
-                pathsUndone = state.pathsUndone + state.paths.last()
+                paths = state.paths.distinct().dropLast(1).toPersistentList(),
+                pathsUndone = (state.pathsUndone + state.paths.last()).toPersistentList()
             )
         }
     }

diff --git a/features/sketch/src/main/java/com/wire/android/feature/sketch/model/DrawingPathProperties.kt b/features/sketch/src/main/java/com/wire/android/feature/sketch/model/DrawingPathProperties.kt
@@ -38,7 +38,7 @@ import androidx.compose.ui.graphics.toArgb
  * Represents the current path properties [DrawingState.currentPath]
  * This can be extended in the future to [strokeWidth], [drawMode] ,etc.
  */
-internal class DrawingPathProperties(
+internal data class DrawingPathProperties(
     var path: Path = Path(),
     var strokeWidth: Float = 10f,
     var color: Color = Color.Black,

diff --git a/features/sketch/src/main/java/com/wire/android/feature/sketch/model/DrawingState.kt b/features/sketch/src/main/java/com/wire/android/feature/sketch/model/DrawingState.kt
@@ -17,12 +17,16 @@
  */
 package com.wire.android.feature.sketch.model
 
+import androidx.compose.runtime.Stable
 import androidx.compose.ui.geometry.Offset
 import androidx.compose.ui.geometry.Size
+import kotlinx.collections.immutable.ImmutableList
+import kotlinx.collections.immutable.persistentListOf
 
+@Stable
 internal data class DrawingState(
-    val paths: List<DrawingPathProperties> = listOf(),
-    val pathsUndone: List<DrawingPathProperties> = listOf(),
+    val paths: ImmutableList<DrawingPathProperties> = persistentListOf(),
+    val pathsUndone: ImmutableList<DrawingPathProperties> = persistentListOf(),
     val drawingMotionEvent: DrawingMotionEvent = DrawingMotionEvent.Idle,
     val currentPath: DrawingPathProperties = DrawingPathProperties(),
     val currentPosition: Offset = Offset.Unspecified,

diff --git a/features/sketch/src/test/java/com/wire/android/feature/sketch/DrawingCanvasViewModelTest.kt b/features/sketch/src/test/java/com/wire/android/feature/sketch/DrawingCanvasViewModelTest.kt
@@ -69,7 +69,7 @@ class DrawingCanvasViewModelTest {
         assertEquals(viewModel.state.currentPosition, Offset.Unspecified)
 
         // when
-        draw(viewModel)
+        draw(viewModel, MOVED_OFFSET)
 
         // then
         with(viewModel.state) {
@@ -79,6 +79,28 @@ class DrawingCanvasViewModelTest {
         }
     }
 
+    @Test
+    fun givenDrawingEventPersisted_WhenCallingTheUndoAction_ThenUpdateShouldNotHaveDuplicatedPathAndRemoveLast() = runTest {
+        // given
+        val (_, viewModel) = Arrangement().arrange()
+        assertEquals(viewModel.state.currentPosition, Offset.Unspecified)
+
+        // when - then
+        draw(viewModel, MOVED_OFFSET)
+        assertEquals(1, viewModel.state.paths.size)
+        assertEquals(0, viewModel.state.pathsUndone.size)
+
+        // repeated path
+        draw(viewModel, MOVED_OFFSET)
+        assertEquals(2, viewModel.state.paths.size)
+        assertEquals(0, viewModel.state.pathsUndone.size)
+
+        // then
+        viewModel.onUndoLastStroke()
+        assertEquals(0, viewModel.state.paths.size)
+        assertEquals(1, viewModel.state.pathsUndone.size)
+    }
+
     @Test
     fun givenStopDrawingEvent_WhenCallingTheAction_ThenUpdateTheStateWithTheFinalPathPosition() = runTest {
         // given
@@ -139,16 +161,16 @@ class DrawingCanvasViewModelTest {
         }
     }
 
-    private fun stopDrawing(viewModel: DrawingCanvasViewModel) = with(viewModel) {
-        draw(viewModel)
+    private fun stopDrawing(viewModel: DrawingCanvasViewModel, movedOffset: Offset = MOVED_OFFSET) = with(viewModel) {
+        draw(viewModel, movedOffset)
         onStopDrawing()
         onStopDrawingEvent()
     }
 
     // simulates the drawing of strokes
-    private fun draw(viewModel: DrawingCanvasViewModel) = with(viewModel) {
+    private fun draw(viewModel: DrawingCanvasViewModel, movedOffset: Offset = MOVED_OFFSET) = with(viewModel) {
         startDrawing(viewModel)
-        onDraw(MOVED_OFFSET)
+        onDraw(movedOffset)
         onDrawEvent()
     }