feat: Add BlinkIdScannerView for custom scanner UI - #30
Conversation
Adds BlinkIdScannerView widget + BlinkIdScannerController that own the camera layer on both platforms, letting callers render their own Flutter overlay with translated strings, branding, and animations. Dart: - BlinkIdGuidance sealed class (searching/tooFar/tooClose/tilted/holdStill/flipDocument) - BlinkIdScannerController (ChangeNotifier, guidanceStream, scan/cancel lifecycle) - BlinkIdScannerView (AndroidView / UiKitView via PlatformViewLink) Android: - BlinkIdScannerViewFactory + BlinkIdScannerView (CameraX PreviewView, ImageAnalysis → session.process, EventChannel for guidance, MethodChannel for result) - CameraX 1.4.1 dependency added to build.gradle - Factory registered in BlinkidFlutterPlugin.onAttachedToEngine iOS: - BlinkIdScannerViewFactory + BlinkIdScannerView (AVCaptureSession, AVCaptureVideoDataOutput, FlutterEventChannel/MethodChannel) - Factory registered in BlinkIdFlutterPlugin.register(with:) - blinkIdSdk visibility widened to internal for factory access Example app: - Full Flutter example app scaffolded in BlinkID/example/ - HomeScreen: "Scan (Native UI)" + "Scan (Custom UI)" buttons - CustomScannerScreen: Stack(BlinkIdScannerView, _GuidanceOverlay) demonstrating animated guidance text driven by guidanceStream performScan() path is untouched.
- Dart: guard null channel in scan(), catch PlatformException from startScan, extract _failScan() helper to always clean up completer, fail pending scan on dispose() to unblock waiters - Android: replace per-frame CoroutineScope(Dispatchers.Main) with view's existing scope to prevent memory leak - Android: catch cameraProviderFuture.get() failure and propagate via onScanError instead of crashing - Example: surface scan errors via SnackBar instead of silent pop
…to scanner view Guidance states (matching Microblink's full DetectionStatus enum): - Added: blur, glare, notFullyVisible, tooCloseToEdge, lowLight, tooMuchLight - Fixed: DOCUMENT_TOO_CLOSE_TO_FRAME_EDGE now maps to tooCloseToEdge (not tooClose) - Updated Android toGuidanceString() and iOS guidanceString extension with all new cases BlinkIdScannerView now accepts: - errorBuilder: shown when camera fails (mirrors mobile_scanner pattern) - placeholderBuilder: shown while camera is initialising - View uses ListenableBuilder on controller so placeholder/error swap in automatically - Controller exposes lastError for use in errorBuilder Example app updated with loading spinner placeholder and error display.
Problem: flipDocument guidance was overridden by tooFar/tooClose etc. Microblink's own UX pauses the camera analyzer on flip and resumes only after the flip animation completes — we mirror that exactly. BlinkIdScanPhase enum (front / flip / back): - Controller latches to .flip on first flipDocument event - Guidance stream goes silent during .flip phase (safety net filter) - Caller renders their own flip animation via phase changes on controller - controller.onFlipComplete() drives transition to .back and calls resumeAfterFlip on native side to re-enable frame processing Native (Android + iOS): - On flipDocument detection: emit event then set isScanning = false - resumeAfterFlip method channel handler sets isScanning = true Example app: - _FlipOverlay shown during .flip phase (card flip animation, 800ms) automatically calls onFlipComplete() when animation ends - _GuidanceOverlay shown during .front and .back phases - Back-side searching shows 'Scan the back side of a document'
initialize() now calls loadBlinkIdSdk() internally before attaching
the platform view. Controller transitions:
uninitialized → loadingSdk (model download / license check)
→ initializing (sdk ready, awaiting platform view)
→ ready (view attached, scan() available)
BlinkIdScannerView placeholderBuilder covers loadingSdk + initializing
so callers get a loading spinner automatically during SDK boot.
…types Android (user-verified against compiled SDK): - Real DetectionStatus enum cases: CameraTooFar, CameraTooClose, DocumentTooCloseToCameraEdge, CameraAngleTooSteep, DocumentPartiallyVisible - Flip trigger: ScanningStatus.SideScanned (not a DetectionStatus case) - Completion: ScanningStatus.DocumentScanned - session.process() is suspend; ImageRotation enum used for rotation - PreviewView.ImplementationMode.COMPATIBLE for TLHC overlay compatibility iOS rewrite: - Removed fictional BlinkIDFrameProcessResult / resultCompletionStatus types - session.process() is Void-returning (matches existing plugin usage) - Polls session.lastDetectionStatus for guidance (TODO: verify property name) - Polls getResult() to detect scan completion (TODO: getScanningStatus equivalent) - DetectionStatus case names aligned to Android's naming pattern - flipDocument emitted from SideScanned, not DetectionStatus; iOS needs equivalent status API — marked with TODO Dart: - BlinkIdGuidance: documented Android-verified vs iOS-only states - flipDocument kept in sealed class as safety net but marked phase-driven - blur/glare/holdStill/lowLight/tooMuchLight marked iOS-only pending verification Example: - CustomScannerScreen: listener-based scan start, _scanStarted guard, explicit loading/error states in build, _GuidanceOverlay as StatefulWidget - BlinkIdScannerView: stripped back to dumb platform view (errorBuilder/ placeholderBuilder removed; screen owns that logic) - widget_test.dart: cleared stale counter test
|
✅ Review posted. View review · run |
moxly
left a comment
There was a problem hiding this comment.
Risk Assessment
Score: 5/10 — medium
This PR adds a large new native surface (Android CameraX + Kotlin coroutines, iOS AVFoundation + Swift concurrency) plus touches the shared session-settings deserialization used by the pre-existing performScan path. The concurrency findings are timing-dependent rather than deterministic crashes, and the settings-serialization concern is unverified against the third-party SDK's actual defaults, which keeps this from being higher.
Review Summary
Verdict: COMMENT
0 critical, 4 warnings, 1 nit
Note
This is a large PR, so I focused my closest review on the changes most likely to carry risk (the native scanner views, the shared settings deserialization, and the controller) and gave the example app / project scaffolding a lighter pass. Splitting it into smaller PRs would let every change get the same close attention.
Cross-cutting notes:
- Cancel race (Android + iOS): both platform implementations let
cancelScanflip local state on the main thread while a frame already in flight on the analyzer/capture thread keeps running to completion, so a lateonDocumentScanned/onScanResultcan land on the Dart side aftercancel()already reset the controller toready. See inline findings onBlinkIdScannerView.ktandBlinkIdScannerView.swift. - iOS/Android parity gap for default scanning-settings: the Android deserializer got an explicit fix in this PR for "all scanning modules null → construct full defaults" (needed because "the Android SDK may default modules to null"), but the parallel iOS deserializer in the same PR didn't get an equivalent accommodation — worth confirming iOS doesn't hang on the same default config used by the example app. See inline finding on
BlinkIdDeserializationUtils.swift.
Fix All — prompt for AI agent
Fix the following issues in this PR:
- In
BlinkID/android/src/main/kotlin/com/microblink/blinkid/flutter/BlinkIdScannerView.kt(lines 93-97, and the analyzer callback around lines 172-233):cancelScandoesn't stop an already-in-flight frame process; guard the async result-delivery path (e.g. with a per-scan generation token checked before invokingonDocumentScanned/onScanResult) so a cancelled scan can't deliver a late result to Dart. - In
BlinkID/ios/blinkid_flutter/Sources/blinkid_flutter/BlinkIdScannerView.swift(lines 102-105, and theTask { @ProcessingActor ... }incaptureOutput): same issue — add an equivalent generation/cancellation check before the.documentScanned/.sideScannedcallbacks fire. - In
BlinkID/ios/blinkid_flutter/Sources/blinkid_flutter/Serialization/BlinkIdDeserializationUtils.swift(lines 92-106): confirm whether iOS's nativeScanningSettings()defaults modules to null the same way Android's does; if so, add the same "all modules null → construct full defaults" accommodation thatBlinkidDeserializationUtils.kthas. - In
BlinkID/android/src/main/kotlin/com/microblink/blinkid/flutter/BlinkIdScannerView.kt(lines 193-204): movesession.getResult(null)/ result serialization offDispatchers.Main(e.g.withContext(Dispatchers.Default)) and only hop back to Main for themethodChannel.invokeMethodcall, so a large result can't block the UI thread. - In
BlinkID/lib/src/scanner/blinkid_scanner_controller.dart(lines 233, 251): add.ignore()to thecancelScaninvokeMethodcalls for consistency with the rest of the file.
After all changes, run the project's linter/formatter.
…sult, .ignore() on cancelScan - iOS/Android: replace generation counter with session-identity check (blinkIdSession === session / scanningSession == null) — same staleness protection without an extra field; cancelScan also resets isProcessingResult - Android: move getResult() and JSON serialization onto Dispatchers.Default so heavy work doesn't block the main thread - Dart: add .ignore() to both cancelScan invokeMethod calls in cancel() and reset() to silence unhandled-future lint
|
✅ Review posted. View review · run |
moxly
left a comment
There was a problem hiding this comment.
Risk Assessment
Score: 4/10 — medium
This re-review is scoped to the incremental diff, so risk assessment reflects the combined state of new findings plus carry-forwards. The iOS cancel race is now fully closed with a solid identity-based guard, and the Android performance fix (moving getResult()/serialization off Main) landed cleanly — but the Android cancel-race fix uses a weaker null-check instead of an identity check, leaving a narrower but still-reachable race where a rapid cancel+restart can clobber an active new scan with a stale result. The unresolved iOS/Android default-settings parity gap also remains untouched.
Review Summary
Verdict: COMMENT
Re-review scoped to changes since commit 9cf78e69.
Unresolved from previous review: 1 warning. New findings: 1 warning.
4 resolved — 4 fixed, 0 dismissed; 1 carried forward.
New finding:
BlinkID/android/src/main/kotlin/.../BlinkIdScannerView.kt:201— the cancel-race guard added this cycle checksscanningSession == nullinstead of identity-comparing to the capturedsession, so a cancel-then-restart during thegetResult()suspension window can still deliver a stale result and null out an active new scan. See inline finding.
Resolved this cycle:
- Android cancel race (previously flagged missing guard) — fixed via
scanningSession == nullchecks around theDocumentScanned/SideScannedhandling (though see the new, narrower finding above). - iOS cancel race — fully fixed via identity checks (
self.blinkIdSession === session) at every hop after a suspension point. - Android
getResult()/serialization on Main dispatcher (performance) — fixed viawithContext(Dispatchers.Default). - Dart
cancelScanmissing.ignore()(nit) — fixed at both call sites.
Unresolved from previous review (carried forward, not re-flagged inline):
BlinkID/ios/blinkid_flutter/Sources/blinkid_flutter/Serialization/BlinkIdDeserializationUtils.swift(around line 92-106) — the "all modules null → construct full defaults" accommodation that Android'sBlinkidDeserializationUtils.ktadded is still absent on iOS. This file wasn't touched in this round of commits, and the flagged construct (bareif let ... = ... as? Dictionarywith noelsebranch, no defaults fallback) is still present. Still worth confirming before merge whether iOS's nativeScanningSettings()shares Android's null-module default behavior.
Fix All — prompt for AI agent
Fix the following issues in this PR:
- In
BlinkID/android/src/main/kotlin/com/microblink/blinkid/flutter/BlinkIdScannerView.kt(lines 195-196 and 201): replace theif (scanningSession == null) return@launchguards with an identity check against thesessionthis coroutine already captured (if (scanningSession !== session) return@launch), matching the iOS identity-based fix in the same PR, so a cancel-then-restart during thegetResult()suspension can't clobber the newly-started session. - In
BlinkID/ios/blinkid_flutter/Sources/blinkid_flutter/Serialization/BlinkIdDeserializationUtils.swift(lines 92-106): confirm whether iOS's nativeScanningSettings()defaults modules to null the same way Android's does; if so, add the same "all modules null → construct full defaults" accommodationBlinkidDeserializationUtils.kthas.
After all changes, run the project's linter/formatter.
Null check misses the cancel-then-restart case where scanningSession is already a new non-null session. Identity check matches the iOS approach (blinkIdSession === session) and covers both cases correctly.
|
✅ Review posted. View review · run |
moxly
left a comment
There was a problem hiding this comment.
Risk Assessment
Score: 3/10 — low
Review Summary
Verdict: COMMENT
Re-review scoped to changes since commit fc285672.
Unresolved from previous review: 1 warning. New findings: 0.
1 resolved — 1 fixed, 0 dismissed; 1 carried forward.
This cycle's only change is the 3-line Android fix (BlinkIdScannerView.kt lines 195, 201, 222) replacing the scanningSession == null guard with an identity check (scanningSession !== session), matching the iOS pattern already in the codebase. Verified: session is captured once per analyzer frame at the top of the setAnalyzer closure, so after the getResult() suspension point a stale coroutine resuming with a different current session (or null after a bare cancel) now correctly bails via return@launch instead of clobbering the active session or delivering a stale result. This fully closes the cancel/restart race flagged in the previous review.
Resolved this cycle:
- Android session-identity race (
BlinkIdScannerView.kt:195/201/222) — fixed via!==identity comparison.
Unresolved from previous review (carried forward, not re-flagged inline):
BlinkID/ios/blinkid_flutter/Sources/blinkid_flutter/Serialization/BlinkIdDeserializationUtils.swift(lines 92-106) — still no "all modules null → construct full defaults" accommodation on iOS matching Android'sBlinkidDeserializationUtils.kt. File untouched this round; construct confirmed still present. Worth confirming before merge whether iOS's nativeScanningSettings()shares Android's null-module default behavior.
Fix All — prompt for AI agent
Fix the following issues in this PR:
- In
BlinkID/ios/blinkid_flutter/Sources/blinkid_flutter/Serialization/BlinkIdDeserializationUtils.swift(lines 92-106): confirm whether iOS's nativeScanningSettings()defaults modules to null the same way Android's does; if so, add the same "all modules null → construct full defaults" accommodationBlinkidDeserializationUtils.kthas.
After all changes, run the project's linter/formatter.
|
@JulioSarmientoMews Ready! Sorry for being too many changes. I also opened it to the official repo, hopefully they would review it. microblink#100 The checks pass there, as this is a fork we don't have the proper action set up. |
JulioSarmientoMews
left a comment
There was a problem hiding this comment.
REQUEST CHANGES — three cheap fixes, no rework needed.
The design here is genuinely good: the status/phase state machine, the native flip gate, and the session-identity staleness guards all hold up. Solid security posture too — license key via --dart-define, .env gitignored, opt-in debug logging, Wiz clean. Reviewed at d8391b2.
1. The camera-switch SIGSEGV mitigation is gone, but the switch path is live again (BlinkIdScannerView.kt:132-138)
4061a336 added withContext(analysisExecutor.asCoroutineDispatcher()) {} + delay(500L) before unbindAll(), with a comment explaining that session.process() resumes before BlinkID's ProcessingQueue has finished with the frame buffer. 860753c7 added the null-session-first ordering. The current handler keeps the ordering but has neither the drain nor the delay, and performCameraSetup() still calls cameraProvider.unbindAll() (line 351). Meanwhile d9c0a16 restored the example's camera-switch button that 9a65a60 had removed for this crash — so the path is reachable.
Was the drain + delay dropped deliberately, or lost in the d9c0a16 rebase? If deliberate, what makes it safe now?
Same window at two more sites, one fix covers all three: dispose() sets the lifecycle to DESTROYED (→ CameraX unbind) then calls analysisExecutor.shutdown() without awaitTermination; "retryCamera" also reaches unbindAll() with no drain.
Context: the previous automated review scoped itself to "changes since 814ab9a8" and treated switchCamera as new surface rather than comparing it against the earlier drain commits — which is why the prior approvals don't cover this.
2. Android lost the thread-safety fix iOS got (BlinkIdScannerView.kt:72-77)
isScanning, scanningSession, debugLoggingEnabled, preferredCameraOverride are written on the main thread and read on the analyzer thread with no @Volatile and no lock. d8391b2 introduced an NSLock on iOS for exactly this; Android didn't get the equivalent. This weakens the scanningSession !== session guard earlier rounds asked for, and it's the same window that feeds #1. @Volatile on the four fields is the whole fix.
3. iOS swallows camera-setup failures (BlinkIdScannerView.swift:226-233, :242-245)
Both guard failures set cameraSetupFailed = true and return with nothing sent to Dart. The controller stays ready, lastError is null, the user gets a black rectangle with no recovery path. Android reports both equivalents ("Camera unavailable", "Camera bind failed"). Please invoke onScanError on both paths.
Non-blocking, but worth doing while you're in here
switchCameracompletes the in-flight scan withBlinkIdScanCancelException, indistinguishable from a user cancel — which is why the example has to sniffstatus == .initializing(custom_scanner_screen.dart:149). Since Android repliessuccess(null)before the rebind finishes,readycan land before thatcatchruns and the screen pops itself. A distinctBlinkIdScanCameraSwitchExceptionremoves both the race and the sniffing.scan()re-checks nothing afterawait _awaitReady(); two concurrent calls duringinitializingboth proceed and the first completer is orphaned → silent hang.startScanoverwritespendingStartResultwithout replying (hung future) and discards an already-created session at line 174.- Sessions are only nulled, never closed, on either platform — one per retry attempt. Does
BlinkIdScanningSessionhold native resources? - The analyzer block is
try { … } finally { close() }with nocatch, thoughd8391b2's message says a try-catch was added — exceptions vanish into CameraX's executor and scanning appears to freeze. BlinkIdGuidanceexposesholdStill/blur/glare/lowLight/tooMuchLight, which neither platform ever emits: 12 cases in an exhaustive switch for 7 reachable states.blinkid_scanner_controller.dart:398debugPrints$eungated. No PII on today's reachable paths (both platforms send aMap), but thejsonDecodebranch would embed document JSON if a native path ever sends a String — cheap to gate now.
Cross-consumer, please verify against a host app before merging
build.gradle now applies kotlin-android with a pinned KGP 2.1.0 classpath and bumps CameraX 1.4.1 → 1.6.1 — host-app Kotlin/AGP conflicts are the usual failure mode for a published plugin. And Package.swift adds .package(path: "../FlutterFramework"), but BlinkID/ios/FlutterFramework is neither committed nor gitignored — confirm it's tool-generated and that podspec consumers are unaffected.
Tests: none, and CI doesn't run on this fork, so nothing automated has validated this branch here. Not a regression (the repo had no tests before), but the Dart controller is pure Dart — a few tests over the cancel/reset/dispose/permission completer paths would cover exactly where every review round has been finding bugs.
On PR size: agreed it can't split cleanly, but the ktlint/swift-format reformatting could have landed as its own commit — that alone would have made the functional diff a fraction of its size and this review considerably faster.
Android emits the string "searching" for the searching/idle state but it had no entry in the from-string switch, falling through to _unknown().
Dart: - Add BlinkIdScanCameraSwitchException distinct from BlinkIdScanCancelException so scan loop can continue after switchCamera() without treating it as cancel - Guard scan() against double-start with StateError - Gate _handleMethodCall error logging on _debugLoggingEnabled - Update guidance comment to reflect neither platform emits holdStill/blur/glare Android BlinkIdScannerView: - Mark debugLoggingEnabled/scanningSession/isScanning/preferredCameraOverride @volatile — all read from analyzer thread, written from main thread - Drain analysisExecutor before setupCamera() in retryCamera and switchCamera to prevent race between in-flight frame and CameraX rebind - Guard startScan against concurrent call (pendingStartResult != null) - Catch and log analyzer exceptions via onDebugLog instead of crashing silently - Move lifecycleRegistry.DESTROYED after executor drain in teardown() iOS BlinkIdScannerView: - Emit onScanError when camera device or session setup fails instead of returning silently, so Flutter can surface the error state Example: - Catch BlinkIdScanCameraSwitchException separately with continue - Remove the status == .initializing hack from BlinkIdScanCancelException handler
Android & iOS BlinkIdScannerView: - Emit 'blur' / 'glare' guidance from frame analysis (blurDetectionStatus / glareDetectionStatus == Detected) ahead of the DetectionStatus fallback - Pass scan result as raw JSON string to onScanResult instead of deserializing to a nested Map — avoids Map<Object?,Object?> from StandardMessageCodec and removes the toNestedMap/toNestedList helpers - Fix EventChannel teardown ordering: null guidanceEventSink in teardown() for immediate effect; setStreamHandler(null/nil) in the 'dispose' method-channel handler (Dart's 'cancel' always arrives first on same queue) Dart scanner controller: - cancel() when status is initializing now rejects all _pendingReady completers with BlinkIdScanCancelException so scan() unblocks immediately (fixes X button being stuck during camera switch) - Simplify onScanResult JSON parsing: String path skips redundant Map.from() Dart blinkid_guidance: - Promote blur/glare to "Quality hints (emitted by both platforms)" section - Update doc comment mapping table to reflect confirmed platform behaviour
|
✅ Review posted. View review · run |
moxly
left a comment
There was a problem hiding this comment.
Risk Assessment
Score: 3/10 — low
Risk Assessment
Score: 3/10 — low-moderate
This cycle's changes (23 files, +3599/-2621 since 814ab9a) are dominated by formatter-driven reformatting (ktlint/swift-format) across the serialization/deserialization files, plus genuine fixes for all 5 carry-forward correctness issues from the previous cycle. Three new warnings surfaced, none crash-level: an asymmetric default-fallback for one settings module, a stuck-UI path in the example app's new SDK-init error handling, and a README/code mismatch for the exception type thrown by switchCamera().
Review Summary
Verdict: APPROVE
Verdict: APPROVE
Note
This review used the selective read strategy (bundle diff was 7760 lines, over the 1000-line auto-read threshold) — files were fetched individually via read_diffs rather than reading the full bundle in one pass.
Re-review scoped to changes since commit 814ab9a
- Resolved (5):
c219992d—cancelScanrace delivering post-cancelonDocumentScanned/onScanResulton Android: fixed via identity-based session guards (scanningSession !== session) throughoutBlinkIdScannerView.kt.a057f229— equivalent iOS cancel race: fixed viaNSLock-protected state and the retainedself.blinkIdSession === sessionidentity checks inBlinkIdScannerView.swift.6672acea—getResult()/serialization running on the Main dispatcher: now explicitly moved toDispatchers.Default, hopping back to Main only for themethodChannel.invokeMethodcall.8f66beb1— un-.ignore()dinvokeMethod('cancelScan')calls: now routed through a private_cancelScan()helper that calls.ignore(), consistent with the rest of the file.4e5847c3— Android's non-identityscanningSession == nullcancel-race guard: replaced withscanningSession !== sessionidentity comparisons matching the iOS pattern.
- Dismissed (0)
- Unresolved (0) — no carry-forward items remain open.
(bc684cbe, the iOS null-module deserialization fix, was already resolved and approved in the previous review cycle per that cycle's body — not recounted here.)
New findings this cycle
| Severity | Count |
|---|---|
| Critical | 0 |
| Warning | 3 |
| Nit | 0 |
BlinkidDeserializationUtils.kt:109—barcodeModuleis missing the?: BarcodeModuleSettings()default fallback that its three sibling modules received, contradicting the comment directly above it and diverging from iOS's parity behavior.custom_scanner_screen.dart:133-135— the newon BlinkIdSdkInitExceptionhandler in_startScanning()silently swallows the error (no SnackBar, no_safePop()), leaving the reference-implementation screen stuck on an un-dismissable loading spinner if the SDK fails to load.README.md:500— documentsswitchCamera()as completing the in-progress scan withBlinkIdScanCancelException, but the code actually throws the distinctBlinkIdScanCameraSwitchException.
Fix All
- Add the missing
?: BarcodeModuleSettings()fallback indeserializeScanningSettings(Android). - Surface the
BlinkIdSdkInitExceptionerror to the user and call_safePop()in the example app's initial catch clause. - Correct the README's
switchCamera()exception name toBlinkIdScanCameraSwitchException.
Android: barcodeModule was missing ?: BarcodeModuleSettings() fallback added in the earlier ScanningSettings refactor — now consistent with mrz/viz/documentCapture modules. Example: BlinkIdSdkInitException during initialize() now shows a snackbar and pops the scanner instead of returning silently. README: correct switchCamera() docs to reference BlinkIdScanCameraSwitchException (was stale BlinkIdScanCancelException after the dedicated exception was added).
JulioSarmientoMews
left a comment
There was a problem hiding this comment.
Impressive amount of hard native work here — the completer discipline, session-identity guards, off-main-thread result serialization, and the permission state machine are all more careful than typical for a platform-view PR, and the commit history shows real convergence on genuinely nasty problems. The custom-scanner API shape is good.
Requesting changes on three issues, all small fixes rather than rethinks.
1. cancelScan / switchCamera don't cancel an in-flight startScan. Four sites: Android leaves pendingStartResult set, iOS leaves _startScanTask running, in both handlers on both platforms. The startScan continuation then unconditionally sets a live session and isScanning = true. Two consequences:
cancel()documents "returns to ready", but native goes back to actively scanning. A host that cancels without tearing down the view is left with a live BlinkID session on an ID-capture path and no Dart handle to it.- On camera switch this reinstates a live session inside the 500 ms window before
unbindAll()— the exact precondition860753c7/4061a336were written to remove.
2. Dart switchCamera() reports ready before the camera has rebound. Android replies result.success(null) synchronously, before the drain + 500 ms delay + setupCamera() coroutine has run, so ready is at least 500 ms early and never means "camera bound". The example's scan loop continues straight into scan() on BlinkIdScanCameraSwitchException, which starts a session against a camera about to be unbound — feeding (1) with no user tap.
3. barcodeModule is disabled by default on Android but enabled on iOS. In deserializeScanningSettings the comment says "Absent or null key → use SDK default (extraction enabled)", but only three of four modules get the ?: XModuleSettings() fallback — barcodeModule passes null through, disabling it. iOS starts from ScanningSettings(), whose init (per your own note on this thread) defaults all four to .init(). Default config on a US driver's licence therefore gets PDF417 fields on iOS and empty on Android, silently. Which behavior is intended?
Also worth resolving before merge, though I'd not block on it alone: build.yaml's explicit_to_json + @JsonKey(includeIfNull: false) means a null module key is now omitted, so there is no longer any way to disable a module from Dart — which makes the NSNull branches you added in 814ab9a8 specifically to honor an explicit null unreachable from the public API. Two commits in this PR pulling opposite ways; needs a decision either direction. Relatedly: build.yaml is repo-wide but only two .g.dart files changed — is anything else under lib/ generated and now stale?
Smaller items are inline. Two broader notes:
- Tests.
widget_test.dartis an emptymain()and CI doesn't run on the fork, so nothing here can fail automatically.BlinkIdScannerControlleris pure Dart behind aMethodChannel, sosetMockMethodCallHandlercovers issues 1, 2 and the dispose race with no device, camera or license key. That's the layer the bugs are actually in. - Verified the churn. I diffed the reformatted files whitespace-insensitively:
BlinkidSerializationUtils.ktand iOSBlinkIdSerializationUtils.swiftare genuinely mechanical, with no serialized key or field altered. Worth stating explicitly since that's the result path every current consumer depends on.
…t-cam rotation Camera result deferral (Android + iOS): - switchCamera/retryCamera no longer complete their Flutter result immediately; result is held in pendingCameraResult and completed from completeCameraResult() once the camera has actually bound (or definitively failed). Dart await on switchCamera() is now accurate rather than resolving before the bind. - switchCamera() returns the lens actually bound (PreferredCamera), which may differ from the request on devices without a front camera. Also exposed as controller.activeCamera. - abortPendingStart() / abortPendingCameraResult() helpers clean up in-flight operations on cancel, switch, and dispose. Android: - startJob + ensureActive() guard: a session created during an in-flight startScan is discarded if cancelled before it can be installed. - resolveCameraLens() returns a String; preferredCameraOverride is updated to the actually resolved lens; cameraSelectorFor() is a separate helper. - resolveModuleSettings() generic replaces 4 nearly-identical optional*Settings() helpers in BlinkidDeserializationUtils. - All Log.i() → Log.d() gated behind Log.isLoggable(TAG, Log.DEBUG). - Plugin logs gated behind Log.isLoggable as well. - CAMERA permission declared in plugin manifest (merged automatically). iOS: - Front camera landscape rotation fix: front sensor is 180° rotated vs back in landscape; frontCameraLandscapeRotationAngle / frontCameraLandscapeAvOrientation extensions handle the inversion; automaticallyAdjustsVideoMirroring disabled before updateVideoOrientation() so manual mirroring isn't overridden. - preferredCameraOverride pinned to resolved lens early in performCameraSetup(). - UIApplication.shared.windows → connectedScenes (deprecated API fix). - Plugin logs wrapped in #if DEBUG. Dart scanner controller: - _disposed flag: guards _handleMethodCall, _onGuidanceEvent, _setStatus and notifyListeners() against late native callbacks racing dispose(). - dispose() sets _disposed first, nulls method call handler before invoking native dispose, closes guidance controller after. - switchCamera() returns Future<PreferredCamera>; _retryCamera() updates _activeCamera from the resolved lens string. - _lastEmittedGuidance: collapses consecutive duplicate guidance frames before they reach guidanceStream (native emits ~30/s; exact duplicates add no info). Phase change resets the last value so "searching" is re-emitted on back side. Dart BlinkIdScanningSettings: - Tri-state sentinel (_unsetModule): omit = SDK default (enabled), explicit null = disabled. @JsonSerializable(createFactory: false) + hand-written fromJson preserve absent-vs-null distinction on the way back in. - @jsonkey(includeIfNull: false) annotations removed; toJson() now emits null for disabled modules so native can honour the explicit disable. Example custom_scanner_screen.dart: - Guidance state lifted to parent State: single _guidanceSub drives both the scan-timeout timer and the display. _GuidanceOverlay takes text + switcherKey instead of a controller reference; _FlipOverlay takes an onFlipComplete callback. Phase change resets guidance text immediately. - _FlipOverlayState.dispose() calls _completeFlip() so onFlipComplete is guaranteed even if the overlay is torn down before animation finishes. - canInteract excludes status == .initializing to hide X/camera buttons during camera rebind. README: camera permission section, module omit-vs-null clarification, switchCamera return value, status table updated.
JulioSarmientoMews
left a comment
There was a problem hiding this comment.
Approving. 97aad3cd addresses every blocking item, and it does it properly rather than papering over.
What I verified rather than took on trust:
- In-flight
startScanabort.abortPendingStart()cancelsstartJobandensureActive()discards the just-created session, with thependingStartResult ?: return@launchcheck behind it.scopeisDispatchers.Mainand channel calls arrive on main, so there's no suspension point betweenensureActive()andisScanning = true— the interleaving window is genuinely closed, not narrowed. iOS mirrors it viacheckCancellation(). - Deferred camera result. I traced every exit from
setupCamera/performCameraSetupon both platforms — permission granted, no activity, soft deny, permanent deny,restricted, provider throws, device unavailable,canAddInputfails, bind ok, bind throws — and each one completes or failspendingCameraResult, withdispose/teardownaborting it. One residual hang path inline below. - Dispose ordering.
_disposedset first,setMethodCallHandler(null)beforeinvokeMethod('dispose'),close()after, guards in_handleMethodCall/_onGuidanceEvent/_setStatus. Correct. - Guidance dedup actually fires. It relies on
BlinkIdGuidanceequality;fromStringreturns fieldlessconstinstances, which Dart canonicalizes, so the identity comparison collapses duplicates as intended. Happy with the reasoned pushback on full smoothing staying caller-side — the doc comment onguidanceStreammakes the contract explicit, which was the real gap. - The
.g.dartquestion I left open last round. Only two generated files exist, andtypes.g.dartuses.toJson()for every nested object and enum maps for enums — coherent with repo-wideexplicit_to_json, nothing stale. That was the one thing that could have kept this blocked.
Nice catches of your own along the way: the deprecated UIApplication.shared.windows migration, pinning preferredCameraOverride to the resolved lens so isFront can't disagree with position, and _FlipOverlay.dispose() calling _completeFlip() so native scanning resumes if the overlay is torn down mid-animation — a real bug, and one the canInteract fix made reachable.
Correction on my last review — two of those 14 comments were wrong when I posted them. The barcodeModule fallback and the README BlinkIdScanCameraSwitchException name were both already fixed in 09ecd102; my review was anchored to that commit but analyzed an older checkout. Sorry for the noise — both threads are already resolved and marked outdated, so nothing needs doing. The other 12 were live at that commit.
Four non-blocking items inline. Nothing there should hold the merge; the type-safety one (A) is the only one I'd want a considered answer on before it goes upstream to microblink#100.
One standing ask rather than a blocker: there are still no tests. _disposed, abortPendingStart and the deferred camera result are now load-bearing for correctness, and all three are reachable from pure Dart via setMockMethodCallHandler — no device, camera or license key. Given CI doesn't run on this fork, that's the cheapest way to stop a future refactor silently reopening any of them.
…EADME fixes Use a per-view serial DispatchQueue for all startRunning()/stopRunning() calls: - Apple documents both as blocking — neither belongs on the main thread. - Serial ordering (enqueued from main thread in call order) prevents a stale session's queued start from running after a later switchCamera()/teardown() has already stopped and detached it — no generation counter needed. - stopCaptureSession() and teardown() capture the session reference before nil-ing captureSession, then dispatch stopRunning() on the queue so the stop is always ordered after this session's own queued start. README: add missing 'dart:async' import to custom scanner example; add BlinkIdScanCameraSwitchException catch to the scan loop snippet.
types.dart — BarcodeModuleSettings, DocumentCaptureModuleSettings, MrzModuleSettings, VizModuleSettings: - All fields marked final; constructors made const. - copyWith() added to each so callers can produce modified copies without constructing from scratch. blinkid_settings.dart — BlinkIdScanningSettings: - Replace _unsetModule sentinel + initialiser list with plain const-default parameters (this.field = const ModuleSettings()). Because the defaults are now const instances, the hand-written fromJson still distinguishes "key absent → leave the const default" from "key present and null → disabled" correctly via containsKey(). - Simplify the comment: the reason createFactory: false is still needed hasn't changed, only the constructor shape. sample_files/module_settings_panel.dart: - Switch all _updateBarcode/_update callbacks from direct mutation to copyWith() to match the now-immutable module settings types.
…t tests iOS: teardown() now calls stopCaptureSession() instead of repeating the same capture/nil/sessionQueue.async pattern inline — eliminates the risk of the two diverging and ensures videoOutput is also cleared on teardown. Tests (BlinkID/test/): - blinkid_scanner_controller_test.dart: pure-Dart state-machine tests driven via mocked method/event channels (no device or license required). Covers: in-flight startScan surviving cancel/switchCamera; switchCamera not reporting ready until the native reply actually lands and returning the resolved lens; late native callbacks after dispose() not throwing or corrupting status; in-flight scan completing with BlinkIdScanDisposeException. - blinkid_settings_test.dart: verifies the tri-state module contract on BlinkIdScanningSettings (unset → SDK-default enabled; explicit null → disabled; round-trip through toJson/fromJson is stable).
|
@JulioSarmientoMews This is final I promise, hopefully claude finds no more things... 🙏🏻 |
|
✅ Review posted. View review · run |
moxly
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: APPROVE
Risk Assessment
Score: 2/10 — low
Risk: low (2/10). All previously open items are fixed in code, the new concurrency paths are covered by new targeted unit tests, and no new correctness/security issues were found in the incremental diff.
Re-review scoped to commits since 3b05e1bd0.
Note
This diff (+1200/-756 across 15 files) exceeded the bundle threshold, so it was read via the selective per-file strategy rather than a single bundle diff.
Resolved (3/3 fixed in code, 0 dismissed):
- Asymmetric
barcodeModuledefault (14d5886c) —blinkid_settings.dart/.g.dartnow give all 4 module settings realconstdefaults with a hand-written tri-statefromJson, and Android'sBlinkidDeserializationUtils.ktgained a single genericresolveModuleSettings<T>()applied uniformly to all 4 modules — closing the gap that previously only affectedbarcodeModule. Verified against the newblinkid_settings_test.dartround-trip tests. BlinkIdSdkInitExceptionswallowed on init failure (f719e05d) —custom_scanner_screen.dartnow surfaces a SnackBar and calls_safePop()on that path instead of silently failing.- README exception-name mismatch (
46fc93ca) — corrected toBlinkIdScanCameraSwitchException, with a new camera-permission documentation section added alongside it.
New in this cycle: nothing rises to a reportable issue. The new deferred camera-switch completion (completeCameraResult/pendingCameraResult on both Android and iOS), the startJob/ensureActive() cancellation guard on Android, and the sessionQueue-serialized startRunning()/stopRunning() on iOS were traced through their cancel/dispose/switch-camera interleavings without finding a concrete race, and are directly exercised by the new blinkid_scanner_controller_test.dart (cancel-during-in-flight-start, dispose-during-in-flight-scan, switchCamera error/resolved-lens paths). The mutable→immutable refactor of the module settings classes (types.dart) was checked repo-wide for stale direct-mutation call sites — none remain.
JulioSarmientoMews
left a comment
There was a problem hiding this comment.
Approving. Everything from both prior rounds is resolved, and the two changes I could least confirm by reading both hold up under inspection.
The Object? sentinel replacement is better than what I suggested. this.barcodeModule = const BarcodeModuleSettings() keeps the tri-state — nullable field, so explicit null still disables and omitted still yields the SDK default — while restoring full static typing and deleting the sentinel and its identical() gymnastics outright. Good call taking the const-default route rather than the const disabled instance I floated.
Since that forced the immutability refactor and types.g.dart is not in the diff, I checked whether the generated code still compiles rather than assuming:
- the four module classes' generated
_$…FromJsonuse constructor-arg style, which is fine againstfinalfields; ClassFilteris the only class whose generatedfromJsonuses mutating cascades, and it kept non-final fields, so it's unaffected;- nothing in
lib/,example/lib/orsample_files/assigns to any now-final field —module_settings_panel.dartmigrated cleanly tocopyWith.
The sessionQueue change (f3264c5) is sound. Both startRunning()/stopRunning() are blocking, every enqueue happens from main in call order, and a serial queue preserves that order — so a stale session's queued start genuinely can't outrun a later stop, without needing generation tracking. Capturing session as a strong local so it stays alive to be stopped after the field is nil'd is the detail that makes it work.
The tests are the right tests. They hit the actual review bugs through TestDefaultBinaryMessengerBinding with no device, camera or license, flutter_test is in dev_dependencies, and every symbol used is exported from the barrel — so they run rather than just existing. One of them has a problem; inline.
Two notes, neither blocking:
Package.swift:15'sFlutterFrameworkpath dependency is untouched for a third round. Still fine by me to ship as-is — just flagging that it's unanswered rather than agreed.- Making the module settings fields
finalis a breaking change for anyone consuming this fork who mutates them in place (settings.barcodeModule.qrScanningEnabled = true). Nothing in-repo does, andcopyWithis the migration path, but it's worth a changelog line — particularly for microblink#100.
Coverage boundary worth stating plainly: these tests cover the Dart half. The native halves of the same fixes — abortPendingStart/ensureActive, the deferred pendingCameraResult, and the sessionQueue ordering — still have no automated coverage on either platform, so those remain reasoning-and-manual-testing only.
Unrelated to this PR and not yours to fix: Analyze (java-kotlin) has failed on every run here and there are no runs on main at all, so there's no green baseline. It's CodeQL default setup on build-mode none, which can't extract Kotlin without a real build. The merge will stay blocked on it until someone with repo admin sets up advanced CodeQL with manual build steps (including a Flutter step to generate local.properties) or drops java-kotlin from the required checks. I'll raise that separately.
Summary
Adds
BlinkIdScannerView— a Flutter platform view that exposes the BlinkID camera scanning engine with a fully custom Flutter UI, alongside the existingperformScan(built-in native UX) path.This PR is intentionally large and cannot be split smaller. The custom scanner depends on every layer beneath it — platform view registration, native session lifecycle, Dart controller, guidance events, and flip-phase coordination. Splitting any layer into a separate PR would leave the plugin non-functional on that branch. Camera switching in particular required multiple passes to stabilise (SIGSEGV on Android, iOS null-module deserialization bug), and those fixes are inseparable from the feature that exposed them.
New public API
BlinkIdScannerController(ChangeNotifier) — manages scan lifecycle, phase, and guidance. Callinitialize()+scan(); handles retry loops viareset().BlinkIdScannerView— platform view widget that renders the native camera surface (Android: CameraX + TextureView in COMPATIBLE mode; iOS: AVFoundation).BlinkIdScanPhase—front/flip/back— drives the flip animation gate. Whenflip, callonFlipComplete()after your animation or the scanner stays paused.BlinkIdScannerStatus— full lifecycle enum:uninitialized → loadingSdk → initializing → ready → scanning → processing → done | error.BlinkIdGuidancesealed class — per-frame detection guidance stream (tooFar,tooClose,tilted,blur,glare,notFullyVisible,wrongSide, …).PreferredCameraenum +preferredCameraparam oninitialize()— choose starting lens.BlinkIdScannerController.switchCamera(PreferredCamera)— runtime camera switch; reinitializes the native session, cancels in-progress scan cleanly.BlinkIdScannerController.setDebugLogging(bool)— opt-in native lifecycle log forwarding to FlutterdebugPrint. No channel traffic unless enabled.Implementation notes
ImageAnalysisanalyzer →session.process()on a single-thread executor;runBlockingbridges coroutine to analyzer thread.PreviewView.ImplementationMode.COMPATIBLE(TextureView) so the camera surface composites into the Flutter layer hierarchy instead of rendering in a separate window.AVCaptureVideoDataOutput→session.process()on@ProcessingActor;isProcessingFrameguard prevents 30 fps backlog buildup. Orientation updates viaUIDevice.orientationDidChangeNotification.SideScanned; resumes only afterresumeAfterFlipmethod call from Dart.debugLoggingEnabledflag on both platforms; key events forwarded viamethodChannel.invokeMethod("onDebugLog", …)only when opted in.Fixes found while building
unbindAll()to letProcessingQueueflush.NSNullvs absent key now correctly disable a module vs keep SDK defaults; previously all modules silently activated with defaults when onlydocumentCaptureModulewas set.logSessionSettingswrapped in#if DEBUG; call sites now pass the correctsourcelabel ("performScan"/"directApi"/"customScanner") instead of hardcoding"performScan"for all paths.Navigator.popon timeout cancel —_onScanTimeoutwas calling_safePop()aftercancel(), then theBlinkIdScanCancelExceptionhandler called it again; removed the redundant call.Example app
BlinkID/example/lib/custom_scanner_screen.dart— reference implementation with:onFlipCompletehandoff)Home screen result display:
CircleAvatarfrom base64)DD/MM/YYYY(no changes to generated/native types)Screen.Recording.2026-08-04.at.09.19.18.mov
Docs
README.md— new "Custom scanner UI" section with lifecycle walkthrough, guidance stream example, flip phase, camera selection, debug logging, and quick-start snippet.preferredCameraoninitialize()andswitchCamera()with code examples.BlinkID/example/README.md— rewritten with actual content covering all three scanning modes.Test plan
performScanstill works (no regression)setDebugLogging(true)shows[BlinkID]logs in Flutter consoleScanningSettingscorrectly disable those modulescancel()/reset()/dispose()don't leave dangling sessions or coroutine scopes