chore: update Kscan to 0.6.0#1164
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates format representations into a new public enum and platform mappers, removes legacy BarcodeFormats objects, refactors scanner UI into ScannerViewContent, updates analyzer/camera flows (including inverted-image re-scan and background session threading), adds iOS utilities and tests, and bumps a Gradle dependency and README references. Changes
Sequence Diagram(s)sequenceDiagram
participant CameraProvider as CameraProvider
participant ImageAnalysis as ImageAnalysis
participant Analyzer as BarcodeAnalyzer
participant ScannerLib as MLKit/AVScanner
participant UI as ScannerView/ResultHandler
CameraProvider->>ImageAnalysis: deliver ImageProxy
ImageAnalysis->>Analyzer: analyze(imageProxy)
Analyzer->>ScannerLib: process(image)
ScannerLib-->>Analyzer: barcodes[]
Analyzer->>Analyzer: filter by requested formats (BarcodeFormatMapper)
alt matching barcodes & duplicate check
Analyzer->>UI: emit OnSuccess
UI->>CameraProvider: unbind/stop
Analyzer->>ImageAnalysis: close ImageProxy
else none relevant
Analyzer->>Analyzer: create inverted NV21-like image
Analyzer->>ScannerLib: process(inverted image)
ScannerLib-->>Analyzer: barcodes[]
alt matching barcodes found
Analyzer->>UI: emit OnSuccess
UI->>CameraProvider: unbind/stop
else still none
Analyzer->>UI: emit OnFailed / continue
end
Analyzer->>ImageAnalysis: close ImageProxy
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/BarcodeAnalyzer.kt`:
- Around line 119-156: The Y-plane padding can corrupt the NV21 layout; in
createInvertedInputImage you must copy only the tightly packed Y bytes (width *
height) into nv21Bytes instead of using yBuffer.remaining() (ySize). Read
yPlane.rowStride and copy row-by-row: for each row read width bytes from yBuffer
at the proper row offset into nv21Bytes (taking into account rowStride), apply
the inversion per byte while writing into the target buffer, and stop after
width*height bytes; then fill the UV section starting at offset width*height
with 127. Update references: createInvertedInputImage, yPlane, yBuffer, yBytes,
nv21Bytes, and replace the single bulk System.arraycopy with a row-wise copy
that strips padding.
In
`@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt`:
- Around line 91-95: LaunchedEffect is keyed on both initializationError and the
function parameter result, so if result is a new lambda on recomposition the
effect will restart and repeatedly call result(BarcodeResult.OnFailed(...)); fix
by keying the LaunchedEffect only on initializationError (use
LaunchedEffect(initializationError)) or keep result stable via
rememberUpdatedState(result) and use the remembered value inside the effect;
update the ScannerView.android.kt usage around LaunchedEffect,
initializationError, and result accordingly.
- Around line 138-151: The BarcodeAnalyzer is capturing the outer nullable var
camera while it's still null (passed at imageAnalysis.setAnalyzer), so later
updates from bindCamera() don't reach it; change the flow to ensure a non-null
Camera is provided after bindCamera() — either call imageAnalysis.setAnalyzer
only after bindCamera() returns the Camera (i.e., move the setAnalyzer into the
success path of bindCamera()), or modify BarcodeAnalyzer to accept a
supplier/lambda (e.g., () -> Camera?) instead of Camera so it defers reading the
outer camera; update usages in scannerOptions within BarcodeAnalyzer accordingly
to call the supplier when needed.
🧹 Nitpick comments (6)
shared/kscan/src/commonMain/kotlin/org/ncgroup/kscan/BarcodeFormat.kt (1)
29-57: Mixing barcode formats and value types in a single enum.The enum combines two distinct concepts — physical encoding formats (
FORMAT_*) and data content types (TYPE_*). While this mirrors the upstream KScan design and the KDoc clearly documents both categories, consider whether a future split into two enums would improve type safety (e.g., preventing aTYPE_*value from being passed where only a scan format is expected). Not blocking since this aligns with upstream.shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/BarcodeAnalyzer.kt (1)
100-116: DoubleimageProxy.close()on failure/cancel in inverted scan path.
addOnFailureListener(Line 109) andaddOnCanceledListener(Line 112) each closeimageProxy, butaddOnCompleteListener(Line 115) also closes it unconditionally. WhileImageProxy.close()is idempotent in CameraX, the intent is clearer if onlyaddOnCompleteListenerhandles cleanup:♻️ Suggested cleanup
scanner .process(invertedImage) .addOnSuccessListener { barcodes -> val relevantBarcodes = barcodes.filter { isRequestedFormat(it) } if (relevantBarcodes.isNotEmpty()) { processFoundBarcodes(relevantBarcodes) } }.addOnFailureListener { onFailed(it) - imageProxy.close() }.addOnCanceledListener { onCanceled() - imageProxy.close() }.addOnCompleteListener { // CRITICAL: Always close the proxy after the final attempt imageProxy.close() }shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt (1)
166-178: Redundant cleanup: bothonReleaseandDisposableEffectcallunbindAll().
AndroidView.onRelease(Line 167) callsprovider.unbindAll(), andDisposableEffect(Line 174) also callscameraProvider?.unbindAll(). While double-unbinding is likely safe (idempotent), the duplication is unnecessary. Consider consolidating cleanup in one place.shared/kscan/src/iosMain/kotlin/org/ncgroup/kscan/BarcodeFormatMapper.kt (1)
38-43: Behavioral asymmetry with Android: empty mapping result returns empty list instead of falling back.On Android, if no app formats map to valid ML Kit formats,
toMlKitFormatsfalls back toFORMAT_ALL_FORMATS. Here on iOS,toAvTypesreturns an empty list, which is then caught downstream inCameraViewController.setupMetadataOutputas an error. The behavior is safe (the error is reported), but the inconsistency means the same invalid input produces a scan-all-formats fallback on Android vs. an error on iOS.If this is intentional, consider adding a brief comment explaining the design choice.
shared/kscan/src/iosMain/kotlin/org/ncgroup/kscan/CameraViewController.kt (2)
97-99: Good improvement: movingstartRunning()/stopRunning()off the main thread.Apple's documentation states that
AVCaptureSession.startRunning()andstopRunning()are blocking calls that should not be invoked on the main thread. Dispatching them to a global queue is the correct approach.The pattern
dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT.toLong(), 0u)is repeated 5 times. Consider extracting it to a private property for readability.Optional: extract background queue
class CameraViewController( ... ) : UIViewController(null, null), AVCaptureMetadataOutputObjectsDelegateProtocol { + private val backgroundQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT.toLong(), 0u) + private lateinit var captureSession: AVCaptureSessionThen replace all occurrences with
backgroundQueue.Also applies to: 124-128, 133-137, 188-192, 247-260
243-243:toFormat()extension is only used once — consider inlining.The private extension
AVMetadataObjectType.toFormat()(Line 243) wraps a singleBarcodeFormatMapper.toAppFormat(this)call and is used only at Line 176. It adds indirection without much readability benefit. Consider calling the mapper directly.
shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/BarcodeAnalyzer.kt
Show resolved
Hide resolved
shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt
Outdated
Show resolved
Hide resolved
|
haha I think we can ignore this 😆 |
|
Hey |
|
@wodoro I've did a quick review on this one and it looks good to me. Just concerned about the CRAI Y-plan comment, were you able to have a look into it? what are your thoughts? |
|
Well most I could do was feed it to different AI and apply the suggestion |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/BarcodeAnalyzer.kt (1)
91-117: Double-close ofimageProxyinscanInvertedfailure/cancel paths.
addOnFailureListenerandaddOnCanceledListenerboth callimageProxy.close(), and thenaddOnCompleteListener(which fires unconditionally after any of the above) also callsimageProxy.close(). WhileImageProxy.close()is idempotent, this contradicts the intent expressed in the line 115 comment ("CRITICAL: Always close the proxy after the final attempt"). Remove theimageProxy.close()calls from the failure and cancel listeners to let the complete listener be the sole closer—matching the stated design intent.Proposed fix
}.addOnFailureListener { onFailed(it) - imageProxy.close() }.addOnCanceledListener { onCanceled() - imageProxy.close() }.addOnCompleteListener { // CRITICAL: Always close the proxy after the final attempt imageProxy.close() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/BarcodeAnalyzer.kt` around lines 91 - 117, In scanInverted, remove the explicit imageProxy.close() calls from the addOnFailureListener and addOnCanceledListener so that only the addOnCompleteListener closes the ImageProxy; keep calling onFailed(it) in the failure listener and onCanceled() in the canceled listener, and retain the early-return catch block that already closes imageProxy if createInvertedInputImage fails—this ensures addOnCompleteListener is the sole closer as intended.shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt (1)
184-207: Nit:cameraProviderFutureparameter name is misleading.This parameter is a
ProcessCameraProvider, not a future. A name likecameraProviderorproviderwould be clearer. Minor since this appears to be inherited code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt` around lines 184 - 207, The parameter name cameraProviderFuture in the bindCamera function is misleading (it is a ProcessCameraProvider, not a future); rename the parameter to cameraProvider (or provider) and update all uses inside bindCamera (the unbindAll call, the safe-call chain, and bindToLifecycle invocation) as well as the function signature and any call sites to use the new name to keep the API clear and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/BarcodeAnalyzer.kt`:
- Around line 93-99: The catch block around createInvertedInputImage in
BarcodeAnalyzer currently swallows all exceptions; update it to record the
exception before cleaning up so diagnostics are preserved (e.g., call your
logger/Log.e/Timber with a descriptive message and the exception), then close
imageProxy and return; ensure the log includes the imageProxy/state context and
the exception so you can trace null/empty-planes or other failures originating
in createInvertedInputImage.
In
`@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt`:
- Around line 141-154: The lambdas passed into BarcodeAnalyzer inside the
AndroidView.factory capture the original result reference and can go stale on
recomposition; replace direct calls to result(...) with the rememberUpdatedState
wrapper (updatedResult) so the latest callback is always invoked (e.g., use
updatedResult.value(...) in the onSuccess, onFailed and onCanceled lambdas
inside the BarcodeAnalyzer instantiation), leaving other behavior (like
provider.unbind(imageAnalysis)) unchanged.
---
Nitpick comments:
In `@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/BarcodeAnalyzer.kt`:
- Around line 91-117: In scanInverted, remove the explicit imageProxy.close()
calls from the addOnFailureListener and addOnCanceledListener so that only the
addOnCompleteListener closes the ImageProxy; keep calling onFailed(it) in the
failure listener and onCanceled() in the canceled listener, and retain the
early-return catch block that already closes imageProxy if
createInvertedInputImage fails—this ensures addOnCompleteListener is the sole
closer as intended.
In
`@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt`:
- Around line 184-207: The parameter name cameraProviderFuture in the bindCamera
function is misleading (it is a ProcessCameraProvider, not a future); rename the
parameter to cameraProvider (or provider) and update all uses inside bindCamera
(the unbindAll call, the safe-call chain, and bindToLifecycle invocation) as
well as the function signature and any call sites to use the new name to keep
the API clear and consistent.
shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/BarcodeAnalyzer.kt
Show resolved
Hide resolved
shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt (1)
156-165:result = resultis inconsistent with theupdatedResultpattern used elsewhere.
bindCameraonly invokesresultsynchronously withingetOrElseduring the factory call, so there is no stale-lambda risk here. However, for consistency with the rest of the file, consider forwardingupdatedResult:♻️ Proposed change
- result = result, + result = { updatedResult(it) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt` around lines 156 - 165, The call to bindCamera passes result = result which is inconsistent with the file's updatedResult pattern; change the argument to forward updatedResult (i.e., result = updatedResult) so bindCamera receives the same stabilized lambda used elsewhere (refer to the updatedResult variable and the bindCamera(...) invocation) to maintain consistency and avoid divergence between synchronous and updated result handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt`:
- Around line 156-165: The call to bindCamera passes result = result which is
inconsistent with the file's updatedResult pattern; change the argument to
forward updatedResult (i.e., result = updatedResult) so bindCamera receives the
same stabilized lambda used elsewhere (refer to the updatedResult variable and
the bindCamera(...) invocation) to maintain consistency and avoid divergence
between synchronous and updated result handlers.
|
rebased to main |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
shared/kscan/src/iosMain/kotlin/org/ncgroup/kscan/CameraViewController.kt (1)
245-268: Ordering note indispose(): preview layer removal races with background session teardown.The preview layer is removed from the superlayer synchronously (main thread) while the session stop + input/output removal runs asynchronously on a background queue. This means the session could still be running briefly after its preview layer is gone. In practice this is harmless (the
runCatchingblocks prevent crashes), but if you observe any visual artifacts during dismissal, consider movingpreviewLayer.removeFromSuperlayer()into the background block's completion or using a serial queue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/kscan/src/iosMain/kotlin/org/ncgroup/kscan/CameraViewController.kt` around lines 245 - 268, The dispose() method currently removes previewLayer on the main thread immediately while stopping captureSession asynchronously, which can race and leave the session running after the layer is gone; move the previewLayer.removeFromSuperlayer() call so it executes after the background teardown completes (either by performing it on the same background block after inputs/outputs are removed, or by dispatching a completion back to main from that block), ensuring you still guard with ::previewLayer.isInitialized and keep barcodesDetected.clear() as appropriate; update references to dispose(), previewLayer, and captureSession accordingly.shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt (1)
156-165:bindCamerareceives the rawresultinstead ofupdatedResult.Inside the
factorylambda (which runs once),bindCamerais called withresult = result(line 163). Its error handler at line 205 invokes this reference directly. Currently this is safe becausebindCameraexecutes synchronously during factory creation, but it's inconsistent with theupdatedResultusage elsewhere in the same block. IfbindCamerawere ever made asynchronous, this would silently revert to the stale-callback bug.Suggested fix for consistency
camera = bindCamera( lifecycleOwner = lifecycleOwner, cameraProviderFuture = provider, selector = selector, preview = preview, imageAnalysis = imageAnalysis, - result = result, + result = updatedResult, cameraControl = { cameraControl = it }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt` around lines 156 - 165, The bindCamera call inside the factory currently passes the original result reference (result) which can become stale; change the call to pass updatedResult instead (i.e., result = updatedResult) and make sure any error handler callbacks invoked by bindCamera also reference updatedResult rather than the original result so the latest callback is used if bindCamera becomes asynchronous; update uses of result within the factory/error handling to consistently use updatedResult and preserve the existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shared/kscan/src/iosMain/kotlin/org/ncgroup/kscan/BarcodeFormatMapper.kt`:
- Around line 38-43: The toAvTypes function currently returns an empty list when
none of the requested BarcodeFormat values map via APP_TO_AV_FORMAT_MAP, causing
CameraViewController.setupMetadataOutput to error; to align behavior with
Android's toMlKitFormats, modify toAvTypes to detect this case and fall back to
allSupportedTypes (or explicitly include BarcodeFormat.FORMAT_ALL_FORMATS
semantics) when appFormats is non-empty but mapNotNull yields an empty result,
ensuring callers receive the same lenient default as Android; update references
in toAvTypes, APP_TO_AV_FORMAT_MAP, and tests that exercise
CameraViewController.setupMetadataOutput accordingly.
---
Duplicate comments:
In `@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/BarcodeAnalyzer.kt`:
- Around line 91-99: In scanInverted, don’t silently swallow exceptions from
createInvertedInputImage: either call the existing onFailed(...) fallback with
the caught exception or at minimum log the exception before closing imageProxy
and returning; update the catch block in scanInverted to capture the Exception e
and pass it to onFailed(e) or processLogger/errorLogger with a clear message,
then imageProxy.close() and return so diagnostic context is preserved.
- Around line 118-156: This review is a duplicate approval—no code change is
required in createInvertedInputImage (the Y-plane handling, inversion via
nv21Bytes and neutral chroma fill are correct); remove the duplicate review
comment/mark ([duplicate_comment]) and keep a single [approve_code_changes] so
the PR shows one approval rather than two.
In
`@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt`:
- Around line 141-154: The BarcodeAnalyzer callbacks already use updatedResult
to avoid stale references (see BarcodeAnalyzer usage in
imageAnalysis.setAnalyzer and updatedResult calls for
onSuccess/onFailed/onCanceled), so no code change is necessary; mark the earlier
review comment as a duplicate and approve the change.
- Around line 94-98: The change to key LaunchedEffect on initializationError and
to call updatedResult(BarcodeResult.OnFailed(Exception(initializationError))) is
correct; no code change is required—keep the LaunchedEffect block as written
(LaunchedEffect(initializationError) { if (initializationError != null) {
updatedResult(BarcodeResult.OnFailed(Exception(initializationError))) } }) and
remove any duplicate review/comment if present.
---
Nitpick comments:
In
`@shared/kscan/src/androidMain/kotlin/org/ncgroup/kscan/ScannerView.android.kt`:
- Around line 156-165: The bindCamera call inside the factory currently passes
the original result reference (result) which can become stale; change the call
to pass updatedResult instead (i.e., result = updatedResult) and make sure any
error handler callbacks invoked by bindCamera also reference updatedResult
rather than the original result so the latest callback is used if bindCamera
becomes asynchronous; update uses of result within the factory/error handling to
consistently use updatedResult and preserve the existing behavior.
In `@shared/kscan/src/iosMain/kotlin/org/ncgroup/kscan/CameraViewController.kt`:
- Around line 245-268: The dispose() method currently removes previewLayer on
the main thread immediately while stopping captureSession asynchronously, which
can race and leave the session running after the layer is gone; move the
previewLayer.removeFromSuperlayer() call so it executes after the background
teardown completes (either by performing it on the same background block after
inputs/outputs are removed, or by dispatching a completion back to main from
that block), ensuring you still guard with ::previewLayer.isInitialized and keep
barcodesDetected.clear() as appropriate; update references to dispose(),
previewLayer, and captureSession accordingly.
| fun toAvTypes(appFormats: List<BarcodeFormat>): List<AVMetadataObjectType> { | ||
| if (appFormats.isEmpty() || appFormats.contains(BarcodeFormat.FORMAT_ALL_FORMATS)) { | ||
| return allSupportedTypes | ||
| } | ||
| return appFormats.mapNotNull { APP_TO_AV_FORMAT_MAP[it] } | ||
| } |
There was a problem hiding this comment.
Behavioral inconsistency with Android mapper when no formats map successfully.
On Android, toMlKitFormats falls back to FORMAT_ALL_FORMATS when all requested formats are unmappable (e.g., requesting FORMAT_CODABAR which has no iOS equivalent). On iOS, toAvTypes returns an empty list, which then triggers "No supported barcode types selected" error in CameraViewController.setupMetadataOutput.
This may be intentional (fail-fast on iOS vs. lenient on Android), but the inconsistency could surprise callers. Consider either aligning the behavior or documenting the difference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shared/kscan/src/iosMain/kotlin/org/ncgroup/kscan/BarcodeFormatMapper.kt`
around lines 38 - 43, The toAvTypes function currently returns an empty list
when none of the requested BarcodeFormat values map via APP_TO_AV_FORMAT_MAP,
causing CameraViewController.setupMetadataOutput to error; to align behavior
with Android's toMlKitFormats, modify toAvTypes to detect this case and fall
back to allSupportedTypes (or explicitly include
BarcodeFormat.FORMAT_ALL_FORMATS semantics) when appFormats is non-empty but
mapNotNull yields an empty result, ensuring callers receive the same lenient
default as Android; update references in toAvTypes, APP_TO_AV_FORMAT_MAP, and
tests that exercise CameraViewController.setupMetadataOutput accordingly.
|
ignoring PR coverage check for the reasons above |
2 participants
There's small differences between our version of Kscan and upstream
I tried to keep our UI changes, while updating the rest and fixed the Ktlint issues
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation
Tests
Chores