responsively-app
responsively-app copied to clipboard
Add screen shot all devices functionality
✨ Pull Request
📓 Referenced Issue
closes #541
ℹ️ About the PR
Give take a screenshot of all device Web views at once, including merging of images.
Features:
+ Choosing screens to capture
+ Choosing screens to capture
+ Capture fail and recovery mechanism
+ Capture fail and recovery mechanism
One of the existing issues with the existing screenshot mechanism is it extremely fail prone. The main reason as I observed is crash electron main render thread, which fails and never recovers when continuously scrollViewPort
function is called. One more issue with the current screenshot capture is if the render thread fails to recover entire application needs to be reloaded.
To counter such fails we introduce a function that returns a promise with a retry mechanism limited to 3 retries. The function listens to webContents crashed
event. we use crashed
event to understand the if webview is killed or just crashed. if Webview is just crashed, the code will again load the crashed web view. Once webview reloads function will listen to dom-ready
event. which notifies once the web view is completely loaded, then we try to capture the image again.
captureWithRetry(retry: number = 3): Promise<Electron.NativeImage> {
return new Promise((resolve, reject) => {
let retryCount = 0;
const address = this.webView.getAttribute('src');
const handler = (event, killed) => {
if (!killed) {
this.webView.setAttribute('src', address);
} else {
reject(new Error('WebView killed.... : ('));
}
};
this.webContents.on('crashed', handler);
const callGetImage = () => {
// noinspection JSIgnoredPromiseFromCall
getImage();
};
this.webContents.on('dom-ready', callGetImage);
const getImage = async () => {
if (retryCount === retry) {
reject(new Error('Unable to produce image'));
}
retryCount += 1;
const image = await this.captureFullHeightScreenShot();
this.webContents.removeListener('dom-ready', callGetImage);
this.webContents.removeListener('crashed', handler);
resolve(image);
};
// noinspection JSIgnoredPromiseFromCall
getImage();
});
}
If Image capture fails beyond the threshold i.e, 3. The code will skip that particular web view and proceed with other web views. This way we are not abandoning the app in a failed state.
+ Merging images
Each web view, whose image is captured, binary data of the image is store in the memory. Once completion of taking individual screenshots is done. The binary data of each data is combined by drawing on a canvas. The canvas data again converted to binary and saved to the folder. The merging of images **does not** block Electron's main threads.
+ Merging images
! Caution: Merging of images takes a lot of time
! Caution: Merging of images takes a lot of time
+ Final indication
Once the capture and saving process is over, automatically the created folder is opened.
+ Final indication
There are some improvements in code refactoring and structure, I think those are self-evident in the PR file changes.
🖼️ Testing Scenarios / Screenshots
please click here to view feature demo video.
Example merged screenshots.
@durgakiran I think, it would be more useful if we add the device name above the screenshot. What do you think?
This will help the use case of showcasing the screenshots.
@durgakiran I think, it would be more useful if we add the device name above the screenshot. What do you think?
This will help the use case of showcasing the screenshots.
I thought about it and forgot to add that functionality. I will update the PR. any other changes you recommend?
I am converting this pull request into draft as I observed a bug.
@manojVivek one thing I observed was when a screenshot of a web view is happening, the screenshot of that webview is not happening until that web view is in the view port of the electron, any reason in your mind about this behaviour? It just does not seem logical to me? what do you think?
@manojVivek one thing I observed was when a screenshot of a web view is happening, the screenshot of that webview is not happening until that web view is in the view port of the electron, any reason in your mind about this behaviour? It just does not seem logical to me? what do you think?
@durgakiran Yeah that is how Chromium behaves. Maybe they don't render/paint the sections of the screen if they are outside the viewport. We handle this behaviour by zoom-out the web-views before the screenshot is taken so that all the web-views are in the viewport and then process the screenshot. Did you face it during your testing? Ideally, you shouldn't have.
@manojVivek yes, yes I faced it. Maybe because I was almost completely recoded the screenshot all functionality. So, I might have missed something.
@durgakiran Yeah look like you missed it. Here is where it is done in the current version: https://github.com/responsively-org/responsively-app/blob/561e1c2ba545e487097a424c7d7d57ddc6c3fe43/desktop-app/app/components/WebView/screenshotUtil.js#L296
The 0.01
arg is the screenshot level.
Ok I will look into that, thanks
@manojVivek I have fixed that issue using webView.scrollIntoView()
. It is now working. Please review the PR.
As far as design changes for png background, I could not see any point in it. It unnecessarily increases the file size.
We can save the image as jpeg
and that will have reduced file size.
The reason why I'm insisting it is having non-transparent background is that is going to make it easier for the users to just share/use it without any additional processing.
Hey still is in PR.. would appreciate this functionality. :)
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 8 committers have signed the CLA.
:white_check_mark: manojVivek
:white_check_mark: durgakiran
:x: karllabrador
:x: rishichawda
:x: ziabs
:x: allcontributors[bot]
:x: jjavierdguezas
:x: Manoj Vivek
Manoj Vivek seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.
@manojVivek thought of picking this up again but found a lot has been changed, since this PR. Will work on this issue again
@durgakiran Great to see you back again. Yeah the app has went through a full revamp, and should be relatively easier to add new features.