-
Notifications
You must be signed in to change notification settings - Fork 979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow multiple barcodes to be decoded #90
Conversation
May need to go into more detail since the result object is significantly different.
I also tweaked the packages and lint. There were some duplicates in each, and no way to run lint from command line that I could see. |
@@ -380,6 +381,10 @@ more possible clashes, or false-positives. One should take care of the order | |||
the readers are given, since some might return a value even though it is not | |||
the correct type (EAN-13 vs. UPC-A). | |||
|
|||
The `multiple` property tells the decoder if it should stop after finding a single result. If multiple is set to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please wrap your lines at 80 chars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Sorry about that, didn't notice the pattern 😄
_resultCollector.addResult(imageData, _inputStream.getCanvasSize(), result.codeResult); | ||
} | ||
} | ||
addResult(result, imageData); | ||
} | ||
|
||
Events.publish("processed", result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The detected
event is not fired any more when multiple
is activated. Maybe you could expand the condition to include something like: result.barcodes && result.barcodes.length > 0
. And since you only add the results to this array if the decoding was successful, this should be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must have overlooked the last part of that method the first time through, good catch. I just pushed a fix for it which also future-proofs the check if we decide to return an array instead of an object. I'm making the assumption that some of the barcodes might not have a codeResult
instead of just checking barcodes.length
. If you agree that's the way we should go I will commit the array result change.
Thanks for the PR. I've just tried it, and it works thus far. I noticed that you are wrapping the result in an object with a single key named |
Great suggestion. I originally had the same idea, but realized that we would then loose the ability to add |
From my point of view, separating the boxes and the results might also be a good idea, since the list of boxes is the same for each result. I would even go that far that we could always return an array of results and the consumer of the API then have to deal with that. Example of a proposed {
"barcodes": [{
"codeResult": {},
"line": [],
"angle": -0.6565217179979483,
"pattern": [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, /* ... */ 1],
"box": [],
}],
"boxes": [
[],
[]
]
} But maybe this can be implemented in the future. Depending on your response, I will merge the the pull-request either now, later. |
I went ahead and pushed my change that returns an array instead of an object. After thinking about it more, I think it makes more sense than separating them because with just one array we don't have to duplicate the boxes between |
I have added an additional configuration to the
decoder
section calledmultiple
. If it is set to true, the decoder will not return after the first valid result. The result object that gets emitted is slightly different for the end user since it has multiple barcodes on it.