Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ThemeListener Refactor & Disposal #4361

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

ThemeListener Refactor & Disposal #4361

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e822fc
Implement SampleController ThemeListener
XAML-Knight Nov 2, 2021
e849126
Rename method
XAML-Knight Nov 2, 2021
5c1ce3a
Removed unneeded event
XAML-Knight Jan 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Microsoft.Toolkit.Uwp.SampleApp/Pages/SampleController.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ namespace Microsoft.Toolkit.Uwp.SampleApp
public sealed partial class SampleController : Page, INotifyPropertyChanged
{
public static SampleController Current { get; private set; }
public ThemeListener ThemeListener => _themeListener;

public SampleController()
{
Expand Down
13 changes: 9 additions & 4 deletions Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ThemeListener/ThemeListenerPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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;
Copy link
Member

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)?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

}

Expand All @@ -39,10 +39,15 @@ private void Listener_ThemeChanged(ThemeListener sender)

private void UpdateThemeState()
{
SystemTheme.Text = Listener.CurrentThemeName;
SystemTheme.Text = SampleController.Current.ThemeListener.CurrentThemeName;
Copy link
Member

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
}
}
}