Skip to content

fix(FxSystemTrayView): use LoadImage for tray icons to ensure proper sizing and alpha - #534

Open
akai07 wants to merge 3 commits into
fxsound2:develop/windowsfrom
akai07:fix/dark-theme-tray-icon
Open

fix(FxSystemTrayView): use LoadImage for tray icons to ensure proper sizing and alpha#534
akai07 wants to merge 3 commits into
fxsound2:develop/windowsfrom
akai07:fix/dark-theme-tray-icon

Conversation

@akai07

@akai07 akai07 commented May 27, 2026

Copy link
Copy Markdown

Summary

Fixes system tray icon rendering in Windows dark theme mode by replacing LoadIcon with LoadImage using explicit small-icon dimensions.

Problem

LoadIcon loads icons at the default system icon size (typically 32x32), which then gets scaled down to the system tray small-icon size (16x16 on most displays). This scaling can cause:

  • Loss of alpha channel information in dark theme mode
  • Visible artifacts around icon edges
  • Icons appearing as white blocks or with incorrect colors on dark taskbars

Fix

Replaced all LoadIcon(hInst, L"IDI_LOGO_*") calls with a new LoadTrayIcon helper that uses LoadImage with explicit dimensions from GetSystemMetrics(SM_CXSMICON) and GetSystemMetrics(SM_CYSMICON). This ensures:

  • Icons are loaded at the exact size needed for the notification area
  • No lossy scaling that degrades alpha transparency
  • Correct rendering on both light and dark Windows themes

Files Changed

  • fxsound/Source/GUI/FxSystemTrayView.cppLoadIconLoadImage in both setStatus() and addIcon()

Related Issues

  • Fixes dark theme tray icon rendering issues on Windows 10/11

akai07 added 3 commits May 28, 2026 02:50
… in OnDeviceStateChanged

- Filter capture (eRender) device events in OnDeviceStateChanged via IMMEndpoint.GetDataFlow
  to prevent unnecessary playback reinit when input devices change state
- Track reconnected device GUID when state transitions to ACTIVE
- Add reconnection detection in sndDevicesImplementDeviceRules to auto-select
  reconnected playback devices (fixes BT headset reconnect after UNPLUGGED)
- Clear reconnection state on reinit
… system resume

- Call SetThreadExecutionState(ES_CONTINUOUS | ES_SYSTEM_REQUIRED | ES_DISPLAY_REQUIRED)
  when audio processing starts to prevent system sleep during playback
- Revert to SetThreadExecutionState(ES_CONTINUOUS) when processing stops
- Handle PBT_APMRESUMEAUTOMATIC power event: check device changes and
  restore power state after system resume from sleep
- Clear execution state on controller destruction
…sizing and alpha

Replaced LoadIcon with LoadImage using explicit SM_CXSMICON/SM_CYSMICON
dimensions so tray icons are rendered at the correct size for the
notification area. LoadIcon uses default icon size which can cause
scaling artifacts and improper alpha blending on Windows 10/11,
especially in dark theme mode where transparency is critical.
@bvijay74
bvijay74 changed the base branch from main to develop/windows July 21, 2026 09:13
@bvijay74

Copy link
Copy Markdown
Member

@claude review this pull request

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @bvijay74's task in 4m 27s —— View job


Reviewing PR #534

  • Gather context (diff, related files, CLAUDE.md)
  • Review FxSystemTrayView.cpp changes for the dark-theme tray icon fix
  • Review the other changed files (sndDevices*, FxController.cpp) for scope/consistency
  • Post review feedback

Summary

The core idea (LoadIconLoadImage with explicit SM_CXSMICON/SM_CYSMICON) is the right, well-documented fix for blurry/washed-out tray icons on Windows, and the refactor into a LoadTrayIcon helper is clean. However, there's one real regression introduced by the change, plus a scope observation.

🔴 Icon handle leak introduced by switching to LoadImage (High)

fxsound/Source/GUI/FxSystemTrayView.cpp:69-74

static HICON LoadTrayIcon(HINSTANCE hInst, LPCWSTR iconName)
{
    return (HICON)LoadImage(hInst, iconName, IMAGE_ICON,
                            GetSystemMetrics(SM_CXSMICON), GetSystemMetrics(SM_CYSMICON),
                            LR_DEFAULTCOLOR);
}

LoadIcon returns a shared, system-cached handle that doesn't need to be freed. LoadImage (without LR_SHARED) returns a private handle that the caller owns and must release with DestroyIcon once Shell_NotifyIcon has consumed it. Neither setStatus() (line 76) nor addIcon() (line 176) ever calls DestroyIcon on nid.hIcon.

setStatus() is invoked frequently during normal use — e.g. FxController.cpp:1358 and :1371 call it on every audio processing on/off toggle — so this leaks one GDI/USER icon handle per call. Over a long-running tray session with repeated play/pause, this can approach the per-process USER object quota (10,000 by default), eventually causing icon updates (or other UI operations) to silently fail.

Fix: either cache the previously-created HICON (e.g. as a member of FxSystemTrayView) and DestroyIcon() it before/after assigning the new one, or load the 4 icon variants once (e.g. in the constructor) and reuse the handles instead of reloading on every setStatus()/addIcon() call.

Fix this →

🟡 PR bundles unrelated changes (Scope)

The PR title/description only describe the tray icon LoadImage fix, but the branch also contains two unrelated commits that make up ~105 of the 120 added lines:

  • a09f1feFxController.cpp: prevents sleep during playback (SetThreadExecutionState) and re-inits audio on system resume (WM_POWERBROADCAST/PBT_APMRESUMEAUTOMATIC)
  • 9488820sndDevices*: filters capture-device events out of OnDeviceStateChanged and adds Bluetooth-style device reconnection handling

These look like reasonable fixes on their own, but bundling them with the tray icon fix makes the PR harder to review/revert independently, and the description doesn't mention them at all. Consider either updating the PR description to cover all three fixes, or splitting them into separate PRs.

🟢 Minor notes (non-blocking)

  • FxController.cpp:1358SetThreadExecutionState(ES_CONTINUOUS | ES_SYSTEM_REQUIRED | ES_DISPLAY_REQUIRED) also forces the display to stay on while audio is processing. For a background audio app this may be more aggressive than needed (most audio players only set ES_SYSTEM_REQUIRED so the screen can still turn off during playback) — worth confirming this is intentional.
  • audiopassthru/src/sndDevices/sndDevicesDeviceCallbacks.cpp:203-224OnDeviceStateChanged now creates a new IMMDeviceEnumerator via CoCreateInstance on every callback just to check GetDataFlow. Not a correctness issue, but a bit wasteful given this callback can fire often; caching the enumerator on cast_handle would avoid the repeated COM allocation.

I verified the hasReconnectedDevice/reconnectedDeviceGuid fields are zero-initialized via sndDevicesReInit before first use (called from sndDevicesInit), and that the new reconnection-selection block in sndDevicesImplementDeviceRules.cpp:234-268 correctly sits after the "user manually selected an active device" check (line 188-193), so it won't override an explicit user selection — no bug there.
· Branch: fix/dark-theme-tray-icondevelop/windows

@bvijay74

Copy link
Copy Markdown
Member

@akai07 I got the PR reviewed.
Can you please address the review comments.

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.

2 participants