Graceful shutdown #3367
Graceful shutdown #3367
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Jack251970
added
enhancement
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@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 ...
|
|
🥷 Code experts: onesounds onesounds has 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:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThe changes update the application’s initialization and shutdown processes across several components. In the main application file, dependency injection, error handling, and disposal patterns are reorganized with added logging. The SingleInstance helper is refactored for asynchronous operations and constant-based mutex naming. Additionally, MainWindow’s XAML and code-behind now include an added closed event handler and enhanced disposal processes. The view model is updated with the IDisposable interface for improved resource cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App.xaml.cs
participant DI as Dependency Injection Container
participant Single as SingleInstance<TApplication>
participant MW as MainWindow
participant Log as Logging/ExitEvents
App->>DI: Retrieve MainViewModel (_mainVM)
App->>Single: InitializeAsFirstInstance()
App->>MW: Create and assign MainWindow instance (_mainWindow)
Note over App,MW: Enhanced error handling and logging
MW->>Log: Register exit events (ProcessExit, AppExit, SessionEnding)
sequenceDiagram
participant MW as MainWindow (UI)
participant Dispose as Disposal Logic
participant Plugin as Plugins
MW->>MW: OnClosing Event Triggered
alt Not ready to close
MW->>Plugin: Dispose plugins
MW->>MW: Set _canClose flag and cancel closure
MW->>MW: Call Close() again
else Ready to close
MW->>Dispose: Invoke Dispose(bool disposing)
Dispose->>MW: Clean up _hwndSource, _notifyIcon, etc.
MW->>MW: OnClosed Event Triggered (unhook resources)
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Flow.Launcher/Helper/SingleInstance.cs (3)
59-59: Consider potential collisions in multi-user scenarios.Appending
Environment.UserNameto the mutex identifier may lead to unexpected behavior if multiple sessions run under the same username on the same machine. Typically unlikely, but worth noting.
67-67: Fire-and-forget async call might hide errors.Consider logging exceptions or awaiting the call if you need error handling for
CreateRemoteServiceAsync.
72-72: Similarly, consider exception handling forSignalFirstInstanceAsync.Fire-and-forget can mask pipe connection failures.
Flow.Launcher/MainWindow.xaml.cs (1)
238-248: Graceful shutdown logic in OnClosing is well-designed.Awaiting plugin disposal before completing the close sequence prevents resource leaks. Consider adding try-catch to handle disposal exceptions gracefully.
Flow.Launcher/App.xaml.cs (1)
105-113: Local function for fail-fast error handling.
Wrapping the message box andEnvironment.FailFastin a local function is a neat approach. Before crashing, consider whether logging or partial cleanup is needed to prevent resource leaks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher/App.xaml.cs(6 hunks)Flow.Launcher/Helper/SingleInstance.cs(7 hunks)Flow.Launcher/MainWindow.xaml(1 hunks)Flow.Launcher/MainWindow.xaml.cs(11 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(2 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
Flow.Launcher/App.xaml.cs (3)
Flow.Launcher/MainWindow.xaml.cs (4) (4)
MainWindow(30-1043)MainWindow(71-83)Dispose(1021-1033)Dispose(1035-1040)Flow.Launcher/ViewModel/MainViewModel.cs (4) (4)
MainViewModel(30-1572)MainViewModel(56-173)Dispose(1550-1562)Dispose(1564-1569)Flow.Launcher/Helper/SingleInstance.cs (2) (2)
SingleInstance(27-142)InitializeAsFirstInstance(56-75)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
Flow.Launcher/MainWindow.xaml.cs (2) (2)
Dispose(1021-1033)Dispose(1035-1040)Flow.Launcher/App.xaml.cs (2) (2)
Dispose(283-315)Dispose(317-322)
Flow.Launcher/MainWindow.xaml.cs (2)
Flow.Launcher/ViewModel/MainViewModel.cs (4) (4)
MainViewModel(30-1572)MainViewModel(56-173)Dispose(1550-1562)Dispose(1564-1569)Flow.Launcher/App.xaml.cs (4) (4)
App(28-334)App(50-114)Dispose(283-315)Dispose(317-322)
Flow.Launcher/Helper/SingleInstance.cs (1)
Flow.Launcher/App.xaml.cs (1) (1)
OnSecondAppStarted(328-331)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (49)
Flow.Launcher/Helper/SingleInstance.cs (13)
11-14: Interface definition looks good.No concerns with this interface addition.
27-27: Generically typed static class usage is appropriate.This approach promotes type safety while enabling reuse for different application types.
40-40: Constant naming is consistent.Defining a dedicated constant for the mutex name enhances readability.
45-45: Consider concurrency implications with the static property.While single-instance logic should be fine, keep in mind that a static property can be accessed anywhere. Ensure any future concurrency usage is well-managed.
56-57: Method signature is clear.The logic for determining if the current process is the first instance is well-structured.
61-61: Channel name creation is straightforward.String concat with delimiter is clear and readable.
64-64: Check for potential exceptions in mutex creation.While rare,
WaitHandleCannotBeOpenedExceptionmay occur if the mutex name is invalid or access is denied. Consider catching or verifying such cases if needed.
95-95: Creating the pipe server in a using statement is good practice.This ensures the
NamedPipeServerStreamis properly disposed after use.
98-100: Infinite listen loop is expected for a single-instance server.No immediate issues; remember to handle cancellation if you ever need to stop the server gracefully.
102-105: Dispatcher invocation for UI thread safety is well-handled.This code properly avoids cross-thread access issues when activating the main instance.
117-117: Method for signaling the first instance is neatly encapsulated.This separation of concerns improves clarity.
120-120: Using block for the client pipe ensures disposal.Resource management appears solid here.
121-123: Connecting with a zero time-out.Be aware that this might fail instantly if the server isn't ready, although that scenario is unlikely in normal usage.
Flow.Launcher/MainWindow.xaml (1)
20-20: Closed event handler usage is clear.Attaching
OnClosedto theClosedevent is a good way to finalize resource cleanup.Flow.Launcher/ViewModel/MainViewModel.cs (2)
30-30: IDisposable implementation on MainViewModel is valuable.This facilitates a proper lifecycle and resource cleanup for the view model.
1545-1571: Dispose pattern is correctly applied.The boolean guard
_disposedand callingGC.SuppressFinalize(this)inDispose()demonstrate a proper implementation, ensuring threads, tasks, and other resources are cleaned up.Flow.Launcher/MainWindow.xaml.cs (20)
30-30: Implementing IDisposable on MainWindow is an excellent choice.This allows for consistent and explicit resource deallocation in UI components.
42-42: Adding a dedicated context menu field.Helps keep code organized and fosters reusability.
45-48: Boolean fields for close state and arrow key presses.These flags improve readability and simplify logic flow.
55-55: Maintaining an HwndSource reference is valid for custom WndProc handling.
62-62: Use of_isClockPanelAnimatingis clear.Good naming for indicating animation state.
64-64: The_disposedfield is key to preventing double-disposal.This adheres to best practices for the Dispose pattern.
223-223: Listening toTextChangedevent on QueryTextBox.Updating the clock panel’s visibility conditionally is a neat approach.
226-226: Visibility changes on the context menu now trigger UI updates.This helps keep the clock panel in sync with user navigation.
232-232: History panel visibility also triggers the same update logic.Ensures a consistent UI experience.
251-263:OnClosedfinalizes hook removal and resets_hwndSource.This robust approach avoids potential WndProc calls on a destroyed window. Clean usage of try-catch.
307-307: KeyDown sets_isArrowKeyPressedfor the Down arrow.Clear approach to coordinate keyboard vs. mouse interactions.
312-312: Likewise, sets_isArrowKeyPressedfor the Up arrow.This keeps the same pattern for consistent navigation logic.
370-370: KeyUp resets_isArrowKeyPressed.Allows mouse interactions again, preventing conflicts.
375-375: Ignores mouse moves when arrow keys are pressed.Prevents unexpected selection changes during keyboard navigation.
545-549: Populating context menu items programmatically.Open, GameMode, PositionReset, Settings, and Exit are clearly defined. Good approach for a tray menu.
560-560: Right-click sets_contextMenu.IsOpento true.Standard approach for system tray context menus.
562-562: Bringing the context menu to foreground.Using
SetForegroundWindowis a reliable cross-API method to ensure proper focus.
567-567: Focusing the context menu after displaying it.Guarantees keyboard accessibility right away.
575-575: Local variable usage for easier updates.Minor but tidy approach to referencing
_contextMenuasmenu.
1021-1042: Dispose pattern is implemented correctly.Disposing
_hwndSourceand_notifyIconinsideDispose(bool disposing)aligns with best practices. The_disposedcheck ensures idempotent disposal.Flow.Launcher/App.xaml.cs (13)
30-34: Introduce public propertyAPI.
Defining the staticAPIproperty and initializing it via DI helps centralize access to the public API. This is a clear, concise design.
36-45: Confirm intention of static_disposingLock.
Using a static lock for disposing prevents multiple concurrent calls triggered by various exit events. Given the single-instance context, this is acceptable. If there were ever a possibility of multiple App objects within the same process, reconsider whether an instance-level lock would be more appropriate.
48-49: No functional changes.
These lines primarily introduce region markers for clarity and have no functional impact.
92-97: RetrieveMainViewModelfrom DI.
Instantiating_mainVMfrom the DI container is consistent with the new structure and ensures it can also be properly disposed later.
120-129: Single-instance logic update.
Removing the old parameter and callingInitializeAsFirstInstance()directly aligns with the revisedSingleInstancedesign. This cleanly ensures only one running instance.
165-169: Storing_mainWindowreference for disposal.
Assigning_mainWindowat startup facilitates a consistent disposal pattern later in the code, promoting maintainability.
237-237: No functional changes.
This line solely closes a region and has no functional effect.
239-239: No functional changes.
Another region marker line with no runtime impact.
242-259: Unified disposal calls for exit events.
LinkingProcessExit,ApplicationExit, andSessionEndingto the same disposal method ensures robust and consistent cleanup logic—even if multiple events are triggered.
281-283: Standard disposable region introduced.
Beginning theIDisposablepattern here improves clarity by grouping related cleanup logic sections.
284-315: RobustDispose(bool disposing)implementation.
Acquiring the static_disposingLockand checking_disposedprevents concurrency issues if disposal is triggered more than once. Invoking window disposal on the UI thread is both necessary and well-handled.
317-323: PublicDispose()method follows standard pattern.
CallingDispose(true)followed byGC.SuppressFinalize(this)ensures compliance with the common .NET disposal approach. No concerns here.
326-333:OnSecondAppStartedsingle-instance logic.
InvokingMainViewModel.Show()properly handles second instance activations. This aligns with the single-instance requirement.
Graceful shutdown
Refer to PowerToys Run.
MainViewModelMainWindow.xaml.csMainWindow.xaml.csSingleInstance.csMainWindowandMainViewModelwhen disposingAppTODO
PluginManager.DisposePluginsAsynccannot be waited