-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
ThemeListener Refactor & Disposal #4361
base: main
Are you sure you want to change the base?
Changes from 1 commit
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 | ||
---|---|---|---|---|
|
@@ -5,6 +5,7 @@ | |||
using Microsoft.Toolkit.Uwp.UI.Helpers; | ||||
using Windows.UI.Xaml; | ||||
using Windows.UI.Xaml.Controls; | ||||
using Windows.UI.Xaml.Navigation; | ||||
|
||||
namespace Microsoft.Toolkit.Uwp.SampleApp.SamplePages | ||||
{ | ||||
|
@@ -16,9 +17,8 @@ public sealed partial class ThemeListenerPage : Page | |||
public ThemeListenerPage() | ||||
{ | ||||
this.InitializeComponent(); | ||||
Listener = new ThemeListener(); | ||||
this.Loaded += ThemeListenerPage_Loaded; | ||||
Listener.ThemeChanged += Listener_ThemeChanged; | ||||
SampleController.Current.ThemeListener.ThemeChanged += Listener_ThemeChanged; | ||||
SampleController.Current.ThemeChanged += Current_ThemeChanged; | ||||
} | ||||
|
||||
|
@@ -39,10 +39,15 @@ private void Listener_ThemeChanged(ThemeListener sender) | |||
|
||||
private void UpdateThemeState() | ||||
{ | ||||
SystemTheme.Text = Listener.CurrentThemeName; | ||||
SystemTheme.Text = SampleController.Current.ThemeListener.CurrentThemeName; | ||||
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. We have a SystemTheme method as well: WindowsCommunityToolkit/Microsoft.Toolkit.Uwp.SampleApp/Pages/SampleController.xaml.cs Line 74 in bdd1d84
(Not sure why this isn't prefixed with 'Get' like CurrentTheme is, probably something else we should clean-up.) 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. Renamed |
||||
CurrentTheme.Text = SampleController.Current.GetCurrentTheme().ToString(); | ||||
} | ||||
|
||||
public ThemeListener Listener { get; } | ||||
protected async override void OnNavigatedFrom(NavigationEventArgs e) | ||||
{ | ||||
Loaded -= ThemeListenerPage_Loaded; | ||||
SampleController.Current.ThemeListener.ThemeChanged -= Listener_ThemeChanged; | ||||
SampleController.Current.ThemeChanged -= Current_ThemeChanged; | ||||
} | ||||
} | ||||
} |
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.
We should just need the one event here. Not sure why we expose the
ThemeListener
publicly on theSampleController
as we expose everything via methods. For instance theSampleController.Current.ThemeChanged
is invoked the same as the ThemeListener:WindowsCommunityToolkit/Microsoft.Toolkit.Uwp.SampleApp/Pages/SampleController.xaml.cs
Lines 41 to 43 in bdd1d84
If we make the
ThemeListener
onSampleController
private what else breaks (once we fix this sample)?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 added the public
ThemeListener
property to the Sample Controller (see above).Since we're just interested in returning the data of a property, and not performing any computation, a property seemed more natural, as well as heeding advice from the Choosing Between Properties and Methods section of Design Guidelines for Developing Class Libraries (note: online doc is in VB, not C#, so shield your eyes accordingly):
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.
For consistency in the Sample Controller, I can convert the public property to a method, instead