Merge pull request #7 from alexdremov/bugs

Coolection of tech debt bugs

Author: alexdremov

app/src/main/java/com/alexdremov/notate/controller/CanvasControllerImpl.kt

@@ -298,7 +298,7 @@ class CanvasControllerImpl(
         if (!selectionManager.hasSelection()) return
 
         val originals = selectionManager.selectedItems.toList()
-        val transform = selectionManager.transformMatrix
+        val transform = selectionManager.getTransform()
 
         val values = FloatArray(9)
         transform.getValues(values)
@@ -359,7 +359,7 @@ class CanvasControllerImpl(
 
         selectionManager.clearSelection()
         selectionManager.selectAll(newSelected)
-        selectionManager.transformMatrix.reset()
+        selectionManager.resetTransform()
 
         runOnUi {
             renderer.invalidate()

app/src/main/java/com/alexdremov/notate/controller/SelectionManager.kt

@@ -3,74 +3,126 @@ package com.alexdremov.notate.controller
 import android.graphics.Matrix
 import android.graphics.RectF
 import com.alexdremov.notate.model.CanvasItem
-import java.util.concurrent.ConcurrentHashMap
 
 /**
  * Manages the state of the active selection.
  * Holds the selected items and the transient transformation matrix.
  * Thread-safe.
  */
 class SelectionManager {
-    private val _selectedItems = ConcurrentHashMap.newKeySet<CanvasItem>()
-    val selectedItems: Set<CanvasItem> get() = _selectedItems
+    private val lock = Any()
+    private val _selectedItems = HashSet<CanvasItem>()
+    val selectedItems: Set<CanvasItem>
+        get() = synchronized(lock) { _selectedItems.toSet() }
 
     // Backwards compatibility for callers expecting Stroke
     val selectedStrokes: Set<com.alexdremov.notate.model.Stroke>
-        get() = _selectedItems.filterIsInstance<com.alexdremov.notate.model.Stroke>().toSet()
+        get() = synchronized(lock) { _selectedItems.filterIsInstance<com.alexdremov.notate.model.Stroke>().toSet() }
 
     // Current transformation applied to the selection (transient)
-    val transformMatrix = Matrix()
+    private val transformMatrix = Matrix()
 
     // Bounding box of the original selection (before transform)
     private val selectionBounds = RectF()
 
+    /**
+     * Returns a defensive copy of the current transformation matrix.
+     *
+     * Note: this allocates a new [Matrix] on every call. For performance-critical,
+     * read-only access that avoids allocations, use [withTransformReadLocked].
+     */
+    fun getTransform(): Matrix {
+        synchronized(lock) {
+            return Matrix(transformMatrix)
+        }
+    }
+
+    /**
+     * Executes [block] while holding the internal lock, providing direct read-only
+     * access to the current transformation matrix without creating a copy.
+     *
+     * Callers must not mutate [Matrix] inside [block]. Violating this contract can
+     * break invariants and thread-safety guarantees of [SelectionManager].
+     */
+    fun <T> withTransformReadLocked(block: (Matrix) -> T): T {
+        synchronized(lock) {
+            return block(transformMatrix)
+        }
+    }
+    fun resetTransform() {
+        synchronized(lock) {
+            transformMatrix.reset()
+        }
+    }
+
     fun select(item: CanvasItem) {
-        _selectedItems.add(item)
-        recomputeBounds()
+        synchronized(lock) {
+            _selectedItems.add(item)
+            recomputeBoundsInternal()
+        }
     }
 
     fun selectAll(items: List<CanvasItem>) {
-        _selectedItems.addAll(items)
-        recomputeBounds()
+        synchronized(lock) {
+            _selectedItems.addAll(items)
+            recomputeBoundsInternal()
+        }
     }
 
     fun deselect(item: CanvasItem) {
-        _selectedItems.remove(item)
-        recomputeBounds()
+        synchronized(lock) {
+            _selectedItems.remove(item)
+            recomputeBoundsInternal()
+        }
     }
 
     fun clearSelection() {
-        _selectedItems.clear()
-        transformMatrix.reset()
-        selectionBounds.setEmpty()
+        synchronized(lock) {
+            _selectedItems.clear()
+            transformMatrix.reset()
+            selectionBounds.setEmpty()
+        }
     }
 
-    fun hasSelection() = _selectedItems.isNotEmpty()
+    fun hasSelection(): Boolean = synchronized(lock) { _selectedItems.isNotEmpty() }
 
-    fun isSelected(item: CanvasItem) = _selectedItems.contains(item)
+    fun isSelected(item: CanvasItem): Boolean = synchronized(lock) { _selectedItems.contains(item) }
 
     private fun recomputeBounds() {
+        synchronized(lock) {
+            recomputeBoundsInternal()
+        }
+    }
+
+    private fun recomputeBoundsInternal() {
         if (_selectedItems.isEmpty()) {
             selectionBounds.setEmpty()
             return
         }
-        val iter = _selectedItems.iterator()
-        if (iter.hasNext()) {
-            selectionBounds.set(iter.next().bounds)
-        }
-        while (iter.hasNext()) {
-            selectionBounds.union(iter.next().bounds)
+
+        val tempBounds = RectF()
+        var first = true
+        for (item in _selectedItems) {
+            if (first) {
+                tempBounds.set(item.bounds)
+                first = false
+            } else {
+                tempBounds.union(item.bounds)
+            }
         }
+        selectionBounds.set(tempBounds)
     }
 
     /**
      * Returns the bounding box of the selection with the current transform applied.
      * Note: This is an AABB (Axis Aligned Bounding Box) of the transformed shape.
      */
     fun getTransformedBounds(): RectF {
-        val r = RectF(selectionBounds)
-        transformMatrix.mapRect(r)
-        return r
+        synchronized(lock) {
+            val r = RectF(selectionBounds)
+            transformMatrix.mapRect(r)
+            return r
+        }
     }
 
     /**
@@ -79,39 +131,47 @@ class SelectionManager {
      * Order: Top-Left, Top-Right, Bottom-Right, Bottom-Left.
      */
     fun getTransformedCorners(): FloatArray {
-        val pts =
-            floatArrayOf(
-                selectionBounds.left,
-                selectionBounds.top, // TL
-                selectionBounds.right,
-                selectionBounds.top, // TR
-                selectionBounds.right,
-                selectionBounds.bottom, // BR
-                selectionBounds.left,
-                selectionBounds.bottom, // BL
-            )
-        transformMatrix.mapPoints(pts)
-        return pts
+        synchronized(lock) {
+            val pts =
+                floatArrayOf(
+                    selectionBounds.left,
+                    selectionBounds.top, // TL
+                    selectionBounds.right,
+                    selectionBounds.top, // TR
+                    selectionBounds.right,
+                    selectionBounds.bottom, // BR
+                    selectionBounds.left,
+                    selectionBounds.bottom, // BL
+                )
+            transformMatrix.mapPoints(pts)
+            return pts
+        }
     }
 
     /**
      * Returns the center of the selection in World coordinates,
      * with the current transform applied.
      */
     fun getSelectionCenter(): FloatArray {
-        val pts = floatArrayOf(selectionBounds.centerX(), selectionBounds.centerY())
-        transformMatrix.mapPoints(pts)
-        return pts
+        synchronized(lock) {
+            val pts = floatArrayOf(selectionBounds.centerX(), selectionBounds.centerY())
+            transformMatrix.mapPoints(pts)
+            return pts
+        }
     }
 
     fun translate(
         dx: Float,
         dy: Float,
     ) {
-        transformMatrix.postTranslate(dx, dy)
+        synchronized(lock) {
+            transformMatrix.postTranslate(dx, dy)
+        }
     }
 
     fun applyTransform(matrix: Matrix) {
-        transformMatrix.postConcat(matrix)
+        synchronized(lock) {
+            transformMatrix.postConcat(matrix)
+        }
     }
 }

app/src/main/java/com/alexdremov/notate/data/CanvasRepository.kt

@@ -52,7 +52,13 @@ class CanvasRepository(
                         wasJson = true
                         Logger.d("CanvasRepository", "Loaded via JSON Fallback")
                     } catch (e2: Exception) {
-                        Logger.w("CanvasRepository", "JSON Fallback failed", e2)
+                        Logger.e("CanvasRepository", "JSON Fallback failed", e2)
+                        val ioException = java.io.IOException(
+                            "Failed to parse canvas data. Protobuf: ${e.message}, JSON: ${e2.message}",
+                            e,
+                        )
+                        ioException.addSuppressed(e2)
+                        throw ioException
                     }
                 }
                 tDecode = System.currentTimeMillis()
@@ -122,18 +128,59 @@ class CanvasRepository(
         bytes: ByteArray,
     ) {
         if (path.startsWith("content://")) {
-            context.contentResolver.openOutputStream(Uri.parse(path), "wt")?.use {
+            val os =
+                context.contentResolver.openOutputStream(Uri.parse(path), "wt")
+                    ?: throw java.io.IOException("Failed to open output stream for SAF path: $path")
+            os.use {
                 it.write(bytes)
             }
         } else {
             // Atomic write for local files
             val targetFile = File(path)
             val tmpFile = File(targetFile.parent, "${targetFile.name}.tmp")
-            tmpFile.writeBytes(bytes)
-            if (targetFile.exists()) {
-                targetFile.delete()
+            val backupFile = File(targetFile.parent, "${targetFile.name}.bak")
+
+            try {
+                tmpFile.writeBytes(bytes)
+
+                if (targetFile.exists()) {
+                    // Create backup
+                    if (backupFile.exists()) backupFile.delete()
+                    if (!targetFile.renameTo(backupFile)) {
+                        throw java.io.IOException("Failed to create backup file: ${backupFile.absolutePath}")
+                    }
+                }
+
+                if (!tmpFile.renameTo(targetFile)) {
+                    // Restore from backup if rename fails
+                    var restoreAttempted = false
+                    var restoreSucceeded = false
+                    if (backupFile.exists()) {
+                        restoreAttempted = true
+                        restoreSucceeded = backupFile.renameTo(targetFile)
+                    }
+                    val restoreMessage = when {
+                        restoreAttempted && !restoreSucceeded ->
+                            " Backup restore also failed; backup may remain at ${backupFile.absolutePath}."
+                        restoreAttempted && restoreSucceeded ->
+                            " Backup restored successfully from ${backupFile.absolutePath}."
+                        else -> ""
+                    }
+                    throw java.io.IOException(
+                        "Failed to rename temp file to: ${targetFile.absolutePath}.$restoreMessage"
+                    )
+                }
+
+                // Success, delete backup
+                if (backupFile.exists()) {
+                    backupFile.delete()
+                }
+            } finally {
+                // Cleanup tmp file if it still exists (e.g. if delete or rename failed)
+                if (tmpFile.exists()) {
+                    tmpFile.delete()
+                }
             }
-            tmpFile.renameTo(targetFile)
         }
     }