Fix critical bugs found in code audit

- MoveStrokesAction.undo(): was offsetting by (0,0), now restores original pre-move point data correctly - Stroke commit flicker: new strokes render incrementally onto existing backing bitmap instead of triggering full redraw - Selection state leak: view-side selection cleared on page navigation via onPageNavigated callback - Initial page fallback: blank canvas prevented if initialPageId not found in DB — falls back to first page - Arrow head Paint: preallocated reusable object, no per-call allocation - Updated PROGRESS.md with audit findings and backup design notes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-24 17:35:06 -07:00
parent b18e77177e
commit ba1571011a
5 changed files with 83 additions and 21 deletions
--- a/PROGRESS.md
+++ b/PROGRESS.md
@@ -129,19 +129,57 @@ Tested on DC-1, identified and fixed rendering issues:
 - **Line snap**: increased threshold to 60pt (~5mm) and switched to max-distance
  tracking to handle EMR stylus hand tremor.

+### Code Audit & Bug Fixes (2026-03-24)
+
+Critical/high-priority fixes applied:
+- **MoveStrokesAction.undo()**: was offsetting by (0,0) instead of restoring
+  original positions. Fixed to write back the pre-move point data.
+- **Stroke commit flicker**: backing bitmap was fully redrawn on each new
+  stroke. Fixed with incremental rendering — new strokes draw directly
+  onto the existing bitmap without clearing.
+- **Selection state leak**: view-side selection wasn't cleared on page
+  navigation. Added `onPageNavigated` callback.
+- **Initial page fallback**: if `initialPageId` isn't found in DB,
+  fall back to first page instead of showing blank canvas.
+- **Arrow head Paint allocation**: preallocated reusable Paint object
+  instead of allocating per drawArrowHeads() call.
+
+Known issues still to address:
+- CopyStrokesAction uses `takeLast(n)` to find new stroke IDs — fragile
+- No mutex guarding concurrent page navigation + paste operations
+- Grid redrawn every frame (could be cached)
+
+## Backup Design (Not Yet Implemented)
+
+Notebook backup as a zip file:
+- Export: `notebook_title.engpad.zip` containing:
+  - `notebook.json` — notebook metadata (title, page_size, page count)
+  - `pages/` directory with one JSON per page:
+    - `pages/001.json` — page metadata + strokes array
+    - Each stroke: `{penSize, color, style, points: [x0,y0,x1,y1,...]}`
+  - Points stored as JSON float arrays (portable, human-readable)
+- Import: parse zip, create notebook + pages + strokes in DB
+- Could be shared via Android share intents like PDF/JPG export
+- Consider: export all notebooks as a single zip for full backup,
+  or individual notebook export for sharing
+
+Alternative: SQLite `VACUUM INTO` for raw DB backup (simpler but not
+portable or human-readable).
+
 ## Status

 All implementation phases complete. Remaining work:
 - Test on Supernote Manta
 - Verify line snap behavior on device
 - Performance profiling with heavily annotated pages
+- Implement notebook backup/restore

 ## Decisions & Deviations

 - **Language**: Kotlin (chosen over Java for better Compose/coroutine support).
 - **Storage**: Room/SQLite with packed float BLOBs for stroke points.
 - **Palm rejection**: Hardware only (EMR digitizer handles it).
- **Pressure sensitivity**: None — two fixed pen sizes (0.38mm, 0.5mm).
+- **Pressure sensitivity**: None — four pen sizes (0.38, 0.5, 0.6, 0.7mm).
 - **Coordinate system**: 300 DPI canonical points (scaled to 72 DPI for PDF export).
 - **UI framework**: Compose for chrome, custom View for canvas (Compose
  Canvas lacks MotionEvent access needed for stylus input).
