Skip to content

Update 1.1.0#4716

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

Update 1.1.0#4716
Lockframe wants to merge 7 commits into
ramensoftware:mainfrom
Lockframe:main

Conversation

@Lockframe

Copy link
Copy Markdown
Contributor

Changelog

  • Added 'Elastic' animation mode, which is a hybrid of stretch and bounce, inspired by Apple's Dynamic Island animations.
  • Added spring physics toggle for smoother animations, which is especially noticeable paired with bounce or elastic animation modes.
  • Added a speed multiplier setting.
  • Added a simple color-crossfade animation when using app icon color mode.
  • Fixed initialization bug where the pill renders at the far left of the taskbar.
  • Fixed an issue where the pill retains a closed app's position before animating to the currently focused app's.

@m417z

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


1. GetFrameworkElementFromNative now uses IsBadReadPtr and a brute-force QI scan — please drop IsBadReadPtr and reconsider the scan. (lines 412-431)

IsBadReadPtr is on the "never use" list — it's unreliable (a readable pointer isn't a valid COM object) and can actually cause crashes by tripping the guard page used for stack growth, and it races under concurrent access. See The Old New Thing. On top of that, the new approach walks slots 1-15 of the object, treats each stored pointer as an IUnknown*, and calls QueryInterface on the first one whose first few vtable bytes happen to be readable. That's semantically different from the previous copy_from_abi at a fixed sub-object offset: you're now dereferencing arbitrary member pointers and invoking a vtable slot on whatever they point at, and this runs on every UpdateVisualStates call (hover, click, app switch), so it's also a per-event cost regression over the old O(1) path.

If the reason for the rewrite was that the fixed offset (pThis + 3) worked for TaskListButton but not for SearchIconButton, prefer a correct per-class offset (or two dedicated helpers) rather than scanning 15 slots and probing memory each event.

2. g_attachedEvents was changed from a leaked heap pointer to a plain global with a non-trivial destructor. (line 296)

-std::vector<AttachedEvent>* g_attachedEvents = new std::vector<AttachedEvent>();
+std::vector<AttachedEvent> g_attachedEvents;

Wh_ModUninit/Wh_ModBeforeUninit do not run when the host process terminates (Explorer restart, sign-out, reboot) — the OS just detaches the DLL and the CRT runs global destructors during DLL_PROCESS_DETACH. AttachedEvent holds a winrt::weak_ref and a CoreDispatcher, so ~vector now releases WinRT/COM references at process-detach time, when the XAML apartment may already be gone. The previous leaked-pointer form deliberately avoided this. Recommend keeping it a pointer (new std::vector<...>()). (The same latent concern applies to the other WinRT-holding globals here — g_pillContexts, g_iconColorCache, g_attachedGroups — which were already plain globals before this PR; worth considering the leaked-pointer pattern for those too, but this one is a regression introduced by this diff.)

3. HookSymbols is called twice for SearchUx.UI.dll on the fallback path (lines 1479-1484). Resolving symbols for a module a second time invalidates the per-module symbol cache and re-resolves. It only happens when UpdateVisualStates fails to resolve (uncommon), and it's init-time, so the impact is small — but if you want a single call, put both symbols in one array (both optional) and resolve once. That does change the semantics to "hook both if both exist" though (see the functionality note below), so only do it if that's acceptable.

Optional improvements

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

  • Clamp SpeedMultiplier to a sane upper bound. You correctly reject <= 0 and NaN, but there's no ceiling — a large value (e.g. >60) drives the spring Period / animation durations to 0, which can make CreateSpringScalarAnimation().Period(0) throw (swallowed by the surrounding try/catch, so the pill just stops animating). A clamp like std::clamp(x, 0.1, 10.0) keeps every derived duration valid.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • Search-button tracking now hooks UpdateVisualStates or PlayStateChange, not both (lines 1459-1486). v1.0.0 hooked both (though both wrote into the same _Original, which was a latent bug you've now fixed by splitting them). The new fallback only reaches PlayStateChange if UpdateVisualStates fails to resolve. If, on a given build, a search-button active-state transition ever fires only through PlayStateChange while UpdateVisualStates also exists, the pill won't update for it. Worth confirming that one entry point covers all the transitions you care about.
  • The point.X == 0 && point.Y == 0 guard (line 693) treats a transform of exactly (0,0) as "not laid out yet" and skips positioning. That's a reasonable readiness heuristic, but a button legitimately at the grid origin would never get a pill. In practice the left edge is occupied by Start/Task View, so this is unlikely to bite — just flagging the assumption.
  • Icon-color cache eviction changed from LRU-10 to a full clear() (lines 623-626). Simpler, but on a taskbar with >100 distinct apps it periodically wipes and re-extracts every color instead of dropping the 10 least-recently-used. Edge case, so probably fine — just noting the behavior change.

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