-
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?
ThemeListener Refactor & Disposal #4361
Conversation
Thanks XAML-Knight for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
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 the SampleController
as we expose everything via methods. For instance the SampleController.Current.ThemeChanged
is invoked the same as the ThemeListener:
WindowsCommunityToolkit/Microsoft.Toolkit.Uwp.SampleApp/Pages/SampleController.xaml.cs
Lines 41 to 43 in bdd1d84
_themeListener.ThemeChanged += (s) => | |
{ | |
ThemeChanged?.Invoke(this, new ThemeChangedArgs { Theme = GetCurrentTheme() }); |
If we make the ThemeListener
on SampleController
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):
In general, methods represent actions and properties represent data. Properties are meant to be used like fields, meaning that properties should not be computationally complex or produce side effects. When it does not violate the following guidelines, consider using a property, rather than a method, because less experienced developers find properties easier to use.
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
@@ -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 comment
The 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
public ApplicationTheme SystemTheme() |
(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 comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed
Fixes #3555
The ThemeListener Sample Page uses it's own ThemeListener, when in reality it could just use the Sample App's Sample Controller ThemeListener instance, instead. This PR also includes some extra disposal/clean-up, when navigating away from the ThemeListener sample page.
PR Type
What kind of change does this PR introduce?
Refactoring (no functional changes, no api changes)
Sample app changes
What is the current behavior?
ThemeListener Sample Page uses it's own, extra copy of a ThemeListener, and also does not clean-up after itself (Bad 🐶 !).
What is the new behavior?
ThemeListener Sample Page now utilizes the Sample App's Sample Controller instance of a ThemeListener. ThemeListener Sample Page now also implements some clean-up, in the override of
OnNavigatedFrom
event.PR Checklist
Please check if your PR fulfills the following requirements:
Other information