--- a/app/src/main/kotlin/net/metacircular/engpad/ui/editor/EditorScreen.kt
+++ b/app/src/main/kotlin/net/metacircular/engpad/ui/editor/EditorScreen.kt
@@ -104,6 +104,9 @@ fun EditorScreen(
        viewModel.onSelectionChanged = { ids ->
            canvasView.setSelection(ids)
        }
+        viewModel.onPageNavigated = {
+            canvasView.clearSelection()
+        }
    }

    Column(modifier = Modifier.fillMaxSize()) {
--- a/app/src/main/kotlin/net/metacircular/engpad/ui/editor/EditorViewModel.kt
+++ b/app/src/main/kotlin/net/metacircular/engpad/ui/editor/EditorViewModel.kt
@@ -46,8 +46,9 @@ class EditorViewModel(
    private var clipboard = emptyList<Stroke>()
    private var clipboardIsCut = false

-    // Callback to update the canvas view's selection
+    // Callbacks for view state sync
    var onSelectionChanged: ((Set<Long>) -> Unit)? = null
+    var onPageNavigated: (() -> Unit)? = null

    // Page navigation
    private val _pages = MutableStateFlow<List<Page>>(emptyList())
@@ -69,11 +70,15 @@ class EditorViewModel(
    init {
        viewModelScope.launch {
            loadPages()
-            // Find initial page index
-            val idx = _pages.value.indexOfFirst { it.id == initialPageId }
+            val pages = _pages.value
+            val idx = pages.indexOfFirst { it.id == initialPageId }
            if (idx >= 0) {
                _currentPageIndex.value = idx
-                _currentPageId.value = _pages.value[idx].id
+                _currentPageId.value = pages[idx].id
+            } else if (pages.isNotEmpty()) {
+                // Fallback to first page if initial page not found
+                _currentPageIndex.value = 0
+                _currentPageId.value = pages[0].id
            }
            loadStrokes()
        }
@@ -98,6 +103,7 @@ class EditorViewModel(
            notebookRepository.updateLastPage(notebookId, pages[index].id)
            _canvasState.value = _canvasState.value.copy(zoom = 1f, panX = 0f, panY = 0f)
            clearSelection()
+            onPageNavigated?.invoke() // Clear view-side selection state
            loadStrokes()
            onStrokesChanged?.invoke()
        }
--- a/app/src/main/kotlin/net/metacircular/engpad/ui/editor/PadCanvasView.kt
+++ b/app/src/main/kotlin/net/metacircular/engpad/ui/editor/PadCanvasView.kt
@@ -187,6 +187,13 @@ class PadCanvasView(context: Context) : View(context) {
        isAntiAlias = false
    }

+    private val arrowPaint = Paint().apply {
+        color = Color.BLACK
+        style = Paint.Style.STROKE
+        strokeCap = Paint.Cap.ROUND
+        isAntiAlias = false
+    }
+
    private val dashedLinePaint = Paint().apply {
        color = Color.BLACK
        style = Paint.Style.STROKE
@@ -219,8 +226,22 @@ class PadCanvasView(context: Context) : View(context) {
    fun addCompletedStroke(id: Long, penSize: Float, color: Int, points: FloatArray, style: String = Stroke.STYLE_PLAIN) {
        val path = buildPathFromPoints(points)
        val paint = buildPaint(penSize, color, style)
-        completedStrokes.add(StrokeRender(path, paint, id, style, points))
-        bitmapDirty = true
+        val sr = StrokeRender(path, paint, id, style, points)
+        completedStrokes.add(sr)
+        // Draw incrementally onto existing backing bitmap to avoid full redraw flicker
+        val bc = backingCanvas
+        if (bc != null && backingBitmap != null && !bitmapDirty) {
+            bc.setMatrix(viewMatrix)
+            bc.drawPath(sr.path, sr.paint)
+            if (sr.points != null && sr.points.size >= 4 &&
+                (sr.style == Stroke.STYLE_ARROW || sr.style == Stroke.STYLE_DOUBLE_ARROW)) {
+                drawArrowHeads(bc, sr.points[0], sr.points[1], sr.points[2], sr.points[3],
+                    if (sr.style == Stroke.STYLE_ARROW) LineStyle.ARROW else LineStyle.DOUBLE_ARROW,
+                    sr.paint.strokeWidth)
+            }
+        } else {
+            bitmapDirty = true
+        }
        invalidate()
    }

@@ -707,13 +728,7 @@ class PadCanvasView(context: Context) : View(context) {
    ) {
        if (style != LineStyle.ARROW && style != LineStyle.DOUBLE_ARROW) return

-        val paint = Paint().apply {
-            color = Color.BLACK
-            this.strokeWidth = strokeWidth
-            this.style = Paint.Style.STROKE
-            strokeCap = Paint.Cap.ROUND
-            isAntiAlias = false
-        }
+        arrowPaint.strokeWidth = strokeWidth

        val arrowLen = 40f
        val arrowAngle = Math.toRadians(25.0)
@@ -726,8 +741,8 @@ class PadCanvasView(context: Context) : View(context) {
        val ay1 = y2 - (arrowLen * Math.sin(angle - arrowAngle)).toFloat()
        val ax2 = x2 - (arrowLen * Math.cos(angle + arrowAngle)).toFloat()
        val ay2 = y2 - (arrowLen * Math.sin(angle + arrowAngle)).toFloat()
-        canvas.drawLine(x2, y2, ax1, ay1, paint)
-        canvas.drawLine(x2, y2, ax2, ay2, paint)
+        canvas.drawLine(x2, y2, ax1, ay1, arrowPaint)
+        canvas.drawLine(x2, y2, ax2, ay2, arrowPaint)

        if (style == LineStyle.DOUBLE_ARROW) {
            val revAngle = angle + Math.PI
@@ -735,8 +750,8 @@ class PadCanvasView(context: Context) : View(context) {
            val by1 = y1 - (arrowLen * Math.sin(revAngle - arrowAngle)).toFloat()
            val bx2 = x1 - (arrowLen * Math.cos(revAngle + arrowAngle)).toFloat()
            val by2 = y1 - (arrowLen * Math.sin(revAngle + arrowAngle)).toFloat()
-            canvas.drawLine(x1, y1, bx1, by1, paint)
-            canvas.drawLine(x1, y1, bx2, by2, paint)
+            canvas.drawLine(x1, y1, bx1, by1, arrowPaint)
+            canvas.drawLine(x1, y1, bx2, by2, arrowPaint)
        }
    }

--- a/app/src/main/kotlin/net/metacircular/engpad/undo/SelectionActions.kt
+++ b/app/src/main/kotlin/net/metacircular/engpad/undo/SelectionActions.kt
@@ -43,11 +43,11 @@ class MoveStrokesAction(
    }

    override suspend fun undo() {
-        val movedBack = offsetStrokes(strokes, 0f, 0f) // restore originals
-        for (s in movedBack) {
+        // Restore original positions (strokes has the pre-move data)
+        for (s in strokes) {
            repository.updateStrokePoints(s.id, s.pointData)
        }
-        onUndo(movedBack)
+        onUndo(strokes)
    }

    private fun offsetStrokes(source: List<Stroke>, dx: Float, dy: Float): List<Stroke> {