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

Screen sharing feature UI adjustment and code refactor #12

Merged
merged 4 commits into from Jan 7, 2022

Conversation

ttycelery
Copy link
Owner

@ttycelery ttycelery commented Jan 6, 2022

In this PR, I tried to tidy things up especially the UI. As a follow-up of #9, in sum, this PR have done the following things:

  • UI adjustment: added a select box for selecting a QR source and moved the auto submit checkbox to the bottom of the page
  • Auto submit checkbox state is now persistent by utilizing localStorage
  • Refactored praesentia.js
  • I also re-organized the static files for nimiq/qr-scanner as it looks quite messy (this should be in another PR, but I guess that's okay)

@ttycelery
Copy link
Owner Author

What do you think? @Noth3r

Feel free to test and/or give suggestions to it.

@ttycelery ttycelery changed the title Screen sharing feature UI adjustment Screen sharing feature UI adjustment and code refactor Jan 6, 2022
@Noth3r
Copy link
Contributor

Noth3r commented Jan 7, 2022

Nice PR. It works perfectly. But I have a little suggestion to stick with the createQrEngine because in my pc if I create the engine manually, my chrome has lower CPU usage. How about yours?
lower_cpu

In nimiq/qr-scanner docs

As an optional third parameter a manually created QR scanner engine instance to be reused can be specified. This improves performance if you're scanning a lot of images. An engine can be manually created via QrScanner.createQrEngine(QrScanner.WORKER_PATH) (async). By default, no engine is reused for single image scanning.

@ttycelery
Copy link
Owner Author

ttycelery commented Jan 7, 2022

@Noth3r Nice catch, I didn't even know that. I can't test its performance, but we are definitely scanning a lot of images (when screen sharing) and are having performance issues. I also noticed that by not using a reusable worker, the browser requests the worker file every time it scans a frame or an image (I think this is what causes the higher CPU usage).
image

Let me wire up the changes.

@ttycelery
Copy link
Owner Author

Does it look good to you? @Noth3r

@Noth3r
Copy link
Contributor

Noth3r commented Jan 7, 2022

Yep works perfectly. Nice one @p4kl0nc4t 🎉

@ttycelery ttycelery merged commit 9d330cd into main Jan 7, 2022
@ttycelery
Copy link
Owner Author

Merged, thanks!

@ttycelery ttycelery deleted the p4kl0nc4t-screensharing-ui-patch branch January 7, 2022 10:05
ttycelery added a commit that referenced this pull request Nov 2, 2023
Screen sharing feature UI adjustment and code refactor
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.

None yet

2 participants