From ba1571011a612cbed541ebfc6354dfb29828263b Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Tue, 24 Mar 2026 17:35:06 -0700 Subject: [PATCH] Fix critical bugs found in code audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- PROGRESS.md | 40 +++++++++++++++++- .../engpad/ui/editor/EditorScreen.kt | 3 ++ .../engpad/ui/editor/EditorViewModel.kt | 14 +++++-- .../engpad/ui/editor/PadCanvasView.kt | 41 +++++++++++++------ .../engpad/undo/SelectionActions.kt | 6 +-- 5 files changed, 83 insertions(+), 21 deletions(-) diff --git a/PROGRESS.md b/PROGRESS.md index 14c1e15..3619be1 100644 --- 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). diff --git a/app/src/main/kotlin/net/metacircular/engpad/ui/editor/EditorScreen.kt b/app/src/main/kotlin/net/metacircular/engpad/ui/editor/EditorScreen.kt index 6a710bd..bd72f9e 100644 --- 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()) { diff --git a/app/src/main/kotlin/net/metacircular/engpad/ui/editor/EditorViewModel.kt b/app/src/main/kotlin/net/metacircular/engpad/ui/editor/EditorViewModel.kt index 45e0097..f1c1651 100644 --- 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() private var clipboardIsCut = false - // Callback to update the canvas view's selection + // Callbacks for view state sync var onSelectionChanged: ((Set) -> Unit)? = null + var onPageNavigated: (() -> Unit)? = null // Page navigation private val _pages = MutableStateFlow>(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() } diff --git a/app/src/main/kotlin/net/metacircular/engpad/ui/editor/PadCanvasView.kt b/app/src/main/kotlin/net/metacircular/engpad/ui/editor/PadCanvasView.kt index 754b7fd..e9d2568 100644 --- 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) } } diff --git a/app/src/main/kotlin/net/metacircular/engpad/undo/SelectionActions.kt b/app/src/main/kotlin/net/metacircular/engpad/undo/SelectionActions.kt index 5f5a19f..724bd77 100644 --- 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, dx: Float, dy: Float): List {