responsively-app icon indicating copy to clipboard operation
responsively-app copied to clipboard

Add screen shot all devices functionality

Open durgakiran opened this issue 3 years ago • 11 comments

✨ 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

image

+ 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.

! 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.

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. Google-all-2021-10-5

durgakiran avatar May 08 '21 12:05 durgakiran

@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.

manojVivek avatar May 09 '21 03:05 manojVivek

@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?

durgakiran avatar May 09 '21 04:05 durgakiran

I am converting this pull request into draft as I observed a bug.

durgakiran avatar May 10 '21 03:05 durgakiran

@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 avatar May 10 '21 03:05 durgakiran

@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 avatar May 10 '21 16:05 manojVivek

@manojVivek yes, yes I faced it. Maybe because I was almost completely recoded the screenshot all functionality. So, I might have missed something.

durgakiran avatar May 10 '21 16:05 durgakiran

@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.

manojVivek avatar May 10 '21 16:05 manojVivek

Ok I will look into that, thanks

durgakiran avatar May 10 '21 16:05 durgakiran

@manojVivek I have fixed that issue using webView.scrollIntoView(). It is now working. Please review the PR.

durgakiran avatar May 11 '21 11:05 durgakiran

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.

manojVivek avatar May 22 '21 12:05 manojVivek

Hey still is in PR.. would appreciate this functionality. :)

rille111 avatar Mar 24 '22 10:03 rille111

CLA assistant check
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.

CLAassistant avatar Mar 28 '23 06:03 CLAassistant

@manojVivek thought of picking this up again but found a lot has been changed, since this PR. Will work on this issue again

durgakiran avatar Apr 07 '23 16:04 durgakiran

@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.

manojVivek avatar Apr 07 '23 19:04 manojVivek