Fix: Fix a issue that change always on top settings won't take effect immediately#45994
Fix: Fix a issue that change always on top settings won't take effect immediately#45994
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a concurrency bug in Always On Top where settings could be read while they’re being reloaded, causing user setting changes (e.g., enable frame, sound, etc.) to not take effect immediately.
Changes:
- Switches AlwaysOnTop settings storage to an atomic
shared_ptrsnapshot (std::atomic<std::shared_ptr<const Settings>>) to avoid concurrent mutation/read races. - Updates call sites to read settings via
AlwaysOnTopSettings::settings()(pointer-style access) and reduces repeated direct access patterns in some places. - Batches observer notifications so the new snapshot is stored before notifying observers of changed settings.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/modules/alwaysontop/AlwaysOnTop/WindowBorder.cpp | Updated settings reads to use the new shared snapshot; minor local caching added. |
| src/modules/alwaysontop/AlwaysOnTop/Settings.h | Changes settings() API to return a shared_ptr<const Settings> and stores settings atomically. |
| src/modules/alwaysontop/AlwaysOnTop/Settings.cpp | Implements copy-on-write settings reload and stores a new snapshot before notifying observers. |
| src/modules/alwaysontop/AlwaysOnTop/AlwaysOnTop.cpp | Updates all settings reads to the new API (shared snapshot / pointer access). |
Comments suppressed due to low confidence (1)
src/modules/alwaysontop/AlwaysOnTop/WindowBorder.cpp:203
- In
UpdateBorderProperties, the localconst auto settings = AlwaysOnTopSettings::settings();is shadowed bywinrt::Windows::UI::ViewManagement::UISettings settings;inside theifblock. This compiles but is confusing and makes it easy to accidentally refer to the wrongsettingslater. Rename the inner WinRT variable (e.g.,uiSettings) to avoid shadowing.
const auto settings = AlwaysOnTopSettings::settings();
COLORREF color;
if (settings->frameAccentColor)
{
winrt::Windows::UI::ViewManagement::UISettings settings;
auto accentValue = settings.GetColorValue(winrt::Windows::UI::ViewManagement::UIColorType::Accent);
color = RGB(accentValue.R, accentValue.G, accentValue.B);
| @@ -276,7 +279,7 @@ void AlwaysOnTop::ProcessCommand(HWND window) | |||
| } | |||
| } | |||
|
|
|||
| if (AlwaysOnTopSettings::settings().enableSound) | |||
| if (AlwaysOnTopSettings::settings()->enableSound) | |||
| { | |||
There was a problem hiding this comment.
ProcessCommand calls AlwaysOnTopSettings::settings() multiple times (blockInGameMode, then later enableSound). Since settings() now returns a shared_ptr, each call does an atomic load + refcount inc/dec; it also means the function can observe different snapshots if a reload happens mid-call. Consider loading once at the start of the function and using that local for all reads within ProcessCommand.
|
@vanzue should this be a 0.98 item? |
|
It's not a regression, while it's a bug found during testing, I think we should include it, I will test this more thoroughly today, make sure everything well |
|
A fully strong-consistency fix would require a different architecture, such as synchronous IPC acknowledgements or blocking update propagation, so the current semantic is a trade off between exact next operation effective and the simplicity, verified locally it works as expected, not observe the gap any more. |
Summary of the Pull Request
Fix #45993
Always on top have a window proc thread that will reload the settings once file watcher trigger a reload of settings.
And always on top has a worker thread to read the settings at the same time.
So it may happen worker thread will read the stale setting, as a result, user change settings, and try to invoke always on top, as if nothing has changed.
As the issue's recording shows.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
The setting take effect once it's changed:
Alwaysontop-fix.mp4