feat: add electron.DesktopCapturer.setSkipCursor() method

[danielehrhardt]

I opened this pull request because the creator of the Original Pull Request closed it without any comments. To read the hole discussion, go here: #28302

Description of Change
feat: Implement electron.DesktopCapturer.setSkipCursor

Useful to dynamically stop cursor capture in the stream.
By default the cursor is captured.
First argument is a deviceId so one can stop capturing
the cursor for a specific streamed source.

#23923

This is essentially a cherrypick of the the previous implementation of @CapOM

#25103

Checklist
PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.
Release Notes
Notes: Implemented DesktopCapturer.setSkipCursor API. Useful to dynamically stop cursor capture in the stream.

[welcome]

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

• fix: don't overwrite prevent_default if default wasn't prevented
• feat: add app.isPackaged() method
• docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

• Follow the JavaScript, C++, and Python coding style.
• Run npm run lint locally to catch formatting errors earlier.
• Document any user-facing changes you've made following the documentation styleguide.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[zcbenz]

I'm good with the API, but I'm afraid the size of the patch is still going to cause concerns as the same with #28302.

[asineth0]

PR looks good to me, need this functionality for my application so I'm willing to help get this merged if there's anything I can do.

[danielehrhardt]

I'm good with the API, but I'm afraid the size of the patch is still going to cause concerns as the same with #28302.

Do you see any way to make the size of the patch smaller?

[zcbenz]

As per #28302 (review), the best way to get this merged is to try to upstream the patch to Chromium first.

[asineth0]

As per #28302 (review), the best way to get this merged is to try to upstream the patch to Chromium first.

This adds to the desktopCapturer API in Electron. Makes sense to merge it here.

[zcbenz]

I'm neutral on this patch, but the maintenance work will lay on @electron/wg-upgrades and we should follow their decision (so it does not matter how I think about the PR). For this PR, guidance in #28302 (review) should be followed.

[nornagon]

I remain 👎 on a patch of this size.

Are there possible workarounds without this patch, e.g. stopping & starting the stream w/ changed options?

[asineth0]

Disabling/enabling the cursor without restarting the stream would be preferable especially for apps that are capturing things like games or other interactive content that capture the mouse. Besides, the patch isn't that large, and it includes tests.

[michalzaq12]

@asineth0 From your reply it appears that there is an option to disable the cursor, but it requires stream restart?

Could you elaborate on how to do this?

[danielehrhardt]

Could maybe someone upstream this to the chromium project?
I have no clue how to do this.

I think this feature can be useful in so many ways and should be really integrated.

[codebytere]

@danielehrhardt here's a guide to contributing to Chromium - i can try to open an upstream CL if you'd prefer though. Can you also please rebase this?

[danielehrhardt]

@danielehrhardt here's a guide to contributing to Chromium - i can try to open an upstream CL if you'd prefer though. Can you also please rebase this?

This would be great.
I did the rebase.

[asineth0]

@asineth0 From your reply it appears that there is an option to disable the cursor, but it requires stream restart?

Could you elaborate on how to do this?

As far as I know, you can't currently toggle the cursor at all. Although, if you know how, please let me know. My use case is a chat app where users can screenshare so transparently restarting the stream might not be out of the question, although it's certainly not ideal compared to just calling setSkipCursor.

Could maybe someone upstream this to the chromium project?
I have no clue how to do this.

I think this feature can be useful in so many ways and should be really integrated.

I feel like this should be done in Electron. Chromium is unlikely to upstream a patch like this since it has no corresponding web standard, and it's unlikely that we'd convince the W3C to add it to one easily.

Electron has it's own desktopCapturer object for a reason, so I see no problems with expanding it's functionality.

[asineth0]

Since #30685 has been merged, maybe we could get this merged easier?

[danielehrhardt]

</3

[zcbenz]

The size of patch has been reduced quite a lot, /cc @electron/wg-upgrades for review.

[miniak]

the desktopCapturer is now only available in the main process. you need to rebase and refactor your PR to accommodate for this change.

[danielehrhardt]

the desktopCapturer is now only available in the main process. you need to rebase and refactor your PR to accommodate for this change.

Is there any chance if i do the rebase and refactor of the PR that it will get merged? I will not invest the work if it is not worth it.

[nornagon]

I think this could be better addressed by changes/extensions to #30702, through getDisplayMedia. It seems like there's a specced (but currently unimplemented) way through getDisplayMedia to do this: https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/cursor and https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/applyConstraints. We should support the specced version.

I'm going to close this for now and add a comment to #30702 to mark that we want to investigate supporting this through getDisplayMedia.

[samuelmaddock]

Just following up to mention that the upstream issue for adding support for this MediaStreamTrack constraint in Chromium is here: https://bugs.chromium.org/p/chromium/issues/detail?id=1007177