Skip to content
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

frontend/qrcodescanner: improve qrscanner component #3048

Merged

Conversation

Beerosagos
Copy link
Collaborator

7c0b2f6 introduced a 300ms wait between the QrScanner creation and its start to fix a bug on re-renders of the component, but it seems not to be enough on certain devices, which still show the bug.

It would be nice to change the 300ms fixed wait to a better way to know when the scanner is ready to be started but we could not find it.

This commit improves the general structure of the component, to limit race conditions across re-renders and seems to improve the behavior and fix the bug with the tested devices.

7c0b2f6 introduced a 300ms wait between the QrScanner creation and its
start to fix a bug on re-renders of the component, but it seems not
to be enough on certain devices, which still show the bug.

It would be nice to change the 300ms fixed wait to a better way to know
when the scanner is ready to be started but we could not find it.

This commit improves the general structure of the component, to limit
race conditions across re-renders and seems to improve the behavior and
fix the bug with the tested devices.
@Beerosagos Beerosagos requested review from thisconnect and shonsirsha and removed request for thisconnect November 12, 2024 13:52
@thisconnect
Copy link
Collaborator

I cannot reproduce the issue @strmci can you test this?

@strmci
Copy link
Collaborator

strmci commented Nov 12, 2024

I cannot reproduce the issue @strmci can you test this?

I already tested it, @Beerosagos sent me the apk

@thisconnect
Copy link
Collaborator

I already tested it

and?

@strmci
Copy link
Collaborator

strmci commented Nov 12, 2024

I already tested it

and?

worked fine, on my Android


return () => {
(async() => {
while (loading.current) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these while loops make me a little nervous 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😂 I know it's not exactly the react way to solve a problem like this. Feel free to suggest changes

@Beerosagos
Copy link
Collaborator Author

I already tested it

and?

worked fine, on my Android

@strmci could you please test again? This is not exactly the same apk you tested, even tho there should not be any relevant difference <3

@strmci
Copy link
Collaborator

strmci commented Nov 12, 2024

I already tested it

and?

worked fine, on my Android

@strmci could you please test again? This is not exactly the same apk you tested, even tho there should not be any relevant difference <3

sure, can you please share apk with me?

@strmci
Copy link
Collaborator

strmci commented Nov 12, 2024

@thisconnect @Beerosagos
tested again on my phone, QR scanner after rotation works fine, it takes approx 2 seconds to reload the camera after the rotation

Copy link
Collaborator

@thisconnect thisconnect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could not reproduce on Android nor macOS, but looks good. Android 14 and macOS 14.7 load the camera

@Beerosagos Beerosagos merged commit 91e4ed7 into BitBoxSwiss:release-v4.46.0 Nov 13, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants