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

Nadro/3079/screenshot improvements #3917

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

nadr0
Copy link
Contributor

@nadr0 nadr0 commented Sep 18, 2024

closes #3079

Documentation https://www.electronjs.org/docs/latest/api/desktop-capturer

Issue

html2canvas or html2canvas-pro work but are fragile and not robust enough. Overtime with libraries being updated or new components we create the "screenshot" feature with this library will break.

Proof of breaking

html2canvas-pro is broken in electron, chrome browser, and edge browser while using linux OS.

electron-broken-modeling-view
chrome-linux-broken-modeling-view
edge-linux-broken-modeling-view

Solution

If users are using the browser based application the screenshot tool will default to taking a snapshot from the video element.

If users are using the desktop application in electron it will default to the native desktop solution in electron to take a screenshot. If the electron desktop screenshot fails, it will fall back to video element stream.

I added an extra comment to the github issue to indicate that browser screenshots are limited.

I removed the html2canvas-pro package.

Example from browser
example1

Example from electron
example2

Copy link

qa-wolf bot commented Sep 18, 2024

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

Copy link

vercel bot commented Sep 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Sep 20, 2024 4:42pm

src/main.ts Outdated
})

for (const source of sources) {
if (source.name === 'Zoo Modeling App') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be a constant so that we refer to the same title that is used when creating the window?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/KittyCAD/modeling-app/pull/3917/files#diff-4fab5baaca5c14d2de62d8d2fceef376ddddcc8e9509d86cfa5643f51b89ce3dR175-R176

I've updated this to use the productName from package.json since that is what is used in electron-builder.
I think this patter is okay.

We do not use the API defined in Browser-Window to set the title of the browser window. So we don't have another reference within the code base. It is only defined in the package.json.

src/main.ts Outdated Show resolved Hide resolved
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.14%. Comparing base (01cc9e7) to head (26d79e3).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/wasm-lib/kcl/src/coredump/mod.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3917      +/-   ##
==========================================
- Coverage   87.15%   87.14%   -0.02%     
==========================================
  Files          69       69              
  Lines       25121    25124       +3     
==========================================
  Hits        21894    21894              
- Misses       3227     3230       +3     
Flag Coverage Δ
wasm-lib 87.14% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nadr0
Copy link
Contributor Author

nadr0 commented Sep 19, 2024

I read that MacOS can prevent screenshots by default. I can get access to see if it is enabled or not.

I cannot ask for one time permissions for a screenshot though since the ask API does not allow screen.

My new plan:

Table based on the documentation of operating systems when asking for media

System Native Screenshot Video Canvas Stream screenshot
Browsers ✔️
Linux ✔️ fallback if native fails
Windows ✔️ fallback if native fails
MacOS needs to enable in system preferences default or fallback if native fails

We can always come back to how to improve the MacOS workflow. If a user wants to enable it manually they will get native screenshot support otherwise it will default to the video canvas stream.

I've updated the github bug ticket to have an HTML comment that says

<!--
  If you are capturing from a browser there is limited support for screenshots, only captures the modeling scene.
  If you are on MacOS native screenshots may be disabled by default. To enable native screenshots add Zoo Modeling App to System Settings -> Screen & SystemAudio Recording for native screenshots.
-->

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.

Screenshot attached to this issue is not what I see on my screen
2 participants