Skip to content

fix: resolve appIcon error in desktopCapturer.getSources#49219

Closed
KanishkRanjan wants to merge 3 commits intoelectron:mainfrom
KanishkRanjan:kanishk/desktopcapturer-appicon-fix/39-x-y
Closed

fix: resolve appIcon error in desktopCapturer.getSources#49219
KanishkRanjan wants to merge 3 commits intoelectron:mainfrom
KanishkRanjan:kanishk/desktopcapturer-appicon-fix/39-x-y

Conversation

@KanishkRanjan
Copy link
Copy Markdown
Contributor

@KanishkRanjan KanishkRanjan commented Dec 15, 2025 •
edited
Loading

Description of Change

Fixes #48063

This issue was fixed via a patch in Chromium. However, during review, it was requested that the icon size be set to 32×32 instead of 128×128.

The Problem: The upstream Chromium implementation window_icon_util_mac.mmcaptures NSImage icons at their default logical size of {32, 32}. When converted to gfx::ImageSkia, only the 1x (32px) and 2x (64px) representations are preserved, discarding the high-resolution assets present in the source file.

The Fix: Created a local copy of the window icon utility logic (derived from window_icon_util_mac.mm) that explicitly enforces a target size of 128x128 to capture high-DPI assets. Modified electron_api_desktop_capturer to call this custom implementation instead of the upstream Chromium function, ensuring Electron receives sharp icons without altering upstream files.

Test Plan: Verified manually with a sample Electron app.

Before and After patch comparison

Before:
image

After:
image

Checklist

Release Notes

Notes: Fixed an issue where window icons returned by desktopCapturer on macOS were low resolution.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Dec 15, 2025
nikwen
nikwen requested changes Dec 15, 2025
View reviewed changes
@nikwen
Copy link
Copy Markdown
Member

nikwen commented Dec 15, 2025 •
edited
Loading

  1. Can you make the formatting of the PR description match our template?
  2. Could you add context to the PR description about why this is implemented the way it is (similar to this comment)?
  3. Could you please sign your commits?

nikwen
nikwen previously requested changes Dec 16, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@nikwen nikwen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for adjusting the PR!

  • In the PR description, it’s a good idea to link manually to the upstream patch and to the specific comment where they requested 32x32.
  • One commit is still unsigned.
  • Can you fix the grammar in the PR title? Example: fix: desktopCapturer.getSources returned empty appIcon
  • Can you remove the issue number from the title and instead add "Fixes #{issue number}" at the beginning of the description? That will make the PR automatically close that issue when it's merged.
  • Can you adjust the release notes? As far as I understand, in Electron the icon is not low-resolution at the moment, but it is empty, correct? So the PR fixes the icon being empty.

