From f16663db16ca0ce71d977fd97e2659c89db96eec Mon Sep 17 00:00:00 2001 From: yamilmedina Date: Thu, 23 May 2024 18:02:32 +0200 Subject: [PATCH] fix: potential fix for multiple drawing, recompositions --- .../feature/sketch/DrawingCanvasViewModel.kt | 11 ++++--- .../sketch/model/DrawingPathProperties.kt | 2 +- .../feature/sketch/model/DrawingState.kt | 8 +++-- .../sketch/DrawingCanvasViewModelTest.kt | 32 ++++++++++++++++--- 4 files changed, 41 insertions(+), 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 index efd3889aefe..67cbcead3d3 100644 --- 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 index 44e66765510..dab2fda4715 100644 --- 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 index 12de2bae850..5597fc694d3 100644 --- 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 = listOf(), - val pathsUndone: List = listOf(), + val paths: ImmutableList = persistentListOf(), + val pathsUndone: ImmutableList = 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 index c283683861a..d063bcb1896 100644 --- 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() }