Skip to content

Taskbar Elastic Pill Update 2.0.0#4829

Open
Lockframe wants to merge 2 commits into
ramensoftware:mainfrom
Lockframe:main
Open

Taskbar Elastic Pill Update 2.0.0#4829
Lockframe wants to merge 2 commits into
ramensoftware:mainfrom
Lockframe:main

Conversation

@Lockframe

Copy link
Copy Markdown
Contributor

Changelog

  • Added elastic intensity setting
  • Added squish multiplier setting
  • Added fade duration multiplier setting
  • Added hover/pressed interactions
  • Added dedicated opacity setting

@m417z

m417z commented Jul 20, 2026

Copy link
Copy Markdown
Member

Submission review

Note: This review was done by Claude, and then refined manually. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


Reference leak in GetFrameworkElementFromNative (double AddRef). The new manual AddRef leaks one reference on every call:

void* iUnknownPtr = (void**)pThis + 3;
if (iUnknownPtr) {
    ((::IUnknown*)iUnknownPtr)->AddRef();   // <-- added in this PR
}
winrt::Windows::Foundation::IUnknown iUnknown;
winrt::copy_from_abi(iUnknown, iUnknownPtr);

winrt::copy_from_abi already takes a copy reference (it AddRefs the interface itself), so the extra manual AddRef is never balanced by a Release — every call leaks one ref on the underlying element. This function runs on every TaskListButton::UpdateVisualStates and SearchIconButton::UpdateVisualStates/PlayStateChange, i.e. constantly (activation, hover, etc.), so the leaked references pin every taskbar button's XAML element and they accumulate for the life of the process — a steadily growing memory leak in explorer.exe. Also note if (iUnknownPtr) is always true here, so it leaks unconditionally.

Fix: drop the added block and keep just the copy_from_abi, which is the established pattern in the other taskbar mods that use this trick — e.g. taskbar-numberer.wh.cpp#L715-L717 and taskbar-on-top.wh.cpp#L1180-L1182. (GetFrameworkElementFromInterface, which uses QueryInterface + put_abi, is already correct — no change needed there.)

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • Settings::PointerInteractions is never loaded. The struct field defaults to true and is used to gate AttachPointerEvents, but LoadSettings never assigns it, so the if (!localSettings.PointerInteractions) return; guards are dead code (always enabled). Since the hover/pressed scales default to 1.0 (no-op), this is harmless, but either wire the field to a real setting or drop the field and the guards.

  • Inconsistent cleanup of the heap-allocated globals. Wh_ModUninit deletes g_pillContexts, g_easingCaches, g_iconColorCache, g_attachedGroups, and g_attachedEvents, but not the two new globals g_iconColorExtracting and g_attachedPointerButtons, so those leak on every normal unload (disable/reload). Add the two matching deletes (or, since this only matters on a controlled unload, consider whether the raw-new global pattern is needed at all — a plain global with [[clang::no_destroy]] where required would be simpler to keep consistent).

  • g_attachedPointerButtons never prunes dead entries. Unlike g_pillContexts (which compacts expired weak refs when it grows past 10), this vector only grows and is dedup-scanned linearly on each activation. In practice the taskbar button count is small so it's negligible, but a periodic prune would keep it bounded.

Functionality notes

Non-critical observations about the feature behavior itself.

  • CustomColor comma semantics changed. In 1.x, "light, dark" meant a light-mode color and a dark-mode color; with the new ParseGradientColorPair, a comma now means a multi-stop gradient and | separates light/dark. That's a behavior change for anyone who used the comma form in 1.x (their old "dark" color becomes a second gradient stop). The description is updated and this is a major version bump, so it's intentional — just flagging it as a heads-up for existing users.

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