-
-
Notifications
You must be signed in to change notification settings - Fork 54
chore: refactor window activated event #642
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 refactors window activation handling by replacing the manual event-driven approach with a declarative XAML-based solution. It introduces a new WindowActivationModeTrigger
class that enables direct handling of window activation state changes through XAML State Triggers instead of code-behind event handlers.
Key changes:
- Introduces
WindowActivationModeTrigger
class that extendsStateTriggerBase
for declarative window activation handling - Removes manual
CoreWindow_Activated
event handler and associated method fromMainPage.xaml.cs
- Updates XAML to use the new trigger-based approach for visual state management
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
Screenbox/Triggers/WindowActivationModeTrigger.cs | New trigger class that handles window activation state changes declaratively |
Screenbox/Screenbox.csproj | Adds the new trigger class to the project compilation |
Screenbox/Pages/MainPage.xaml.cs | Removes manual event handler registration and state management method |
Screenbox/Pages/MainPage.xaml | Updates visual state management to use the new trigger instead of manual state transitions |
Comments suppressed due to low confidence (1)
Screenbox/Pages/MainPage.xaml:415
- The 'Activated' visual state is being removed but there's no corresponding trigger-based replacement. This could break existing functionality that depends on the activated state. Consider adding a trigger for the activated state as well.
<VisualStateGroup x:Name="WindowActivationStates">
_coreWindow = Window.Current?.CoreWindow; | ||
if (_coreWindow != null) | ||
{ | ||
_coreWindow.Activated += CoreWindow_Activated; | ||
} |
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.
The Window.Current property may not be available in multi-window scenarios or when called from background threads. Consider using a more robust method to obtain the CoreWindow reference or handle potential null scenarios more explicitly.
_coreWindow = Window.Current?.CoreWindow; | |
if (_coreWindow != null) | |
{ | |
_coreWindow.Activated += CoreWindow_Activated; | |
} | |
_coreWindow = Window.Current?.CoreWindow ?? Windows.ApplicationModel.Core.CoreApplication.MainView.CoreWindow; | |
if (_coreWindow != null) | |
{ | |
_coreWindow.Activated += CoreWindow_Activated; | |
} | |
else | |
{ | |
// Log or handle the case where no CoreWindow is available | |
System.Diagnostics.Debug.WriteLine("Warning: Unable to obtain a CoreWindow reference."); | |
} |
Copilot uses AI. Check for mistakes.
_coreWindow = Window.Current?.CoreWindow; | ||
if (_coreWindow != null) | ||
{ | ||
_coreWindow.Activated += CoreWindow_Activated; |
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.
The event handler is subscribed in the constructor but never unsubscribed, which could lead to memory leaks. Consider implementing IDisposable or providing a cleanup mechanism to unsubscribe from the event when the trigger is no longer needed.
Copilot uses AI. Check for mistakes.
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.
I've been thinking about this and I don't think we can call Dispose on a CoreWindow or XAML trigger. Instead, we probably should cache the Activated event to prevent multiple calls.
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.
Very clean! Thank you for this!
Removes
CoreWindow_Activated
event handler and its associated method, which were previously responsible for handling the window activation state changes. It's replaced with a newWindowActivationModeTrigger
, allowing to directly handle the state changes inXAML
.Additionally, it facilitates the introduction of more deactivated states on any
Page
.