-
Notifications
You must be signed in to change notification settings - Fork 9.3k
[Unpackaged] Fix taskbar glomming due to AUMID #20064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d18b3f5
ffaa89e
73af681
dad44d5
14269ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,8 @@ | |||||||||||
| #include <wil/token_helpers.h> | ||||||||||||
| #include <winrt/TerminalApp.h> | ||||||||||||
| #include <sddl.h> | ||||||||||||
| #include <propkey.h> | ||||||||||||
| #include <propvarutil.h> | ||||||||||||
|
|
||||||||||||
| #include "AppHost.h" | ||||||||||||
| #include "resource.h" | ||||||||||||
|
|
@@ -313,6 +315,107 @@ AppHost* WindowEmperor::_mostRecentWindow() const noexcept | |||||||||||
| return mostRecent; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // GH#20053: The shell resolves taskbar grouping identity as: per-window AUMID > | ||||||||||||
| // per-process AUMID > auto-derived from exe path. Before we started setting a | ||||||||||||
| // process AUMID, both the pinned .lnk and the process used auto-derived | ||||||||||||
| // identity, so they matched. Now that we set an explicit AUMID, a pinned .lnk | ||||||||||||
| // that predates the AUMID change has no AUMID and still uses auto-derived | ||||||||||||
| // identity, causing a mismatch and a duplicate taskbar button. | ||||||||||||
| // | ||||||||||||
| // To fix this, we check if a pinned taskbar shortcut (.lnk) points to our exe. | ||||||||||||
| // If it already carries our AUMID (or no pin exists), we set the process AUMID | ||||||||||||
| // normally. If a pin exists WITHOUT our AUMID, we skip setting the process | ||||||||||||
| // AUMID for THIS launch (both sides use auto-derived identity, so they match) | ||||||||||||
| // and defer stamping the shortcut to process exit. On the next launch, the pin | ||||||||||||
| // has our AUMID, so we set the process AUMID to match, and both agree. | ||||||||||||
|
Comment on lines
+328
to
+330
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not update the LNK immediately?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See terminal/src/cascadia/WindowsTerminal/WindowEmperor.cpp Lines 405 to 409 in 73af681
If we stamp now, we get two choices:
Deferring means that we always have a match:
These were battle scars I earned while testing/implementing this change haha.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this explanation more than the one in the code to be honest. 🙈 May be worth adjusting the comment IMO. |
||||||||||||
| // | ||||||||||||
| // NOTE: On the first launch after pinning, the process AUMID is not set. If | ||||||||||||
| // toast notifications are needed in the future, use | ||||||||||||
| // ToastNotificationManager::CreateToastNotifier(aumid) with the AUMID string | ||||||||||||
| // directly. That API does not depend on SetCurrentProcessExplicitAppUserModelID. | ||||||||||||
| // A Start Menu shortcut with the AUMID (separate from the taskbar pin) is also | ||||||||||||
| // required for toast routing; see | ||||||||||||
| // https://learn.microsoft.com/windows/apps/develop/notifications/app-notifications/send-local-toast-other-apps | ||||||||||||
| void WindowEmperor::_setupAumid(const std::wstring& aumid) | ||||||||||||
| { | ||||||||||||
| const auto ourExePath = wil::GetModuleFileNameW<std::wstring>(nullptr); | ||||||||||||
|
|
||||||||||||
| bool needsDeferredStamping = false; | ||||||||||||
| std::wstring pinnedLnkPath; | ||||||||||||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||||||||||||
|
|
||||||||||||
| const auto taskbarGlob = wil::ExpandEnvironmentStringsW<std::wstring>( | ||||||||||||
| LR"(%APPDATA%\Microsoft\Internet Explorer\Quick Launch\User Pinned\TaskBar\*.lnk)"); | ||||||||||||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||||||||||||
|
|
||||||||||||
| WIN32_FIND_DATAW findData{}; | ||||||||||||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||||||||||||
| const wil::unique_hfind findHandle{ FindFirstFileExW(taskbarGlob.c_str(), FindExInfoBasic, &findData, FindExSearchNameMatch, nullptr, FIND_FIRST_EX_LARGE_FETCH) }; | ||||||||||||
| if (findHandle) | ||||||||||||
| { | ||||||||||||
| const auto lastSlash = taskbarGlob.rfind(L'\\'); | ||||||||||||
| const auto taskbarDir = taskbarGlob.substr(0, lastSlash + 1); | ||||||||||||
|
|
||||||||||||
| do | ||||||||||||
| { | ||||||||||||
| const auto lnkPath = taskbarDir + findData.cFileName; | ||||||||||||
|
|
||||||||||||
| wil::com_ptr<IShellLinkW> shellLink; | ||||||||||||
| if (FAILED(CoCreateInstance(CLSID_ShellLink, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&shellLink)))) | ||||||||||||
| { | ||||||||||||
| continue; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const auto persistFile = shellLink.try_query<IPersistFile>(); | ||||||||||||
| if (!persistFile || FAILED(persistFile->Load(lnkPath.c_str(), STGM_READ))) | ||||||||||||
| { | ||||||||||||
| continue; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| wchar_t targetPath[MAX_PATH]{}; | ||||||||||||
| if (FAILED(shellLink->GetPath(targetPath, MAX_PATH, nullptr, SLGP_RAWPATH))) | ||||||||||||
| { | ||||||||||||
| continue; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (til::compare_ordinal_insensitive(targetPath, ourExePath) != 0) | ||||||||||||
| { | ||||||||||||
| continue; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // Found a pin pointing to us. Assume it needs stamping unless | ||||||||||||
| // we confirm it already has our AUMID. | ||||||||||||
| pinnedLnkPath = lnkPath; | ||||||||||||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||||||||||||
| needsDeferredStamping = true; | ||||||||||||
|
|
||||||||||||
| if (const auto propertyStore = shellLink.try_query<IPropertyStore>()) | ||||||||||||
| { | ||||||||||||
| wil::unique_prop_variant pv; | ||||||||||||
| if (SUCCEEDED(propertyStore->GetValue(PKEY_AppUserModel_ID, &pv)) && | ||||||||||||
| pv.vt == VT_LPWSTR && pv.pwszVal && | ||||||||||||
| aumid == pv.pwszVal) | ||||||||||||
| { | ||||||||||||
| needsDeferredStamping = false; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| break; | ||||||||||||
| } while (FindNextFileW(findHandle.get(), &findData)); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (needsDeferredStamping) | ||||||||||||
| { | ||||||||||||
| // The pin exists but doesn't have our AUMID yet. Don't set the process | ||||||||||||
| // AUMID or stamp the shortcut now. Writing the shortcut causes the | ||||||||||||
| // shell to re-read it immediately, changing the pin's cached identity | ||||||||||||
| // mid-launch and creating a mismatch in the opposite direction. Instead, | ||||||||||||
| // stamp it at shutdown when the taskbar association no longer matters. | ||||||||||||
| _pendingAumidLnkPath = std::move(pinnedLnkPath); | ||||||||||||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||||||||||||
| _pendingAumid = aumid; | ||||||||||||
| } | ||||||||||||
| else | ||||||||||||
| { | ||||||||||||
| LOG_IF_FAILED(SetCurrentProcessExplicitAppUserModelID(aumid.c_str())); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| void WindowEmperor::HandleCommandlineArgs(int nCmdShow) | ||||||||||||
| { | ||||||||||||
| // When running without package identity, set an explicit AppUserModelID so | ||||||||||||
|
|
@@ -373,7 +476,7 @@ void WindowEmperor::HandleCommandlineArgs(int nCmdShow) | |||||||||||
| #else | ||||||||||||
| fmt::format_to(std::back_inserter(unpackagedAumid), FMT_COMPILE(L".{:08x}"), hash); | ||||||||||||
| #endif | ||||||||||||
| LOG_IF_FAILED(SetCurrentProcessExplicitAppUserModelID(unpackagedAumid.c_str())); | ||||||||||||
| _setupAumid(unpackagedAumid); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
|
|
@@ -553,6 +656,29 @@ void WindowEmperor::HandleCommandlineArgs(int nCmdShow) | |||||||||||
| Shell_NotifyIconW(NIM_DELETE, &_notificationIcon); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // GH#20053: Deferred shortcut stamping. See _setupAumid() for context. | ||||||||||||
| if (!_pendingAumidLnkPath.empty()) | ||||||||||||
| { | ||||||||||||
| wil::com_ptr<IShellLinkW> shellLink; | ||||||||||||
| if (SUCCEEDED(CoCreateInstance(CLSID_ShellLink, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&shellLink)))) | ||||||||||||
| { | ||||||||||||
| if (const auto persistFile = shellLink.try_query<IPersistFile>(); | ||||||||||||
| persistFile && SUCCEEDED(persistFile->Load(_pendingAumidLnkPath.c_str(), STGM_READWRITE))) | ||||||||||||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||||||||||||
| { | ||||||||||||
| if (const auto propertyStore = shellLink.try_query<IPropertyStore>()) | ||||||||||||
| { | ||||||||||||
| wil::unique_prop_variant pv; | ||||||||||||
| if (SUCCEEDED(InitPropVariantFromString(_pendingAumid.c_str(), &pv)) && | ||||||||||||
| SUCCEEDED(propertyStore->SetValue(PKEY_AppUserModel_ID, pv)) && | ||||||||||||
| SUCCEEDED(propertyStore->Commit())) | ||||||||||||
| { | ||||||||||||
| persistFile->Save(_pendingAumidLnkPath.c_str(), TRUE); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // There's a mysterious crash in XAML on Windows 10 if you just let _app get destroyed (GH#15410). | ||||||||||||
| // We also need to ensure that all UI threads exit before WindowEmperor leaves the scope on the main thread (MSFT:46744208). | ||||||||||||
| // Both problems can be solved and the shutdown accelerated by using TerminateProcess. | ||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So LNK files essentially... cache? the AUMID that an app sets? Is that right?
So if my app ever changes the ID all LNKs are invalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! The LNK can either have an AUMID explicitly set or it's derived by the OS. I've definitely had a few apps update and not glom to the same taskbar entry and I'm guessing it's because they set a new AUMID.