From d6527bf9ec7cb4d782060453ab49f5c408003f81 Mon Sep 17 00:00:00 2001 From: Aziz Becha Date: Sun, 11 Oct 2026 16:57:10 +0100 Subject: [PATCH 1/3] perf: keep document work off the UI thread and cap the iOS brush map getStrokes serialized every point to JSON on the UI thread on both platforms, and setStrokes built a stroke per point there too. Both now run on a worker thread and answer back on the UI thread, the way the image export already did. The Android export executor is shared, so the document commands queue behind it instead of building a second thread. The iOS brush map grew for the whole session and was never pruned, because clear and undo leave entries that undo itself still needs. Keeping the most recent 1000 bounds the memory while leaving undo intact. --- .../java/me/sectiontn/inkpad/InkpadView.kt | 54 +++++++++++----- ios/INKPadView.mm | 63 +++++++++++++++---- 2 files changed, 90 insertions(+), 27 deletions(-) diff --git a/android/src/main/java/me/sectiontn/inkpad/InkpadView.kt b/android/src/main/java/me/sectiontn/inkpad/InkpadView.kt index b637e13..4caf86c 100644 --- a/android/src/main/java/me/sectiontn/inkpad/InkpadView.kt +++ b/android/src/main/java/me/sectiontn/inkpad/InkpadView.kt @@ -256,21 +256,45 @@ class InkpadView(private val reactContext: ThemedReactContext) : FrameLayout(rea } internal fun getStrokes(requestId: Int) { - val doc = Doc(width / density, height / density, strokes.map(InkConverter::fromInk)) - emitResult(requestId, DocumentJson.write(doc)) + val snapshot = strokes.toList() + val canvasWidth = width / density + val canvasHeight = height / density + // A long drawing serializes to a large JSON string, so it belongs off the UI thread like the export. + WORKER.execute { + val result = runCatching { + DocumentJson.write(Doc(canvasWidth, canvasHeight, snapshot.map(InkConverter::fromInk))) + } + post { + result.fold( + onSuccess = { emitResult(requestId, it) }, + onFailure = { error -> + emitError(requestId, ErrorCodes.NATIVE, error.message ?: "could not read the strokes") + }, + ) + } + } } internal fun setStrokes(requestId: Int, json: String?) { - try { - val metrics = resources.displayMetrics - val unitLengthCm = InkConverter.strokeUnitLengthCm(density, metrics.xdpi, metrics.ydpi) - val next = DocumentJson.parse(json ?: "").strokes.map { InkConverter.toInk(it, unitLengthCm) } - replaceAll(next) - emitResult(requestId, "") - } catch (e: InkpadException) { - emitError(requestId, e.code, e.message ?: "") - } catch (e: IllegalArgumentException) { - emitError(requestId, ErrorCodes.NATIVE, e.message ?: "Ink rejected the strokes") + val metrics = resources.displayMetrics + val unitLengthCm = InkConverter.strokeUnitLengthCm(density, metrics.xdpi, metrics.ydpi) + // Loading a saved drawing builds an ink stroke per point, which is as heavy as reading one back. + WORKER.execute { + val result = runCatching { + DocumentJson.parse(json ?: "").strokes.map { InkConverter.toInk(it, unitLengthCm) } + } + post { + result.fold( + onSuccess = { next -> + replaceAll(next) + emitResult(requestId, "") + }, + onFailure = { error -> + val code = if (error is InkpadException) error.code else ErrorCodes.NATIVE + emitError(requestId, code, error.message ?: "Ink rejected the strokes") + }, + ) + } } } @@ -286,7 +310,7 @@ class InkpadView(private val reactContext: ThemedReactContext) : FrameLayout(rea val color = canvasColor val load = imageLoad val fit = backgroundFit - EXPORTER.execute { + WORKER.execute { val image = runCatching { load?.get() }.getOrNull() val result = runCatching { ImageExporter.export(reactContext, snapshot, canvasWidth, canvasHeight, color, image, fit, options) @@ -349,7 +373,9 @@ class InkpadView(private val reactContext: ThemedReactContext) : FrameLayout(rea companion object { private const val ERASER_RADIUS = 10f - private val EXPORTER = Executors.newSingleThreadExecutor() + + // Commands that read or build the whole document share one thread, so they never touch ink at once. + private val WORKER = Executors.newSingleThreadExecutor() private val LOADER = Executors.newCachedThreadPool() } } diff --git a/ios/INKPadView.mm b/ios/INKPadView.mm index ebd2e79..a62bebc 100644 --- a/ios/INKPadView.mm +++ b/ios/INKPadView.mm @@ -18,6 +18,9 @@ using namespace facebook::react; +// Brushes outlive the strokes they belong to, because undo can bring those strokes back. +static const NSUInteger INKBrushLimit = 1000; + @interface INKPadView () @end @@ -48,6 +51,7 @@ @implementation INKPadView { CGFloat _brushSize; BOOL _erasing; NSMutableDictionary *_brushes; + NSMutableArray *_brushOrder; } + (ComponentDescriptorProvider)componentDescriptorProvider @@ -71,6 +75,7 @@ - (instancetype)initWithFrame:(CGRect)frame _brushColor = UIColor.blackColor; _brushSize = 3; _brushes = [NSMutableDictionary new]; + _brushOrder = [NSMutableArray new]; _afterImageLoad = [NSMutableArray new]; _container = [[UIView alloc] initWithFrame:frame]; @@ -245,22 +250,40 @@ - (void)replaceDrawing:(PKDrawing *)drawing - (void)getStrokes:(NSInteger)requestId { - NSString *json = [INKStrokeCodec encodeDrawing:_canvas.drawing size:self.bounds.size brushes:_brushes]; - [self emitResult:requestId ok:YES payload:json errorCode:@"" message:@""]; + PKDrawing *drawing = _canvas.drawing; + NSDictionary *brushes = [_brushes copy]; + CGSize size = self.bounds.size; + __weak INKPadView *weakSelf = self; + // A long drawing serializes to a large JSON string, and exporting already renders off the main thread. + dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ + NSString *json = [INKStrokeCodec encodeDrawing:drawing size:size brushes:brushes]; + dispatch_async(dispatch_get_main_queue(), ^{ + [weakSelf emitResult:requestId ok:YES payload:json errorCode:@"" message:@""]; + }); + }); } - (void)setStrokes:(NSInteger)requestId json:(NSString *)json { - NSString *errorCode = nil; - NSString *message = nil; - INKDecodedDrawing *decoded = [INKStrokeCodec decode:json errorCode:&errorCode message:&message]; - if (decoded == nil) { - [self emitResult:requestId ok:NO payload:@"" errorCode:errorCode message:message]; - return; - } - [_brushes addEntriesFromDictionary:decoded.brushes]; - [self replaceDrawing:decoded.drawing]; - [self emitResult:requestId ok:YES payload:@"" errorCode:@"" message:@""]; + __weak INKPadView *weakSelf = self; + // Loading a saved drawing builds a stroke path per point, which is as heavy as encoding one. + dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ + NSString *errorCode = nil; + NSString *message = nil; + INKDecodedDrawing *decoded = [INKStrokeCodec decode:json errorCode:&errorCode message:&message]; + dispatch_async(dispatch_get_main_queue(), ^{ + INKPadView *view = weakSelf; + if (decoded == nil) { + [view emitResult:requestId ok:NO payload:@"" errorCode:errorCode message:message]; + return; + } + [decoded.brushes enumerateKeysAndObjectsUsingBlock:^(NSDate *created, INKBrushInfo *info, BOOL *stop) { + [view rememberBrush:info forDate:created]; + }]; + [view replaceDrawing:decoded.drawing]; + [view emitResult:requestId ok:YES payload:@"" errorCode:@"" message:@""]; + }); + }); } // A command can arrive before the props JS rendered with it, so the export brings the image it expects. @@ -392,7 +415,21 @@ - (void)rememberNewStrokes info.hasPressure = stylus; info.hasTilt = stylus; info.hasOrientation = stylus; - _brushes[created] = info; + [self rememberBrush:info forDate:created]; + } +} + +// Undo can bring back any stroke that is still in the drawing, so the brushes stay in a bounded window +// instead of growing for the whole session. +- (void)rememberBrush:(INKBrushInfo *)info forDate:(NSDate *)date +{ + if (![_brushOrder.lastObject isEqualToDate:date]) { + [_brushOrder addObject:date]; + } + _brushes[date] = info; + while (_brushOrder.count > INKBrushLimit) { + [_brushes removeObjectForKey:_brushOrder.firstObject]; + [_brushOrder removeObjectAtIndex:0]; } } From a204fc3e9eeb95a31a49a57cc17e587250e25f0f Mon Sep 17 00:00:00 2001 From: Rayen Sbai Date: Fri, 23 Oct 2026 23:26:55 +0100 Subject: [PATCH 2/3] fix(android): run view commands in the order they were sent setStrokes now builds its strokes on a worker thread, so a clear, undo, redo, getStrokes or exportImage sent right after it could run before the load landed. Commands now go through a main-thread queue: they still run at once, and only wait while a setStrokes is loading. Exports go back to their own thread, because they can wait there for the background image and document commands should not wait behind that. Answers post through the main looper, since View.post holds them until a detached view is attached again. Co-authored-by: Aziz Becha --- .../inkpad/InkpadViewCommandsTest.kt | 135 ++++++++++++++++++ .../inkpad/RecordingReactContext.java | 52 +++++++ .../java/me/sectiontn/inkpad/CommandQueue.kt | 24 ++++ .../java/me/sectiontn/inkpad/InkpadView.kt | 107 ++++++++------ .../me/sectiontn/inkpad/CommandQueueTest.kt | 50 +++++++ 5 files changed, 321 insertions(+), 47 deletions(-) create mode 100644 android/src/androidTest/java/me/sectiontn/inkpad/InkpadViewCommandsTest.kt create mode 100644 android/src/androidTest/java/me/sectiontn/inkpad/RecordingReactContext.java create mode 100644 android/src/main/java/me/sectiontn/inkpad/CommandQueue.kt create mode 100644 android/src/test/java/me/sectiontn/inkpad/CommandQueueTest.kt diff --git a/android/src/androidTest/java/me/sectiontn/inkpad/InkpadViewCommandsTest.kt b/android/src/androidTest/java/me/sectiontn/inkpad/InkpadViewCommandsTest.kt new file mode 100644 index 0000000..b499963 --- /dev/null +++ b/android/src/androidTest/java/me/sectiontn/inkpad/InkpadViewCommandsTest.kt @@ -0,0 +1,135 @@ +package me.sectiontn.inkpad + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import com.facebook.react.soloader.OpenSourceMergedSoMapping +import com.facebook.react.uimanager.ThemedReactContext +import com.facebook.soloader.SoLoader +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith + +@RunWith(AndroidJUnit4::class) +class InkpadViewCommandsTest { + private val instrumentation = InstrumentationRegistry.getInstrumentation() + private lateinit var context: RecordingReactContext + private lateinit var view: InkpadView + + @Before + fun createView() { + val base = instrumentation.targetContext + // Events build their payloads with React Native's native maps. + SoLoader.init(base, OpenSourceMergedSoMapping) + context = RecordingReactContext(base) + onMain { + view = InkpadView(ThemedReactContext(context, base, null, -1)) + view.layout(0, 0, 400, 400) + } + } + + @Test + fun loadsStrokesWhileDetached() { + onMain { view.setStrokes(1, document(3)) } + + assertEquals(3, settledStrokeCount()) + } + + @Test + fun clearWaitsForAPendingLoad() { + onMain { + view.setStrokes(1, document(3)) + view.clear() + } + + assertEquals(0, settledStrokeCount()) + } + + @Test + fun undoWaitsForAPendingLoad() { + onMain { view.setStrokes(1, document(2)) } + settledStrokeCount() + onMain { + view.setStrokes(2, document(5)) + view.undo() + } + + assertEquals(2, settledStrokeCount()) + } + + @Test + fun getStrokesWaitsForAPendingLoad() { + onMain { + view.setStrokes(1, document(4)) + view.getStrokes(2) + } + settle() + + assertEquals(4, DocumentJson.parse(payload(2)).strokes.size) + } + + @Test + fun appliesLoadsInTheOrderTheyWereSent() { + onMain { + view.setStrokes(1, document(200)) + view.setStrokes(2, document(1)) + } + + assertEquals(1, settledStrokeCount()) + } + + @Test + fun runsCommandsSentAfterALoadThatFails() { + onMain { view.setStrokes(1, document(3)) } + settledStrokeCount() + onMain { + view.setStrokes(2, "not a document") + view.clear() + } + + assertEquals(0, settledStrokeCount()) + } + + private fun onMain(block: () -> Unit) = instrumentation.runOnMainSync(block) + + // Two rounds, since a load that lands on the main thread can start a save on the document thread. + private fun settle() = repeat(2) { + InkpadView.WORKER.submit {}.get() + instrumentation.waitForIdleSync() + } + + private fun settledStrokeCount(): Int { + settle() + var count = -1 + onMain { count = view.strokes.size } + return count + } + + private fun payload(requestId: Int): String { + var payload: String? = null + onMain { + payload = context.events + .firstOrNull { it.hasKey("requestId") && it.getInt("requestId") == requestId } + ?.getString("payload") + } + return checkNotNull(payload) { "no answer to request $requestId" } + } + + private fun document(strokes: Int): String = DocumentJson.write( + Doc( + 400f, + 400f, + List(strokes) { s -> + val points = FloatArray(20 * 6) { k -> + when (k % 6) { + 0 -> 20f + (k / 6) * 5 + 1 -> 20f + s * 30 + 2 -> (k / 6) * 8f + else -> -1f + } + } + DocStroke(BrushType.PEN, 0xFF000000.toInt(), 3f, InputType.TOUCH, points) + }, + ), + ) +} diff --git a/android/src/androidTest/java/me/sectiontn/inkpad/RecordingReactContext.java b/android/src/androidTest/java/me/sectiontn/inkpad/RecordingReactContext.java new file mode 100644 index 0000000..081560d --- /dev/null +++ b/android/src/androidTest/java/me/sectiontn/inkpad/RecordingReactContext.java @@ -0,0 +1,52 @@ +package me.sectiontn.inkpad; + +import android.content.Context; +import com.facebook.react.bridge.BridgeReactContext; +import com.facebook.react.bridge.ReadableMap; +import com.facebook.react.uimanager.events.BatchEventDispatchedListener; +import com.facebook.react.uimanager.events.Event; +import com.facebook.react.uimanager.events.EventDispatcher; +import com.facebook.react.uimanager.events.EventDispatcherListener; +import com.facebook.react.uimanager.events.EventDispatcherProvider; +import java.util.ArrayList; +import java.util.List; + +// Keeps each event payload. Java, so it can implement React Native's internal provider and read protected event data. +class RecordingReactContext extends BridgeReactContext implements EventDispatcherProvider { + final List events = new ArrayList<>(); + + private final EventDispatcher dispatcher = + new EventDispatcher() { + @Override + public void dispatchEvent(Event event) { + if (event instanceof InkEvent) events.add(((InkEvent) event).getEventData()); + } + + @Override + public void dispatchAllEvents() {} + + @Override + public void addListener(EventDispatcherListener listener) {} + + @Override + public void removeListener(EventDispatcherListener listener) {} + + @Override + public void addBatchEventDispatchedListener(BatchEventDispatchedListener listener) {} + + @Override + public void removeBatchEventDispatchedListener(BatchEventDispatchedListener listener) {} + + @Override + public void onCatalystInstanceDestroyed() {} + }; + + RecordingReactContext(Context context) { + super(context); + } + + @Override + public EventDispatcher getEventDispatcher() { + return dispatcher; + } +} diff --git a/android/src/main/java/me/sectiontn/inkpad/CommandQueue.kt b/android/src/main/java/me/sectiontn/inkpad/CommandQueue.kt new file mode 100644 index 0000000..6955867 --- /dev/null +++ b/android/src/main/java/me/sectiontn/inkpad/CommandQueue.kt @@ -0,0 +1,24 @@ +package me.sectiontn.inkpad + +// Runs view commands in the order JS sent them while some finish on another thread. Main thread only. +internal class CommandQueue { + class Slot internal constructor() { + internal var action: (() -> Unit)? = null + } + + private val slots = ArrayDeque() + + fun run(action: () -> Unit) = fill(reserve(), action) + + // Holds a place for a command that finishes later, so the ones sent after it wait. + fun reserve(): Slot = Slot().also(slots::addLast) + + fun fill(slot: Slot, action: () -> Unit) { + slot.action = action + while (true) { + val next = slots.firstOrNull()?.action ?: return + slots.removeFirst() + next() + } + } +} diff --git a/android/src/main/java/me/sectiontn/inkpad/InkpadView.kt b/android/src/main/java/me/sectiontn/inkpad/InkpadView.kt index 4caf86c..a6576a7 100644 --- a/android/src/main/java/me/sectiontn/inkpad/InkpadView.kt +++ b/android/src/main/java/me/sectiontn/inkpad/InkpadView.kt @@ -6,6 +6,8 @@ import android.graphics.Canvas import android.graphics.Color import android.graphics.Matrix import android.os.Build +import android.os.Handler +import android.os.Looper import android.view.MotionEvent import android.widget.FrameLayout import androidx.ink.authoring.InProgressStrokeId @@ -29,6 +31,8 @@ import java.util.concurrent.FutureTask @SuppressLint("ViewConstructor") class InkpadView(private val reactContext: ThemedReactContext) : FrameLayout(reactContext) { private val density = resources.displayMetrics.density + // View.post holds a runnable until a detached view is attached again, so answers use the main looper. + private val main = Handler(Looper.getMainLooper()) private val strokesView = StrokesView(reactContext) private val inProgressView = InProgressStrokesView(reactContext) private val motionToCanvas = Matrix().apply { setScale(1f / density, 1f / density) } @@ -38,6 +42,7 @@ class InkpadView(private val reactContext: ThemedReactContext) : FrameLayout(rea internal val strokes = mutableListOf() private val history = StrokeHistory() + private val commands = CommandQueue() internal var brushType = BrushType.PEN internal var brushColor = Color.BLACK @@ -244,33 +249,35 @@ class InkpadView(private val reactContext: ThemedReactContext) : FrameLayout(rea } internal fun undo() { - if (history.undo(strokes)) changed() + commands.run { if (history.undo(strokes)) changed() } } internal fun redo() { - if (history.redo(strokes)) changed() + commands.run { if (history.redo(strokes)) changed() } } internal fun clear() { - if (strokes.isNotEmpty()) replaceAll(emptyList()) + commands.run { if (strokes.isNotEmpty()) replaceAll(emptyList()) } } internal fun getStrokes(requestId: Int) { - val snapshot = strokes.toList() - val canvasWidth = width / density - val canvasHeight = height / density - // A long drawing serializes to a large JSON string, so it belongs off the UI thread like the export. - WORKER.execute { - val result = runCatching { - DocumentJson.write(Doc(canvasWidth, canvasHeight, snapshot.map(InkConverter::fromInk))) - } - post { - result.fold( - onSuccess = { emitResult(requestId, it) }, - onFailure = { error -> - emitError(requestId, ErrorCodes.NATIVE, error.message ?: "could not read the strokes") - }, - ) + commands.run { + val snapshot = strokes.toList() + val canvasWidth = width / density + val canvasHeight = height / density + // A long drawing serializes to a large JSON string, so it belongs off the UI thread like the export. + WORKER.execute { + val result = runCatching { + DocumentJson.write(Doc(canvasWidth, canvasHeight, snapshot.map(InkConverter::fromInk))) + } + main.post { + result.fold( + onSuccess = { emitResult(requestId, it) }, + onFailure = { error -> + emitError(requestId, ErrorCodes.NATIVE, error.message ?: "could not read the strokes") + }, + ) + } } } } @@ -278,22 +285,25 @@ class InkpadView(private val reactContext: ThemedReactContext) : FrameLayout(rea internal fun setStrokes(requestId: Int, json: String?) { val metrics = resources.displayMetrics val unitLengthCm = InkConverter.strokeUnitLengthCm(density, metrics.xdpi, metrics.ydpi) + val slot = commands.reserve() // Loading a saved drawing builds an ink stroke per point, which is as heavy as reading one back. WORKER.execute { val result = runCatching { DocumentJson.parse(json ?: "").strokes.map { InkConverter.toInk(it, unitLengthCm) } } - post { - result.fold( - onSuccess = { next -> - replaceAll(next) - emitResult(requestId, "") - }, - onFailure = { error -> - val code = if (error is InkpadException) error.code else ErrorCodes.NATIVE - emitError(requestId, code, error.message ?: "Ink rejected the strokes") - }, - ) + main.post { + commands.fill(slot) { + result.fold( + onSuccess = { next -> + replaceAll(next) + emitResult(requestId, "") + }, + onFailure = { error -> + val code = if (error is InkpadException) error.code else ErrorCodes.NATIVE + emitError(requestId, code, error.message ?: "Ink rejected the strokes") + }, + ) + } } } } @@ -304,25 +314,27 @@ class InkpadView(private val reactContext: ThemedReactContext) : FrameLayout(rea options.backgroundColor?.let { canvasColor = it } options.backgroundImageUri?.let { backgroundImageUri = it } options.backgroundFit?.let { backgroundFit = it } - val snapshot = strokes.toList() - val canvasWidth = width / density - val canvasHeight = height / density val color = canvasColor val load = imageLoad val fit = backgroundFit - WORKER.execute { - val image = runCatching { load?.get() }.getOrNull() - val result = runCatching { - ImageExporter.export(reactContext, snapshot, canvasWidth, canvasHeight, color, image, fit, options) - } - post { - result.fold( - onSuccess = { emitResult(requestId, it) }, - onFailure = { error -> - val code = (error as? InkpadException)?.code ?: ErrorCodes.EXPORT_FAILED - emitError(requestId, code, error.message ?: "export failed") - }, - ) + commands.run { + val snapshot = strokes.toList() + val canvasWidth = width / density + val canvasHeight = height / density + EXPORTER.execute { + val image = runCatching { load?.get() }.getOrNull() + val result = runCatching { + ImageExporter.export(reactContext, snapshot, canvasWidth, canvasHeight, color, image, fit, options) + } + main.post { + result.fold( + onSuccess = { emitResult(requestId, it) }, + onFailure = { error -> + val code = (error as? InkpadException)?.code ?: ErrorCodes.EXPORT_FAILED + emitError(requestId, code, error.message ?: "export failed") + }, + ) + } } } } @@ -374,8 +386,9 @@ class InkpadView(private val reactContext: ThemedReactContext) : FrameLayout(rea companion object { private const val ERASER_RADIUS = 10f - // Commands that read or build the whole document share one thread, so they never touch ink at once. - private val WORKER = Executors.newSingleThreadExecutor() + // Saves and loads documents. Exports have their own thread, since they can wait on the background image. + internal val WORKER = Executors.newSingleThreadExecutor() + private val EXPORTER = Executors.newSingleThreadExecutor() private val LOADER = Executors.newCachedThreadPool() } } diff --git a/android/src/test/java/me/sectiontn/inkpad/CommandQueueTest.kt b/android/src/test/java/me/sectiontn/inkpad/CommandQueueTest.kt new file mode 100644 index 0000000..bcc5a15 --- /dev/null +++ b/android/src/test/java/me/sectiontn/inkpad/CommandQueueTest.kt @@ -0,0 +1,50 @@ +package me.sectiontn.inkpad + +import org.junit.Assert.assertEquals +import org.junit.Test + +class CommandQueueTest { + private val log = mutableListOf() + private val queue = CommandQueue() + + @Test + fun runsACommandRightAwayWhenNothingWaits() { + queue.run { log += "clear" } + + assertEquals(listOf("clear"), log) + } + + @Test + fun holdsCommandsBehindAReservedSlot() { + val load = queue.reserve() + queue.run { log += "clear" } + assertEquals(emptyList(), log) + + queue.fill(load) { log += "load" } + + assertEquals(listOf("load", "clear"), log) + } + + @Test + fun appliesLoadsInTheOrderTheyWereSentWhenTheyFinishOutOfOrder() { + val first = queue.reserve() + queue.run { log += "clear" } + val second = queue.reserve() + queue.run { log += "undo" } + + queue.fill(second) { log += "second" } + assertEquals(emptyList(), log) + queue.fill(first) { log += "first" } + + assertEquals(listOf("first", "clear", "second", "undo"), log) + } + + @Test + fun runsCommandsRightAwayAgainOnceTheQueueDrains() { + val load = queue.reserve() + queue.fill(load) { log += "load" } + queue.run { log += "redo" } + + assertEquals(listOf("load", "redo"), log) + } +} From a0e5154d8f88877e2c272cce5d1d60b5a1be296a Mon Sep 17 00:00:00 2001 From: Rayen Sbai Date: Fri, 23 Oct 2026 23:30:57 +0100 Subject: [PATCH 3/3] fix(ios): run view commands in the order they were sent setStrokes decoded on a concurrent global queue, so two loads could land out of order, and a clear, undo, redo, getStrokes or exportImage sent after a load could run before it. Document work now runs on a private serial queue, and commands go through a main-thread queue that only waits while a load is pending. An export that waits for the background image keeps the drawing it was asked for. The brush map drops its 1000-entry cap. An entry is about 116 bytes while undo history holds about 4.8 KB per 150-point stroke, and the cap gave evicted strokes the current brush on save. Co-authored-by: Aziz Becha --- .github/workflows/ci.yml | 4 +- ios/INKCommandQueue.h | 20 ++++ ios/INKCommandQueue.m | 44 ++++++++ ios/INKPadView.mm | 120 ++++++++++++--------- src/__tests__/iosCommandQueue.test.ts | 32 ++++++ src/__tests__/native/command_queue_check.m | 55 ++++++++++ 6 files changed, 220 insertions(+), 55 deletions(-) create mode 100644 ios/INKCommandQueue.h create mode 100644 ios/INKCommandQueue.m create mode 100644 src/__tests__/iosCommandQueue.test.ts create mode 100644 src/__tests__/native/command_queue_check.m diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 345e4d0..a831a6b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -140,10 +140,10 @@ jobs: npx agent-device prepare ios-runner --platform ios --udid "$udid" --timeout 900000 npx agent-device daemon stop - - name: Stroke codec on the simulator + - name: Stroke codec and command queue env: INKPAD_IOS_SIMULATOR: ${{ steps.simulator.outputs.udid }} - run: yarn vitest run src/__tests__/iosCodec.test.ts + run: yarn vitest run src/__tests__/iosCodec.test.ts src/__tests__/iosCommandQueue.test.ts - name: End-to-end tests env: diff --git a/ios/INKCommandQueue.h b/ios/INKCommandQueue.h new file mode 100644 index 0000000..65c8b96 --- /dev/null +++ b/ios/INKCommandQueue.h @@ -0,0 +1,20 @@ +#import + +NS_ASSUME_NONNULL_BEGIN + +@interface INKCommandSlot : NSObject +@end + +// Runs view commands in the order JS sent them while some finish on another thread. Main thread only. +@interface INKCommandQueue : NSObject + +- (void)run:(dispatch_block_t)action; + +// Holds a place for a command that finishes later, so the ones sent after it wait. +- (INKCommandSlot *)reserve; + +- (void)fill:(INKCommandSlot *)slot action:(dispatch_block_t)action; + +@end + +NS_ASSUME_NONNULL_END diff --git a/ios/INKCommandQueue.m b/ios/INKCommandQueue.m new file mode 100644 index 0000000..d7d2822 --- /dev/null +++ b/ios/INKCommandQueue.m @@ -0,0 +1,44 @@ +#import "INKCommandQueue.h" + +@interface INKCommandSlot () +@property (nonatomic, copy, nullable) dispatch_block_t action; +@end + +@implementation INKCommandSlot +@end + +@implementation INKCommandQueue { + NSMutableArray *_slots; +} + +- (instancetype)init +{ + if (self = [super init]) { + _slots = [NSMutableArray new]; + } + return self; +} + +- (void)run:(dispatch_block_t)action +{ + [self fill:[self reserve] action:action]; +} + +- (INKCommandSlot *)reserve +{ + INKCommandSlot *slot = [INKCommandSlot new]; + [_slots addObject:slot]; + return slot; +} + +- (void)fill:(INKCommandSlot *)slot action:(dispatch_block_t)action +{ + slot.action = action; + while (_slots.firstObject.action != nil) { + dispatch_block_t next = _slots.firstObject.action; + [_slots removeObjectAtIndex:0]; + next(); + } +} + +@end diff --git a/ios/INKPadView.mm b/ios/INKPadView.mm index a62bebc..3af0668 100644 --- a/ios/INKPadView.mm +++ b/ios/INKPadView.mm @@ -12,14 +12,25 @@ #import "INKBrushes.h" #import "INKCanvasView.h" #import "INKColor.h" +#import "INKCommandQueue.h" #import "INKImageExporter.h" #import "INKStrokeCodec.h" #import "RCTFabricComponentsPlugins.h" using namespace facebook::react; -// Brushes outlive the strokes they belong to, because undo can bring those strokes back. -static const NSUInteger INKBrushLimit = 1000; +// Saving and loading run here one at a time, off the main thread. +static dispatch_queue_t INKDocumentQueue(void) +{ + static dispatch_queue_t queue; + static dispatch_once_t once; + dispatch_once(&once, ^{ + queue = dispatch_queue_create( + "me.sectiontn.inkpad.document", + dispatch_queue_attr_make_with_qos_class(DISPATCH_QUEUE_SERIAL, QOS_CLASS_USER_INITIATED, 0)); + }); + return queue; +} @interface INKPadView () @end @@ -50,8 +61,9 @@ @implementation INKPadView { UIColor *_brushColor; CGFloat _brushSize; BOOL _erasing; + // Kept for the view's lifetime, since undo can bring back any stroke. NSMutableDictionary *_brushes; - NSMutableArray *_brushOrder; + INKCommandQueue *_commands; } + (ComponentDescriptorProvider)componentDescriptorProvider @@ -75,7 +87,7 @@ - (instancetype)initWithFrame:(CGRect)frame _brushColor = UIColor.blackColor; _brushSize = 3; _brushes = [NSMutableDictionary new]; - _brushOrder = [NSMutableArray new]; + _commands = [INKCommandQueue new]; _afterImageLoad = [NSMutableArray new]; _container = [[UIView alloc] initWithFrame:frame]; @@ -218,23 +230,29 @@ - (void)handleCommand:(const NSString *)commandName args:(const NSArray *)args - (void)undo { - if (_canvas.undoManager.canUndo) { - [_canvas.undoManager undo]; - } + [_commands run:^{ + if (self->_canvas.undoManager.canUndo) { + [self->_canvas.undoManager undo]; + } + }]; } - (void)redo { - if (_canvas.undoManager.canRedo) { - [_canvas.undoManager redo]; - } + [_commands run:^{ + if (self->_canvas.undoManager.canRedo) { + [self->_canvas.undoManager redo]; + } + }]; } - (void)clear { - if (_canvas.drawing.strokes.count > 0) { - [self replaceDrawing:[PKDrawing new]]; - } + [_commands run:^{ + if (self->_canvas.drawing.strokes.count > 0) { + [self replaceDrawing:[PKDrawing new]]; + } + }]; } // Registering the reverse inside the undo handler is what makes redo work. @@ -250,38 +268,40 @@ - (void)replaceDrawing:(PKDrawing *)drawing - (void)getStrokes:(NSInteger)requestId { - PKDrawing *drawing = _canvas.drawing; - NSDictionary *brushes = [_brushes copy]; - CGSize size = self.bounds.size; - __weak INKPadView *weakSelf = self; - // A long drawing serializes to a large JSON string, and exporting already renders off the main thread. - dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ - NSString *json = [INKStrokeCodec encodeDrawing:drawing size:size brushes:brushes]; - dispatch_async(dispatch_get_main_queue(), ^{ - [weakSelf emitResult:requestId ok:YES payload:json errorCode:@"" message:@""]; + [_commands run:^{ + PKDrawing *drawing = self->_canvas.drawing; + NSDictionary *brushes = [self->_brushes copy]; + CGSize size = self.bounds.size; + __weak INKPadView *weakSelf = self; + // A long drawing serializes to a large JSON string, and exporting already renders off the main thread. + dispatch_async(INKDocumentQueue(), ^{ + NSString *json = [INKStrokeCodec encodeDrawing:drawing size:size brushes:brushes]; + dispatch_async(dispatch_get_main_queue(), ^{ + [weakSelf emitResult:requestId ok:YES payload:json errorCode:@"" message:@""]; + }); }); - }); + }]; } - (void)setStrokes:(NSInteger)requestId json:(NSString *)json { - __weak INKPadView *weakSelf = self; + INKCommandSlot *slot = [_commands reserve]; // Loading a saved drawing builds a stroke path per point, which is as heavy as encoding one. - dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0), ^{ + dispatch_async(INKDocumentQueue(), ^{ NSString *errorCode = nil; NSString *message = nil; INKDecodedDrawing *decoded = [INKStrokeCodec decode:json errorCode:&errorCode message:&message]; dispatch_async(dispatch_get_main_queue(), ^{ - INKPadView *view = weakSelf; - if (decoded == nil) { - [view emitResult:requestId ok:NO payload:@"" errorCode:errorCode message:message]; - return; - } - [decoded.brushes enumerateKeysAndObjectsUsingBlock:^(NSDate *created, INKBrushInfo *info, BOOL *stop) { - [view rememberBrush:info forDate:created]; - }]; - [view replaceDrawing:decoded.drawing]; - [view emitResult:requestId ok:YES payload:@"" errorCode:@"" message:@""]; + [self->_commands fill:slot + action:^{ + if (decoded == nil) { + [self emitResult:requestId ok:NO payload:@"" errorCode:errorCode message:message]; + return; + } + [self->_brushes addEntriesFromDictionary:decoded.brushes]; + [self replaceDrawing:decoded.drawing]; + [self emitResult:requestId ok:YES payload:@"" errorCode:@"" message:@""]; + }]; }); }); } @@ -316,15 +336,23 @@ - (void)applyExportBackground:(NSString *)optionsJson - (void)exportImage:(NSInteger)requestId optionsJson:(NSString *)optionsJson { - __weak INKPadView *weakSelf = self; [self applyExportBackground:optionsJson]; + [_commands run:^{ + [self exportDrawing:self->_canvas.drawing requestId:requestId optionsJson:optionsJson]; + }]; +} + +// An export that waits for the background image keeps the drawing it was asked for. +- (void)exportDrawing:(PKDrawing *)drawing requestId:(NSInteger)requestId optionsJson:(NSString *)optionsJson +{ + __weak INKPadView *weakSelf = self; if (_imageLoading) { [_afterImageLoad addObject:^{ - [weakSelf exportImage:requestId optionsJson:optionsJson]; + [weakSelf exportDrawing:drawing requestId:requestId optionsJson:optionsJson]; }]; return; } - [INKImageExporter exportDrawing:_canvas.drawing + [INKImageExporter exportDrawing:drawing canvasSize:self.bounds.size canvasColor:_container.backgroundColor ?: UIColor.clearColor backgroundImage:_backgroundImage @@ -415,21 +443,7 @@ - (void)rememberNewStrokes info.hasPressure = stylus; info.hasTilt = stylus; info.hasOrientation = stylus; - [self rememberBrush:info forDate:created]; - } -} - -// Undo can bring back any stroke that is still in the drawing, so the brushes stay in a bounded window -// instead of growing for the whole session. -- (void)rememberBrush:(INKBrushInfo *)info forDate:(NSDate *)date -{ - if (![_brushOrder.lastObject isEqualToDate:date]) { - [_brushOrder addObject:date]; - } - _brushes[date] = info; - while (_brushOrder.count > INKBrushLimit) { - [_brushes removeObjectForKey:_brushOrder.firstObject]; - [_brushOrder removeObjectAtIndex:0]; + _brushes[created] = info; } } diff --git a/src/__tests__/iosCommandQueue.test.ts b/src/__tests__/iosCommandQueue.test.ts new file mode 100644 index 0000000..67bf70c --- /dev/null +++ b/src/__tests__/iosCommandQueue.test.ts @@ -0,0 +1,32 @@ +import { execFileSync, spawnSync } from 'node:child_process'; +import { mkdtempSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { expect, it } from 'vitest'; + +const path = (relative: string) => fileURLToPath(new URL(relative, import.meta.url)); + +it.skipIf(process.platform !== 'darwin')( + 'runs iOS view commands in the order they were sent', + () => { + const binary = join(mkdtempSync(join(tmpdir(), 'inkpad-')), 'command_queue_check'); + execFileSync('xcrun', [ + 'clang', + '-fobjc-arc', + '-Wall', + '-Werror', + '-framework', + 'Foundation', + path('../../ios/INKCommandQueue.m'), + path('./native/command_queue_check.m'), + '-o', + binary, + ]); + const run = spawnSync(binary); + const output = `${run.stdout}${run.stderr}`; + expect(output).not.toContain('FAIL'); + expect(run.status, output).toBe(0); + }, + 60_000, +); diff --git a/src/__tests__/native/command_queue_check.m b/src/__tests__/native/command_queue_check.m new file mode 100644 index 0000000..5aac1df --- /dev/null +++ b/src/__tests__/native/command_queue_check.m @@ -0,0 +1,55 @@ +#import + +#import "../../../ios/INKCommandQueue.h" + +static int failures = 0; + +static void expect(NSArray *log, NSArray *expected, const char *name) +{ + if (![log isEqualToArray:expected]) { + printf("FAIL %s: got %s\n", name, [[log componentsJoinedByString:@","] UTF8String]); + failures++; + } +} + +int main(void) +{ + @autoreleasepool { + { + NSMutableArray *log = [NSMutableArray new]; + INKCommandQueue *queue = [INKCommandQueue new]; + [queue run:^{ [log addObject:@"clear"]; }]; + expect(log, @[ @"clear" ], "runs a command right away when nothing waits"); + } + { + NSMutableArray *log = [NSMutableArray new]; + INKCommandQueue *queue = [INKCommandQueue new]; + INKCommandSlot *load = [queue reserve]; + [queue run:^{ [log addObject:@"clear"]; }]; + expect(log, @[], "holds commands behind a reserved slot"); + [queue fill:load action:^{ [log addObject:@"load"]; }]; + expect(log, @[ @"load", @"clear" ], "runs held commands after the slot fills"); + } + { + NSMutableArray *log = [NSMutableArray new]; + INKCommandQueue *queue = [INKCommandQueue new]; + INKCommandSlot *first = [queue reserve]; + [queue run:^{ [log addObject:@"clear"]; }]; + INKCommandSlot *second = [queue reserve]; + [queue run:^{ [log addObject:@"undo"]; }]; + [queue fill:second action:^{ [log addObject:@"second"]; }]; + expect(log, @[], "waits for the first load when the second finishes first"); + [queue fill:first action:^{ [log addObject:@"first"]; }]; + expect(log, @[ @"first", @"clear", @"second", @"undo" ], "applies loads in the order they were sent"); + } + { + NSMutableArray *log = [NSMutableArray new]; + INKCommandQueue *queue = [INKCommandQueue new]; + INKCommandSlot *load = [queue reserve]; + [queue fill:load action:^{ [log addObject:@"load"]; }]; + [queue run:^{ [log addObject:@"redo"]; }]; + expect(log, @[ @"load", @"redo" ], "runs commands right away again once the queue drains"); + } + } + return failures == 0 ? 0 : 1; +}