-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Resolve issue with buttons on notifications not working correctly #13921
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
Conversation
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.
Pull request overview
This PR fixes a regression in notification button functionality that occurred after introducing a command line parsing helper class. The core issue involved incorrect argument formatting when spawning wslhost.exe from notification actions.
Key Changes:
- Fixed argument passing to
wslhost.exeby removing double--prefix and properly escaping the executable path - Corrected COM embedding flag from
--Embeddingto-Embeddingto match COM conventions - Refactored telemetry worker to check notification registry settings lazily rather than at initialization
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/windows/wslhost/main.cpp | Added proper path quoting when constructing command line for notification activation |
| src/windows/inc/wslhost.h | Corrected COM embedding option from --Embedding to -Embedding |
| src/windows/common/notifications.cpp | Removed -- prefix from notification action arguments to prevent double-dash issue |
| src/windows/service/exe/LxssUserSession.h | Removed drvFsNotifications parameter from TelemetryWorker signature |
| src/windows/service/exe/LxssUserSession.cpp | Moved registry check for DrvFs notifications into TelemetryWorker loop |
* cleanup: minor tweaks to path translation failed messages (#13892) Co-authored-by: Ben Hillis <[email protected]> * Localization change from build: 135812430 (#13905) * Localization change from build: 135812430 * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> * remove unused string --------- Co-authored-by: WSL localization <[email protected]> Co-authored-by: Ben Hillis <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Ben Hillis <[email protected]> * Resolve issue with config file writing sections outside of their expected header. (#13898) * Resolve issue with config file writing sections outside of their expected header. * add more writewslconfig variations * formatting --------- Co-authored-by: Ben Hillis <[email protected]> * Resolve issue with buttons on notifications not working correctly (#13921) * Resolve issue with buttons on notifications not working correctly * pr feedback --------- Co-authored-by: Ben Hillis <[email protected]> * Fix minor typo in UserConfig.cmake.sample (.exe instead of .dll) (#13926) Co-authored-by: Ben Hillis <[email protected]> * test: extend networking test coverage (#13914) * test: extend coverage of virtioproxy networking mode * test: add dns test variations to all networking classes * remove bridged dns variations * pr feedback --------- Co-authored-by: Ben Hillis <[email protected]> * cleanup: remove duplicate AppId registration for wsldevicehost.dll (#13928) Co-authored-by: Ben Hillis <[email protected]> * Remove the explicit desktop field when creating a Windows process (#13930) * [GH 13837] Remove trailing slash from $XDG_RUNTIME_DIR (#13929) * [GH 13837] Remove trailing slash from $XDG_RUNTIME_DIR * pr feedback --------- Co-authored-by: Ben Hillis <[email protected]> * test: add more path translation variations (#13927) * test: add more path translation variations * Update test/windows/SimpleTests.cpp Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Ben Hillis <[email protected]> * virtiofs: enable additional test cases and skip one racey test (#13933) Co-authored-by: Ben Hillis <[email protected]> * cleanup: add scsi disks during VM creation (#13939) * cleanup: add scsi disks during VM creation * pr feedback --------- Co-authored-by: Ben Hillis <[email protected]> * cleanup: hcs logging improvements (#13942) Co-authored-by: Ben Hillis <[email protected]> * cleanup: switch drvfs.cpp to use common MountUtil helper (#13890) * cleanup: switch drvfs.cpp to use common MountUtil helper * adjust mount.drvfs error logging * adjust mount.drvfs error logging --------- Co-authored-by: Ben Hillis <[email protected]> --------- Co-authored-by: Ben Hillis <[email protected]> Co-authored-by: Blue <[email protected]> Co-authored-by: WSL localization <[email protected]> Co-authored-by: Copilot <[email protected]>
This pull request resolves an issue where the buttons on notifications were not working properly. This regressed with the introduction of the command line parsing helper class because this is difficult to test programmatically. There are three changes:
--character insertion)C:\Program Files\WSL\wslhost.exepath to resolve issue with space in the path name.-Embeddingto--EmbeddingResolves #13908