From 7d39166b15341eb8f64fff2a2ec3e907459dda87 Mon Sep 17 00:00:00 2001 From: markm39 Date: Fri, 2 Oct 2026 15:30:21 -0500 Subject: [PATCH] fix(selection): drop stale selection indices after undo, redo and clear Selection, object-eraser and transform state stored positions into the stroke list. Undo/redo/clear/load and object erase rebuilt that list without invalidating them, so Delete after Undo underflowed a size_t in deleteSelection and aborted the app with an uncaught std::length_error, and Delete/Move could act on strokes the user never selected. Reset that state wherever the stroke list is rebuilt, cancel an in-flight transform before reverting, keep reserve() bounded by removed strokes, skip empty undo entries, and hide the iOS selection toolbar after history changes. Adds a native regression smoke test run in CI. Release 0.3.5. --- .github/workflows/ci.yml | 3 + CHANGELOG.md | 6 +- cpp/DrawingSelection.cpp | 4 +- cpp/SkiaDrawingEngine.cpp | 17 +- cpp/SkiaDrawingEngine.h | 4 + cpp/SkiaDrawingEngineEraser.cpp | 3 +- cpp/SkiaDrawingEngineSelection.cpp | 12 ++ ios/MobileInkModule/MobileInkCanvasView.swift | 21 +++ package-lock.json | 4 +- package.json | 5 +- scripts/selection_history_smoke.cpp | 175 ++++++++++++++++++ 11 files changed, 237 insertions(+), 17 deletions(-) create mode 100644 scripts/selection_history_smoke.cpp diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7b554ba..7e25365 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,6 +49,9 @@ jobs: - name: Run eraser cursor regression test run: npm run test:native:eraser-smoke + - name: Run selection history regression test + run: npm run test:native:selection-smoke + - name: Build package artifacts run: npm run build diff --git a/CHANGELOG.md b/CHANGELOG.md index dfc47ac..255ac13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,13 @@ All notable changes to `@mathnotes/mobile-ink` will be documented here. -## [Unreleased] +## [0.3.5] - 2026-10-02 +- Fixed a crash when deleting a selection after Undo, Redo or Clear. The selection kept stroke indices from before the history change, and deleting it underflowed a `size_t` and aborted the app with an uncaught `std::length_error`. +- Fixed Delete or Move after Undo acting on strokes the user never selected. Undo, Redo, Clear and loading now drop index-based selection, object-eraser and transform state, and cancel an in-flight selection transform before reverting. +- iOS hides the selection toolbar after Undo, Redo and Clear, so it no longer offers actions on strokes that are gone. - Added unit tests for `normalizePagePayloadForNativeLoad` covering blank, malformed, and valid native-load payloads (#12). +- Added `npm run test:native:selection-smoke` regression coverage, run in CI. ## [0.3.4] - 2026-08-15 diff --git a/cpp/DrawingSelection.cpp b/cpp/DrawingSelection.cpp index dab3350..29450b6 100644 --- a/cpp/DrawingSelection.cpp +++ b/cpp/DrawingSelection.cpp @@ -128,7 +128,7 @@ void DrawingSelection::deleteSelection( size_t newIndex = 0; std::vector remainingStrokes; - remainingStrokes.reserve(strokes.size() - selectedIndices.size()); + remainingStrokes.reserve(strokes.size() - delta.removedStrokes.size()); for (size_t i = 0; i < strokes.size(); i++) { if (selectedIndices.count(i) == 0) { @@ -154,7 +154,7 @@ void DrawingSelection::deleteSelection( strokes = remainingStrokes; selectedIndices.clear(); - if (commit) commit(std::move(delta)); + if (commit && !delta.removedStrokes.empty()) commit(std::move(delta)); } void DrawingSelection::copySelection( diff --git a/cpp/SkiaDrawingEngine.cpp b/cpp/SkiaDrawingEngine.cpp index d9c033d..26d976e 100644 --- a/cpp/SkiaDrawingEngine.cpp +++ b/cpp/SkiaDrawingEngine.cpp @@ -684,6 +684,7 @@ void SkiaDrawingEngine::clear() { // the one operation that genuinely needs an O(N) snapshot to support // undo, but it happens once per clear (not per stroke), so the cost // is bounded. + cancelSelectionTransform(); StrokeDelta delta; delta.kind = StrokeDelta::Kind::Clear; delta.clearedStrokes = strokes_; @@ -691,6 +692,7 @@ void SkiaDrawingEngine::clear() { strokes_.clear(); eraserCircles_.clear(); + resetIndexedSelectionState(); currentPoints_.clear(); currentPath_.reset(); clearActiveShapePreview(); @@ -714,10 +716,12 @@ void SkiaDrawingEngine::undo() { std::lock_guard lock(stateMutex_); if (undoStack_.empty()) return; + cancelSelectionTransform(); StrokeDelta delta = std::move(undoStack_.back()); undoStack_.pop_back(); revertDelta(delta); redoStack_.push_back(std::move(delta)); + resetIndexedSelectionState(); cachedEraserCircleCount_ = 0; bakedCircleCount_ = 0; @@ -731,10 +735,12 @@ void SkiaDrawingEngine::redo() { std::lock_guard lock(stateMutex_); if (redoStack_.empty()) return; + cancelSelectionTransform(); StrokeDelta delta = std::move(redoStack_.back()); redoStack_.pop_back(); applyDelta(delta); undoStack_.push_back(std::move(delta)); + resetIndexedSelectionState(); cachedEraserCircleCount_ = 0; bakedCircleCount_ = 0; @@ -855,18 +861,11 @@ bool SkiaDrawingEngine::deserializeDrawing(const std::vector& data) { return false; } + cancelSelectionTransform(); strokes_ = std::move(loadedStrokes); eraserCircles_.clear(); // Clear eraser circles when loading bakedCircleCount_ = 0; // No circles to bake - selectedIndices_.clear(); - isDraggingSelection_ = false; - hasDragCache_ = false; - selectionOffsetX_ = 0.0f; - selectionOffsetY_ = 0.0f; - dragBackgroundSnapshot_ = nullptr; - nonSelectedSnapshot_ = nullptr; - selectedSnapshot_ = nullptr; - selectionHighlightSnapshot_ = nullptr; + resetIndexedSelectionState(); // Reset history. Loading a serialized notebook is treated as a // checkpoint -- the user wouldn't expect to undo past the load. undoStack_.clear(); diff --git a/cpp/SkiaDrawingEngine.h b/cpp/SkiaDrawingEngine.h index 60e0f08..c9d235b 100644 --- a/cpp/SkiaDrawingEngine.h +++ b/cpp/SkiaDrawingEngine.h @@ -137,6 +137,10 @@ class SkiaDrawingEngine { void commitDelta(StrokeDelta&& delta); void applyDelta(const StrokeDelta& delta); // forward (used by redo) void revertDelta(const StrokeDelta& delta); // backward (used by undo) + // Selection and object-eraser state store positions into strokes_. Call + // whenever strokes_ is rebuilt or compacted (undo/redo/clear/load/object + // erase) so no stale index can address a different or missing stroke. + void resetIndexedSelectionState(); // Pixel-eraser accumulator. During an eraser drag (touchBegan eraser // -> touchMoved... -> touchEnded), applyPixelEraserAt only collects diff --git a/cpp/SkiaDrawingEngineEraser.cpp b/cpp/SkiaDrawingEngineEraser.cpp index 18ce9fa..41a60fc 100644 --- a/cpp/SkiaDrawingEngineEraser.cpp +++ b/cpp/SkiaDrawingEngineEraser.cpp @@ -31,7 +31,7 @@ void SkiaDrawingEngine::eraseObjects() { std::unordered_map oldToNew; size_t newIdx = 0; std::vector remaining; - remaining.reserve(strokes_.size() - indicesToRemove.size()); + remaining.reserve(strokes_.size() - delta.removedStrokes.size()); for (size_t i = 0; i < strokes_.size(); ++i) { if (indicesToRemove.count(i) == 0) { @@ -49,6 +49,7 @@ void SkiaDrawingEngine::eraseObjects() { } if (remaining.size() != strokes_.size()) { strokes_ = remaining; + resetIndexedSelectionState(); commitDelta(std::move(delta)); markStrokeCachesDirty(); } diff --git a/cpp/SkiaDrawingEngineSelection.cpp b/cpp/SkiaDrawingEngineSelection.cpp index 577e463..ed6928a 100644 --- a/cpp/SkiaDrawingEngineSelection.cpp +++ b/cpp/SkiaDrawingEngineSelection.cpp @@ -144,6 +144,18 @@ void SkiaDrawingEngine::clearSelection() { } } +void SkiaDrawingEngine::resetIndexedSelectionState() { + std::lock_guard lock(stateMutex_); + + // Transform state is owned by cancelSelectionTransform(), which callers + // run before rebuilding strokes_ so its originals restore into place. + // clearSelection() is not reused: drag snapshots must be freed even + // when the selection is already empty. + selectedIndices_.clear(); + pendingDeleteIndices_.clear(); + endSelectionDrag(); +} + void SkiaDrawingEngine::deleteSelection() { std::lock_guard lock(stateMutex_); diff --git a/ios/MobileInkModule/MobileInkCanvasView.swift b/ios/MobileInkModule/MobileInkCanvasView.swift index 9f194c0..2939e7d 100644 --- a/ios/MobileInkModule/MobileInkCanvasView.swift +++ b/ios/MobileInkModule/MobileInkCanvasView.swift @@ -995,25 +995,46 @@ class MobileInkCanvasView: MTKView { @objc func clear() { guard let engine = drawingEngine else { return } + let hadSelection = getSelectionCount(engine) > 0 clearCanvas(engine) + endSelectionInteractionAfterHistoryChange(hadSelection: hadSelection) requestDisplay() onDrawingChange?([:]) } @objc func undo() { guard let engine = drawingEngine else { return } + let hadSelection = getSelectionCount(engine) > 0 undoStroke(engine) + endSelectionInteractionAfterHistoryChange(hadSelection: hadSelection) requestDisplay() onDrawingChange?([:]) } @objc func redo() { guard let engine = drawingEngine else { return } + let hadSelection = getSelectionCount(engine) > 0 redoStroke(engine) + endSelectionInteractionAfterHistoryChange(hadSelection: hadSelection) requestDisplay() onDrawingChange?([:]) } + /// Undo, redo and clear rebuild the stroke list, so the engine drops its + /// selection. Mirror that here: stop any in-flight move/transform and hide + /// the selection toolbar so it cannot act on strokes that are gone. + /// An in-progress lasso is kept: it selects against the new strokes on + /// pen-up. + private func endSelectionInteractionAfterHistoryChange(hadSelection: Bool) { + isMovingSelection = false + isTransformingSelection = false + selectionTransformHandleIndex = -1 + hasSelectionMoveDelta = false + if hadSelection { + notifySelectionChange() + } + } + // MARK: - Eraser Cursor private func setupEraserCursor() { diff --git a/package-lock.json b/package-lock.json index da88a84..3915a00 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@mathnotes/mobile-ink", - "version": "0.3.4", + "version": "0.3.5", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@mathnotes/mobile-ink", - "version": "0.3.4", + "version": "0.3.5", "license": "Apache-2.0", "devDependencies": { "@babel/core": "^7.25.2", diff --git a/package.json b/package.json index c121e4c..3a57d9c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@mathnotes/mobile-ink", - "version": "0.3.4", + "version": "0.3.5", "description": "Production-grade React Native ink engine with native Skia drawing and continuous canvas primitives.", "license": "Apache-2.0", "author": "BuilderPro LLC", @@ -42,11 +42,12 @@ "test:release": "node --test scripts/*.test.mjs", "test:native:smoke": "clang++ -std=c++20 scripts/drawing_serialization_smoke.cpp cpp/DrawingTypes.cpp cpp/DrawingSerialization.cpp cpp/ShapeRecognition.cpp -I cpp -I node_modules/@shopify/react-native-skia/cpp/skia -I node_modules/@shopify/react-native-skia/cpp/skia/modules/pathops/include node_modules/@shopify/react-native-skia/libs/apple/libskia.xcframework/macos-arm64_x86_64/libskia.a node_modules/@shopify/react-native-skia/libs/apple/libpathops.xcframework/macos-arm64_x86_64/libpathops.a -framework ApplicationServices -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation -framework QuartzCore -o /tmp/mobile_ink_drawing_serialization_smoke && /tmp/mobile_ink_drawing_serialization_smoke", "test:native:eraser-smoke": "clang++ -std=c++20 scripts/eraser_cursor_smoke.cpp cpp/*.cpp -I cpp -I node_modules/@shopify/react-native-skia/cpp/skia -I node_modules/@shopify/react-native-skia/cpp/skia/modules/pathops/include node_modules/@shopify/react-native-skia/libs/apple/libskia.xcframework/macos-arm64_x86_64/libskia.a node_modules/@shopify/react-native-skia/libs/apple/libpathops.xcframework/macos-arm64_x86_64/libpathops.a -framework ApplicationServices -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation -framework QuartzCore -o /tmp/mobile_ink_eraser_cursor_smoke && /tmp/mobile_ink_eraser_cursor_smoke", + "test:native:selection-smoke": "clang++ -std=c++20 scripts/selection_history_smoke.cpp cpp/*.cpp -I cpp -I node_modules/@shopify/react-native-skia/cpp/skia -I node_modules/@shopify/react-native-skia/cpp/skia/modules/pathops/include node_modules/@shopify/react-native-skia/libs/apple/libskia.xcframework/macos-arm64_x86_64/libskia.a node_modules/@shopify/react-native-skia/libs/apple/libpathops.xcframework/macos-arm64_x86_64/libpathops.a -framework ApplicationServices -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation -framework QuartzCore -o /tmp/mobile_ink_selection_history_smoke && /tmp/mobile_ink_selection_history_smoke", "test:example:typecheck": "npm --prefix example run typecheck", "test:example:export:ios": "npm --prefix example run export:ios", "test:example:export:android": "npm --prefix example run export:android", "pack:dry-run": "npm pack --dry-run", - "validate": "npm run typecheck && npm run test && npm run test:release && npm run test:native:smoke && npm run test:native:eraser-smoke && npm run build && npm run pack:dry-run && npm run test:example:typecheck && npm run test:example:export:ios && npm run test:example:export:android", + "validate": "npm run typecheck && npm run test && npm run test:release && npm run test:native:smoke && npm run test:native:eraser-smoke && npm run test:native:selection-smoke && npm run build && npm run pack:dry-run && npm run test:example:typecheck && npm run test:example:export:ios && npm run test:example:export:android", "prepack": "npm run build" }, "peerDependencies": { diff --git a/scripts/selection_history_smoke.cpp b/scripts/selection_history_smoke.cpp new file mode 100644 index 0000000..24f231b --- /dev/null +++ b/scripts/selection_history_smoke.cpp @@ -0,0 +1,175 @@ +// Regression coverage for selection state across undo/redo/clear. +// +// Selection, the object eraser and selection transforms remember strokes by +// their index in the engine's stroke list. Undo/redo/clear rebuild that list, +// so any index kept across them can point at a different stroke or past the +// end. Shipped builds crashed (uncaught std::length_error from a size_t +// underflow in deleteSelection) after: lasso two strokes -> Undo -> Delete, +// and could delete a stroke the user never selected. These assertions drive +// the shared C++ engine directly (raster surfaces, no GPU context). + +#include + +#include "../cpp/SkiaDrawingEngine.h" + +using namespace nativedrawing; + +namespace { + +int g_failures = 0; + +void check(bool condition, const char* message) { + if (!condition) { + std::cerr << "FAIL: " << message << std::endl; + ++g_failures; + } +} + +// A slightly wavy horizontal stroke, like real pen input (a perfectly flat +// stroke has empty bounds and is skipped by lasso rejection). +void drawStroke(SkiaDrawingEngine& engine, float y) { + engine.setTool("pen"); + engine.touchBegan(100.0f, y, 1.0f); + for (int i = 1; i <= 20; ++i) { + engine.touchMoved(100.0f + i * 10.0f, y + ((i % 2) ? 6.0f : -6.0f), 1.0f); + } + engine.touchEnded(0); +} + +void lasso(SkiaDrawingEngine& engine, float left, float top, float right, float bottom) { + const float corners[5][2] = {{left, top}, {right, top}, {right, bottom}, {left, bottom}, {left, top}}; + engine.setTool("select"); + engine.touchBegan(corners[0][0], corners[0][1], 1.0f); + for (int edge = 0; edge < 4; ++edge) { + for (int step = 1; step <= 40; ++step) { + const float t = step / 40.0f; + engine.touchMoved( + corners[edge][0] + (corners[edge + 1][0] - corners[edge][0]) * t, + corners[edge][1] + (corners[edge + 1][1] - corners[edge][1]) * t, + 1.0f); + } + } + engine.touchEnded(0); +} + +bool strokeAt(SkiaDrawingEngine& engine, float y) { + const bool hit = engine.selectStrokeAt(150.0f, y); + engine.clearSelection(); + return hit; +} + +void undoThenDeleteDoesNotCrash() { + SkiaDrawingEngine engine(820, 1061); + drawStroke(engine, 200.0f); + drawStroke(engine, 260.0f); + lasso(engine, 50.0f, 150.0f, 400.0f, 320.0f); + check(engine.getSelectionCount() == 2, "lasso selects both strokes"); + + engine.undo(); + check(engine.getSelectionCount() == 0, "undo drops the selection"); + + try { + engine.deleteSelection(); + } catch (const std::exception& error) { + std::cerr << "FAIL: deleteSelection threw after undo: " << error.what() << std::endl; + ++g_failures; + } + check(strokeAt(engine, 200.0f), "delete after undo leaves the surviving stroke"); +} + +void undoNeverRetargetsSelection() { + SkiaDrawingEngine engine(820, 1061); + drawStroke(engine, 200.0f); // A + drawStroke(engine, 400.0f); // B + drawStroke(engine, 600.0f); // C + engine.setTool("select"); + engine.selectStrokeAt(150.0f, 200.0f); + engine.deleteSelection(); // delete A + engine.selectStrokeAt(150.0f, 600.0f); // select C (index 1 now) + + engine.undo(); // A returns at index 0; index 1 is now B + check(engine.getSelectionCount() == 0, "undo drops a selection whose indices shifted"); + engine.deleteSelection(); + check(strokeAt(engine, 200.0f) && strokeAt(engine, 400.0f) && strokeAt(engine, 600.0f), + "delete after undo never removes an unselected stroke"); +} + +void redoAndClearDropSelection() { + SkiaDrawingEngine engine(820, 1061); + drawStroke(engine, 200.0f); + drawStroke(engine, 400.0f); + engine.undo(); + engine.selectStrokeAt(150.0f, 200.0f); + check(engine.getSelectionCount() == 1, "stroke selected before redo"); + engine.redo(); + check(engine.getSelectionCount() == 0, "redo drops the selection"); + + engine.selectStrokeAt(150.0f, 200.0f); + engine.clear(); + check(engine.getSelectionCount() == 0, "clear drops the selection"); + engine.deleteSelection(); + engine.undo(); // undo the clear, not a phantom delete + check(strokeAt(engine, 200.0f) && strokeAt(engine, 400.0f), "undo after clear restores every stroke"); +} + +void undoCancelsInFlightTransform() { + SkiaDrawingEngine engine(820, 1061); + drawStroke(engine, 200.0f); + drawStroke(engine, 600.0f); + engine.selectStrokeAt(150.0f, 200.0f); + engine.beginSelectionTransform(0); + engine.updateSelectionTransform(400.0f, 450.0f); + engine.undo(); // removes the second stroke; the uncommitted transform must not leak + check(engine.getSelectionCount() == 0, "undo during a transform drops the selection"); + check(strokeAt(engine, 200.0f), "undo during a transform restores the stroke's original geometry"); + check(!strokeAt(engine, 600.0f), "undo during a transform still reverts the last change"); +} + +void undoDropsPendingObjectErase() { + SkiaDrawingEngine engine(820, 1061); + drawStroke(engine, 200.0f); // A + drawStroke(engine, 400.0f); // B + engine.selectStrokeAt(150.0f, 200.0f); + engine.deleteSelection(); // strokes: [B] + + engine.setToolWithParams("eraser", 20.0f, 0x000000, "object"); + engine.touchBegan(150.0f, 400.0f, 1.0f); + engine.touchMoved(160.0f, 400.0f, 1.0f); // B (index 0) marked for deletion + engine.undo(); // A returns at index 0 + engine.touchEnded(0); + check(strokeAt(engine, 200.0f), "object erase pending across undo never removes another stroke"); +} + +void objectEraseDropsLiveSelection() { + SkiaDrawingEngine engine(820, 1061); + drawStroke(engine, 200.0f); // A + drawStroke(engine, 400.0f); // B + engine.selectStrokeAt(150.0f, 400.0f); // select B (index 1) + + // A host that keeps the selection while object-erasing A shifts B to 0. + engine.setToolWithParams("eraser", 20.0f, 0x000000, "object"); + engine.touchBegan(150.0f, 200.0f, 1.0f); + engine.touchMoved(160.0f, 200.0f, 1.0f); + engine.touchEnded(0); + check(engine.getSelectionCount() == 0, "object erase drops a selection whose indices shifted"); + engine.deleteSelection(); + check(strokeAt(engine, 400.0f), "delete after object erase never removes an unselected stroke"); +} + +} // namespace + +int main() { + undoThenDeleteDoesNotCrash(); + undoNeverRetargetsSelection(); + redoAndClearDropSelection(); + undoCancelsInFlightTransform(); + undoDropsPendingObjectErase(); + objectEraseDropsLiveSelection(); + + if (g_failures == 0) { + std::cout << "Selection history smoke tests passed" << std::endl; + return 0; + } + std::cerr << g_failures << " selection history smoke test(s) failed" << std::endl; + return 1; +}