Use sleep mode listener to fix modern standby sleep mode issue #4024
Conversation
|
🥷 Code experts: taooceros Jack251970, taooceros have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
This comment has been minimized.
This comment has been minimized.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a Win32 suspend/resume notification listener in infrastructure, exposes Register/Unregister APIs in Win32Helper, and updates MainWindow to reinitialize and thread-safely manage sound effects on resume. Removes WM_POWERBROADCAST handling and ensures listener registration and cleanup during window lifecycle. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant OS as OS Power Manager
participant Win32 as Win32Helper
participant MW as MainWindow
participant UI as Dispatcher (UI Thread)
rect rgba(230,240,255,0.5)
note right of MW: App startup
MW->>Win32: RegisterSleepModeListener(onResume)
Win32-->>MW: Listener registered (handle)
end
OS-->>Win32: Suspend/Resume notification
alt Resume (Automatic/Suspend)
Win32->>MW: Invoke onResume callback
MW->>UI: Dispatcher.Invoke(InitSoundEffects)
UI->>UI: Acquire _soundLock
UI->>UI: Recreate/refresh audio players
UI-->>MW: Completed
else Suspend
note right of Win32: No action required
end
rect rgba(245,235,235,0.5)
note right of MW: App shutdown
MW->>Win32: UnregisterSleepModeListener()
Win32-->>MW: Cleanup completed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher/MainWindow.xaml.cs (1)
861-861: Fix the forbidden pattern flagged by the spell checker.The pipeline failure indicates that "work around" matches a forbidden pattern. Please change it to "workaround" (one word).
Apply this diff:
- // Initialize call twice to work around multi-display alignment issue- https://github.com/Flow-Launcher/Flow.Launcher/issues/2910 + // Initialize call twice to workaround multi-display alignment issue- https://github.com/Flow-Launcher/Flow.Launcher/issues/2910
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Infrastructure/NativeMethods.txt(1 hunks)Flow.Launcher.Infrastructure/Win32Helper.cs(2 hunks)Flow.Launcher/MainWindow.xaml.cs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Flow.Launcher/MainWindow.xaml.cs (2)
Flow.Launcher.Infrastructure/Win32Helper.cs (3)
Win32Helper(32-1019)RegisterSleepModeListener(935-962)UnregisterSleepModeListener(967-977)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
LogException(311-311)
🪛 GitHub Actions: Check Spelling
Flow.Launcher.Infrastructure/Win32Helper.cs
[warning] 21-21: Spell check: 'Dwm' is not a recognized word. (unrecognized-spelling)
Flow.Launcher/MainWindow.xaml.cs
[error] 861-861: forbidden-pattern: 'work around' matches a line_forbidden.patterns entry: \bwork[- ]arounds?\b.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (11)
Flow.Launcher/MainWindow.xaml.cs (7)
5-5: LGTM!The
System.Threadingimport is correctly added to support the newLockinstance used for sound effect synchronization.
65-67: LGTM!Sound effect fields are properly encapsulated as private with appropriate naming conventions, and the readonly lock ensures thread-safe initialization/playback.
98-98: LGTM!The sleep mode listener registration is correctly placed after sound initialization, ensuring resources are ready before the listener can trigger reinitialization.
681-715: LGTM!Sound initialization and playback are properly protected with locks. The code correctly:
- Disposes existing resources before creating new ones
- Conditionally initializes WMP or WPF sound players based on availability
- Resets MediaPlayer position and volume before playback
717-744: LGTM!The sleep mode listener registration is well-implemented with proper safeguards:
- Null check on
Application.Currentprevents crashes during shutdownDispatcher.CheckAccess()ensuresMediaPlayer(aDispatcherObject) is always reinitialized on the UI thread- Exception handling with logging provides visibility into registration failures
746-756: LGTM!Unregistration is properly implemented with exception handling and logging, preventing disposal failures from propagating.
1479-1482: LGTM!Sound resources and the sleep mode listener are correctly cleaned up during disposal, ensuring no resource leaks.
Flow.Launcher.Infrastructure/NativeMethods.txt (1)
88-94: LGTM!The native method declarations correctly define all the required types and functions for power notification registration:
DEVICE_NOTIFY_SUBSCRIBE_PARAMETERSfor the registration structurePBT_APMRESUMEAUTOMATICandPBT_APMRESUMESUSPENDconstants for resume events- Registration/unregistration functions and callback routine type
These align with the usage in
Win32Helper.cs.Flow.Launcher.Infrastructure/Win32Helper.cs (3)
22-22: LGTM!The
Windows.Win32.System.Powernamespace import is correctly added to support power notification APIs.
935-962: Consider thread safety for the registration guard.The re-registration check at line 937 (
if (_callback != null)) is not thread-safe. IfRegisterSleepModeListeneris called concurrently from multiple threads, both could pass the null check and attempt registration, leading to a race condition.However, if this method is only called from the UI thread during
MainWindowinitialization (as observed inMainWindow.xaml.cs), this may not be a practical concern.Please verify the threading model and consider adding a lock or using
Interlocked.CompareExchangeif concurrent calls are possible:private static readonly object _registrationLock = new(); public static unsafe void RegisterSleepModeListener(Action func) { lock (_registrationLock) { if (_callback != null) { // Only register if not already registered return; } // ... rest of registration logic } }
997-1016: LGTM!The
StructSafeHandle<T>class correctly implements theSafeHandlepattern for managing native memory:
- Allocates unmanaged memory via
Marshal.AllocHGlobal- Marshals the managed struct to native memory
- Ensures cleanup via
ReleaseHandlewithMarshal.FreeHGlobalThe sealed modifier prevents inheritance issues with
SafeHandle.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: s.b. workaround(s)If the flagged items are 🤯 false positivesIf items relate to a ...
|
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the existing WM_POWERBROADCAST message handling with a more robust sleep mode listener to fix issues with sound effects not playing after modern standby sleep mode on S0 devices.
- Implements a dedicated sleep mode listener using PowerRegisterSuspendResumeNotification API
- Adds thread-safe locking for sound effect initialization and playback
- Replaces WndProc power broadcast handling with callback-based approach
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| MainWindow.xaml.cs | Removes WM_POWERBROADCAST handling, adds sleep mode listener registration, and implements thread-safe sound effect methods |
| Win32Helper.cs | Implements sleep mode listener functionality using Windows Power APIs with proper resource management |
| NativeMethods.txt | Adds required Windows Power API definitions for the new sleep mode listener |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Flow.Launcher.Infrastructure/Win32Helper.cs (1)
979-995: Add synchronization in callback to prevent race with unregistration.The past review correctly identified that
_funccould be nulled during callback execution. However, beyond the null-conditional operator?., this callback also needs synchronization to ensure it doesn't race withUnregisterSleepModeListener.Capture
_funcunder lock to ensure thread-safe access:private static unsafe uint DeviceNotifyCallback(void* context, uint type, void* setting) { + Action localFunc; + lock (_sleepModeListenerLock) + { + localFunc = _func; + } + + if (localFunc == null) return 0; + switch (type) { case PInvoke.PBT_APMRESUMEAUTOMATIC: // Operation is resuming automatically from a low-power state.This message is sent every time the system resumes - _func?.Invoke(); + localFunc(); break; case PInvoke.PBT_APMRESUMESUSPEND: // Operation is resuming from a low-power state.This message is sent after PBT_APMRESUMEAUTOMATIC if the resume is triggered by user input, such as pressing a key - localFunc?.Invoke(); + localFunc(); break; } return 0; }
🧹 Nitpick comments (1)
Flow.Launcher.Infrastructure/Win32Helper.cs (1)
997-1016: Consider simplifying by removing redundant_ptrfield.The
_ptrfield duplicates thehandlefield inherited fromSafeHandle. SinceSetHandle(pRecipient)is called with the allocated pointer, you can usehandledirectly inReleaseHandle.private sealed class StructSafeHandle<T> : SafeHandle where T : struct { - private readonly nint _ptr = nint.Zero; public StructSafeHandle(T recipient) : base(nint.Zero, true) { var pRecipient = Marshal.AllocHGlobal(Marshal.SizeOf<T>()); Marshal.StructureToPtr(recipient, pRecipient, false); SetHandle(pRecipient); - _ptr = pRecipient; } public override bool IsInvalid => handle == nint.Zero; protected override bool ReleaseHandle() { - Marshal.FreeHGlobal(_ptr); + Marshal.FreeHGlobal(handle); return true; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Infrastructure/Win32Helper.cs(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Check Spelling
Flow.Launcher.Infrastructure/Win32Helper.cs
[warning] 1-1: Spell check warning: 'Dwm' is not a recognized word.
[warning] 1-1: Spell check warning: 'PInvoke' is not a recognized word.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher.Infrastructure/Win32Helper.cs (1)
22-22: LGTM!The using directive is necessary for the power notification APIs introduced in the Sleep Mode Listener region.
|
Have you tested using Sys plugin command's sleep and hibernate as well? |
I have tested these commands and |
CHANGES
Follow on with #3532.
Use sleep mode listener to listen power mode change event to fix standby sleep mode issue.
Inspired by: https://github.com/XKaguya/LenovoLegionToolkit and https://blog.csdn.net/mochounv/article/details/114668594.
Add lock for InitSoundEffects() & SoundPlay().
Resolve #4018.
Test
Tested by H5820121.
Test on S0 modern standby sleep device. It works well.