Skip to content

Taskbar Elastic Pill Update 2.0.0#4829

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

Taskbar Elastic Pill Update 2.0.0#4829
Lockframe wants to merge 5 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.

@m417z

m417z commented Jul 22, 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.


LayoutW is written in two different units (1x vs 4x), so the settings-change check misfires on every call. Since the blurriness fix, the pill element is 4x-sized (pill.Width(settingsToUse.PillWidth * 4.0)) and rendered at Scale * 0.25. UpdatePillPosition stores the 4x value:

double actW = p.ActualWidth();          // 4x, e.g. 64 for PillWidth=16
...
visual.Properties().InsertScalar(L"LayoutW", (float)pillW);

but EnsurePillAndPosition reads that same property back and compares/writes it in 1x settings units:

visual.Properties().TryGetScalar(L"LayoutW", lastW);           // 64
if (std::abs(lastW - (float)settingsToUse.PillWidth) > 0.001f  // 64 vs 16 -> always true
    ...) {
    visual.Properties().InsertScalar(L"LayoutW", (float)settingsToUse.PillWidth);
    ...
    visual.Properties().InsertScalar(L"LastTargetX", std::numeric_limits<float>::quiet_NaN());

Consequences: after the first pill movement, every EnsurePillAndPosition invocation (i.e. every UpdateVisualStates dispatch — activation changes, hover state changes, etc.) takes the "settings changed" branch. That resets LastTargetX to NaN, so the immediately following UpdatePillPosition call takes the snap path — StopAnimation(L"BaseScale") kills an in-flight stretch mid-animation, and movement that should animate can degrade to a snap depending on whether the LayoutUpdated path ran first. It also transiently publishes the 1x value into LayoutW while the persistent stretch expression (props.RightX - props.LeftX) / props.LayoutW may still be attached to BaseScale, which makes the pill momentarily evaluate at 4x scale. (In v1.2.0 this was consistent because the pill was 1x-sized, so ActualWidth == PillWidth; the 4x change broke the invariant.)

Fix: keep LayoutW exclusively in device (4x) units for the expression math, and track the settings snapshot under a separate key, e.g.:

float lastW = -1.0f, ...;
visual.Properties().TryGetScalar(L"SettingsW", lastW);
...
if (std::abs(lastW - (float)settingsToUse.PillWidth) > 0.001f || ...) {
    visual.Properties().InsertScalar(L"SettingsW", (float)settingsToUse.PillWidth);
    visual.Properties().InsertScalar(L"LayoutW", (float)settingsToUse.PillWidth * 4.0f);
    ...
}

HandlePointerUpdate dereferences dead weak refs and runs unguarded XAML calls. Every other XAML-touching path in the mod null-checks the weak refs and wraps the work in try/catch, but the new pointer handlers do neither:

UpdatePillPosition(c->pill.get(), c->grid.get(), b, localSettings);

If c->pill or c->grid has expired, UpdatePillPosition calls p.ActualWidth() / TransformToVisual on a null object — a null-vtable call that crashes explorer. And XAML APIs here can also throw (e.g. TransformToVisual on a disconnecting element — PointerCaptureLost can fire during tree teardown), which would propagate out of the event handler unhandled. Cheap fix, matching the rest of the code:

auto p = c->pill.get();
auto g = c->grid.get();
if (!p || !g) return;
try {
    UpdatePillPosition(p, g, b, localSettings);
    ...
    UpdatePillColor(b, p, c, localSettings, true, c->isHover, c->isPress, stableKey, isExtracting);
} catch (...) {}
Optional improvements

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

  • In-flight async completions aren't covered by the unload drain. Wh_ModBeforeUninit waits for the queued dispatcher lambdas, but not for RenderTargetBitmap::RenderAsync/GetPixelsAsync completions or Storyboard.Completed handlers that may still be pending; if one fires after the DLL is unmapped, explorer crashes. Relatedly, a PillContext destroyed on the uninit thread releases a non-null colorAnimBoard (a strong XAML Storyboard) off its UI thread, and g_easingCaches releases per-thread Compositor/easing objects from the uninit thread. All of these can only bite during the narrow unload window (icon extraction or a color animation in flight), hence optional — but counting the async chains into the existing pending counter would close it.

  • g_taskbarViewModule and g_searchUxModule are write-only. They're assigned in Wh_ModInit and LoadLibraryExW_Hook but never read — remove them.

  • Pill.PillRadius isn't reset before parsing. Every other setting is re-defaulted at the top of its block in LoadSettings, but PillRadius is only assigned inside if (radiusStr.get()[0]), so clearing the setting keeps the previous value instead of reverting to 1.5.

Functionality notes

Non-critical observations about the feature behavior itself.

  • Turning HideInactiveDots off restores dots lazily. Wh_ModSettingsChanged only refreshes the pill; each button's native RunningIndicator opacity is restored on that button's next UpdateVisualStates, so already-hidden dots stay hidden until their state next changes.

  • The pointer hover/pressed scale animation uses a fixed 150 ms and ignores SpeedMultiplier, unlike all other animations. If that's intentional, fine — just noting the inconsistency.

  • ElasticIntensity doesn't affect the "Bounce" style, even though Bounce is also a spring animation (fixed DampingRatio(0.6f)). The description says it applies to "spring physics modes", so users picking Bounce may expect it to work there too.

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