@KanishkRanjan KanishkRanjan changed the title fix: desktopCapturer.getSources got error appIcon (#48063) fix: resolve appIcon error in desktopCapturer.getSources Dec 16, 2025
@KanishkRanjan KanishkRanjan force-pushed the kanishk/desktopcapturer-appicon-fix/39-x-y branch 2 times, most recently from cfece9a to 86203b8 Compare December 16, 2025 20:53
@KanishkRanjan
Copy link
Copy Markdown
Contributor Author

KanishkRanjan commented Dec 16, 2025

Hi @nikwen,

Thank you for the review 😊. I have made changes according to review.
please review it .

Best,
Kanishk

@KanishkRanjan KanishkRanjan requested a review from nikwen December 16, 2025 21:25
@nikwen nikwen changed the title fix: resolve appIcon error in desktopCapturer.getSources fix: resolve appIcon error in desktopCapturer.getSources Dec 16, 2025
@nikwen
Copy link
Copy Markdown
Member

nikwen commented Dec 16, 2025 •
edited
Loading

Thanks, Kanishk!

I'd be fine with solving it this way. Personally, I would have done it as described in #48063 (comment) to make it easier to understand from reading the code that we overwrite the upstream macOS behavior. However, I'd be fine with either.

Let's have other reviewers weigh in on this, too.

Thanks for adding a test. That's appreciated!

@nikwen nikwen mentioned this pull request Dec 16, 2025
Closed
3 tasks
@nikwen
Copy link
Copy Markdown
Member

nikwen commented Dec 20, 2025

Not sure why the builds keep failing. Could you rebase on the latest main, please? Sometimes that solves it.

@KanishkRanjan KanishkRanjan force-pushed the kanishk/desktopcapturer-appicon-fix/39-x-y branch from a09fa8b to c880480 Compare December 21, 2025 16:58
@KanishkRanjan
Copy link
Copy Markdown
Contributor Author

KanishkRanjan commented Dec 21, 2025

@nikwen , I have rebased, PTAL.

Thank you for your time.

@nikwen nikwen added semver/minor backwards-compatible functionality target/39-x-y PR should also be added to the "39-x-y" branch. target/40-x-y PR should also be added to the "40-x-y" branch. labels Dec 22, 2025
@nikwen nikwen added semver/patch backwards-compatible bug fixes and removed semver/minor backwards-compatible functionality api-review/requested 🗳 labels Dec 22, 2025
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Dec 22, 2025
@fanhillary
Copy link
Copy Markdown

fanhillary commented Jan 12, 2026

Hi @nikwen and @KanishkRanjan , was wondering if there are any updates on this fix?
Our desktop capturer is still returning all null windowIcons on Mac26 :(.

@KanishkRanjan
Copy link
Copy Markdown
Contributor Author

KanishkRanjan commented Jan 13, 2026

@fanhillary let’s wait for nikwen’s reply. You can run native code with Electron, which allows you to extract icons ( you can take inspiration from this gist ). This gives you much finer control. If you want, I can put together a simple example program.

@nikwen
Copy link
Copy Markdown
Member

nikwen commented Jan 13, 2026

let’s wait for nikwen’s reply

This is waiting for another maintainer to take a look. I reached out to some maintainers yesterday even before @fanhillary's message but haven't received a reply yet.

@nikwen nikwen dismissed their stale review January 19, 2026 10:15

Removing my stale review.

I'm good with merging this. However, I would like another reviewer to look at #49219 (comment). I'd love to get a second opinion on what's the better way to implement this. Then I'll happily approve either of the two approaches.

@github-actions github-actions bot added the target/41-x-y PR should also be added to the "41-x-y" branch. label Jan 19, 2026
dsanders11
dsanders11 reviewed Jan 21, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

My two cents, I think the one-line patch described by @nikwen here would be the lesser of two evils. Other maintainers may disagree.

I know our patch policy says to avoid indefinite-lifetime patches, but for a one liner I think the trade off is worth avoiding the code copy, especially since this PR is adding a test case.

Copying code from upstream inherently causes divergence over time, and over time that means we miss out on bug fixes or improvements upstream since we've copied old code and have no forcing mechanism to notice the copied code has changed upstream.

@KanishkRanjan
Copy link
Copy Markdown
Contributor Author

KanishkRanjan commented Jan 21, 2026

Thank you for your review. 😊

I think in future users might request for a feature to control image icon resolution so for that case we might require our own solution.

what do you think @dsanders11 ?

nikwen
nikwen requested changes Jan 28, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@nikwen nikwen left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

You previously had a test for the icon size. Can we add that back?

@KanishkRanjan KanishkRanjan force-pushed the kanishk/desktopcapturer-appicon-fix/39-x-y branch from 5a70d7b to c78144f Compare February 9, 2026 19:57
@KanishkRanjan KanishkRanjan requested a review from nikwen February 9, 2026 20:11
nikwen
nikwen requested changes Feb 10, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@nikwen nikwen left a comment

Choose a reason for hiding this comment

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

Thanks for adding back tests!

Unfortunately, the Chromium patch changed beyond what we had discussed. The goal was to keep it merged into the patch that it was merged into and only change the comments in that patch file.

Could you please undo the patch changes to the previous revision you pushed and only make the comment changes that were requested?

Thanks!

@KanishkRanjan KanishkRanjan force-pushed the kanishk/desktopcapturer-appicon-fix/39-x-y branch from 769fa1e to f7a0324 Compare February 11, 2026 07:46
@KanishkRanjan KanishkRanjan marked this pull request as draft February 11, 2026 07:53
@KanishkRanjan KanishkRanjan force-pushed the kanishk/desktopcapturer-appicon-fix/39-x-y branch from f7a0324 to 2f261a1 Compare February 17, 2026 03:37
@KanishkRanjan KanishkRanjan marked this pull request as ready for review February 17, 2026 03:40
@jkleinsc
Copy link
Copy Markdown
Member

jkleinsc commented Feb 24, 2026

@KanishkRanjan can you rebase to fix the CI error on the lint job?

@KanishkRanjan
Copy link
Copy Markdown
Contributor Author

KanishkRanjan commented Feb 26, 2026

@jkleinsc Thank you for suggestion; I have ran the comman git rebase origin. I hope this should be sufficient.

nikwen
nikwen reviewed Feb 27, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@nikwen nikwen left a comment

Choose a reason for hiding this comment

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

I don't think the PR has been rebased.

@KanishkRanjan
Copy link
Copy Markdown
Contributor Author

KanishkRanjan commented Feb 27, 2026 •
edited
Loading

@nikwen I strongly believe that it is rebased but I could be wrong. Please suggestion be step or how to verify this ?

Thank you.

@jkleinsc
Copy link
Copy Markdown
Member

jkleinsc commented Feb 27, 2026 •
edited
Loading

@KanishkRanjan this does not look to be properly rebased. Try the following on your fork:

  1. Verify that origin points to electron/electron. Run git remote -v. You should see something like:
origin	git@github.com:electron/electron.git (fetch)
origin	git@github.com:electron/electron.git (push)

OR

origin	https://github.com/electron/electron.git (fetch)
origin	https://github.com/electron/electron.git (push)

If for some reason git remote -v shows electron/electron with another name use that. If it isn't listed at all, add it first:
git remote add upstream https://github.com/electron/electron.git

  1. git fetch origin
  2. git rebase origin/main

@KanishkRanjan KanishkRanjan force-pushed the kanishk/desktopcapturer-appicon-fix/39-x-y branch from 29c4f83 to 856c02f Compare February 27, 2026 18:17
@KanishkRanjan KanishkRanjan force-pushed the kanishk/desktopcapturer-appicon-fix/39-x-y branch from 856c02f to 71881cc Compare February 27, 2026 18:19
@KanishkRanjan
Copy link
Copy Markdown
Contributor Author

KanishkRanjan commented Feb 27, 2026

Thank you @jkleinsc, I have rebased it to main; my remote org was outdated, I have synced it.

@nikwen
Copy link
Copy Markdown
Member

nikwen commented Mar 4, 2026

Looking at the test output, it seems like Windows does indeed return 32x32px icons (as discussed in #49219 (comment)).

In that case, I don't see why we should return higher-resolution icons on macOS. The trade-off of floating a Chromium patch doesn't seem worth it.

However, what we might want to do then is backport https://crrev.com/c/7239386 to Electron 40 so that people like @fanhillary can get their icons back in a stable release.

@KanishkRanjan Would you be willing to do that?

@KanishkRanjan
Copy link
Copy Markdown
Contributor Author

KanishkRanjan commented Mar 7, 2026

on it.

@KanishkRanjan KanishkRanjan marked this pull request as draft March 8, 2026 07:52
KanishkRanjan added a commit to KanishkRanjan/electron that referenced this pull request Mar 8, 2026
@KanishkRanjan
fix: backport high-res mac icons from chromium (crrev.com/c/7239386)
8c6b3ea 
Backports the fix for high-resolution icons on macOS from Chromium.

Chromium CL: https://chromium-review.googlesource.com/c/chromium/src/+/7239386
Backport of electron#49219
@KanishkRanjan KanishkRanjan mentioned this pull request Mar 10, 2026
Merged
6 tasks
KanishkRanjan added a commit to KanishkRanjan/electron that referenced this pull request Mar 10, 2026
@KanishkRanjan
fix: backport high-res mac icons from chromium (crrev.com/c/7239386)
37bec07 
Backports the fix for high-resolution icons on macOS from Chromium.

Chromium CL: https://chromium-review.googlesource.com/c/chromium/src/+/7239386
Backport of electron#49219
@KanishkRanjan
Copy link
Copy Markdown
Contributor Author

KanishkRanjan commented Mar 13, 2026

Closing this PR as this is fixed in Electron v41.0.0 and above, as the patch was merged in Chromium 143. A backport was also made in the v40-x-y branch of Electron.

@KanishkRanjan
Copy link
Copy Markdown
Contributor Author

KanishkRanjan commented Mar 13, 2026

@fanhillary please take a look above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikwen nikwen nikwen requested changes

@dsanders11 dsanders11 Awaiting requested review from dsanders11

Assignees

No one assigned

Labels

semver/patch backwards-compatible bug fixes target/39-x-y PR should also be added to the "39-x-y" branch. target/40-x-y PR should also be added to the "40-x-y" branch. target/41-x-y PR should also be added to the "41-x-y" branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

desktopCapturer.getSources got error appIcon

5 participants

@KanishkRanjan @nikwen @fanhillary @jkleinsc @dsanders11