-
-
Notifications
You must be signed in to change notification settings - Fork 56
Add launch page settings #634
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
base: main
Are you sure you want to change the base?
Changes from all commits
3a31faa
15155db
4123b5e
95ab5a7
a09a2d8
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 | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,36 @@ | ||||||
using Screenbox.Core.Enums; | ||||||
using Screenbox.Pages; | ||||||
using System; | ||||||
|
||||||
namespace Screenbox.Helpers; | ||||||
public static class EnumExtensions | ||||||
{ | ||||||
public static string GetNavPageFirstLevel(this LaunchPageOption launchPageOption) | ||||||
{ | ||||||
return launchPageOption switch | ||||||
{ | ||||||
LaunchPageOption.Home => "home", | ||||||
LaunchPageOption.Songs => "music", | ||||||
LaunchPageOption.Albums => "music", | ||||||
LaunchPageOption.Artists => "music", | ||||||
LaunchPageOption.VideoFolders => "videos", | ||||||
LaunchPageOption.AllVideos => "videos", | ||||||
LaunchPageOption.Network => "network", | ||||||
LaunchPageOption.PlayQueue => "queue", | ||||||
_ => throw new ArgumentOutOfRangeException(nameof(launchPageOption), launchPageOption, null), | ||||||
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. The ArgumentOutOfRangeException message parameter is null. Consider providing a descriptive error message to help with debugging.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
}; | ||||||
} | ||||||
|
||||||
public static string GetNavPageSecondLevel(this LaunchPageOption launchPageOption) | ||||||
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. The GetNavPageSecondLevel method doesn't handle all LaunchPageOption enum values. Home, Network, and PlayQueue options will throw ArgumentOutOfRangeException when called, but these methods are used in navigation logic where any enum value could be passed. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback 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. The GetNavPageSecondLevel method will throw ArgumentOutOfRangeException for Home, Network, and PlayQueue launch page options, but these are valid enum values that may be passed to this method. Consider handling these cases or documenting that this method should only be called for specific launch page types. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
{ | ||||||
return launchPageOption switch | ||||||
{ | ||||||
LaunchPageOption.Songs => "songs", | ||||||
LaunchPageOption.Albums => "albums", | ||||||
LaunchPageOption.Artists => "artists", | ||||||
LaunchPageOption.VideoFolders => "folders", | ||||||
LaunchPageOption.AllVideos => "all", | ||||||
_ => throw new ArgumentOutOfRangeException(nameof(launchPageOption), launchPageOption, null), | ||||||
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. The ArgumentOutOfRangeException message parameter is null. Consider providing a descriptive error message to help with debugging.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
}; | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,9 @@ | |
using System.Numerics; | ||
using CommunityToolkit.Mvvm.DependencyInjection; | ||
using Screenbox.Core; | ||
using Screenbox.Core.Services; | ||
using Screenbox.Core.ViewModels; | ||
using Screenbox.Helpers; | ||
using Sentry; | ||
using Windows.ApplicationModel.Core; | ||
using Windows.ApplicationModel.DataTransfer; | ||
|
@@ -36,6 +38,8 @@ public sealed partial class MainPage : Page, IContentFrame | |
|
||
private readonly Dictionary<string, Type> _pages; | ||
|
||
private ISettingsService Settings => Ioc.Default.GetRequiredService<ISettingsService>(); | ||
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. Getting the service directly from the control breaks the MVVM pattern. Ideally, we want only the ViewModels to have access to the services. Then the control will get the value from the ViewModel. |
||
|
||
public MainPage() | ||
{ | ||
InitializeComponent(); | ||
|
@@ -145,7 +149,12 @@ private void MainPage_Loaded(object sender, RoutedEventArgs e) | |
if (!ViewModel.PlayerVisible) | ||
{ | ||
SetTitleBar(); | ||
NavView.SelectedItem = NavView.MenuItems[0]; | ||
var launchPage = Settings.LaunchPage; | ||
var launchPageTag = launchPage.GetNavPageFirstLevel(); | ||
var navItem = NavView.MenuItems | ||
.OfType<muxc.NavigationViewItem>() | ||
.FirstOrDefault(item => item.Tag?.ToString() == launchPageTag); | ||
NavView.SelectedItem = navItem ?? NavView.MenuItems[0]; | ||
_ = ViewModel.FetchLibraries(); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,16 +1,19 @@ | ||||||||||||||||||||||||||||
#nullable enable | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
using CommunityToolkit.Mvvm.DependencyInjection; | ||||||||||||||||||||||||||||
using Screenbox.Core; | ||||||||||||||||||||||||||||
using Screenbox.Core.ViewModels; | ||||||||||||||||||||||||||||
using System; | ||||||||||||||||||||||||||||
using System.Collections.Generic; | ||||||||||||||||||||||||||||
using System.Linq; | ||||||||||||||||||||||||||||
using CommunityToolkit.Mvvm.DependencyInjection; | ||||||||||||||||||||||||||||
using Screenbox.Core; | ||||||||||||||||||||||||||||
using Screenbox.Core.Services; | ||||||||||||||||||||||||||||
using Screenbox.Core.ViewModels; | ||||||||||||||||||||||||||||
using Screenbox.Helpers; | ||||||||||||||||||||||||||||
using Windows.UI.Xaml.Controls; | ||||||||||||||||||||||||||||
using Windows.UI.Xaml.Media.Animation; | ||||||||||||||||||||||||||||
using Windows.UI.Xaml.Navigation; | ||||||||||||||||||||||||||||
using NavigationView = Microsoft.UI.Xaml.Controls.NavigationView; | ||||||||||||||||||||||||||||
using NavigationViewSelectionChangedEventArgs = Microsoft.UI.Xaml.Controls.NavigationViewSelectionChangedEventArgs; | ||||||||||||||||||||||||||||
using muxc = Microsoft.UI.Xaml.Controls; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// The Blank Page item template is documented at https://go.microsoft.com/fwlink/?LinkId=234238 | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
@@ -31,6 +34,8 @@ public sealed partial class MusicPage : Page, IContentFrame | |||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
private readonly Dictionary<string, Type> _pages; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
private ISettingsService Settings => Ioc.Default.GetRequiredService<ISettingsService>(); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
public MusicPage() | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
this.InitializeComponent(); | ||||||||||||||||||||||||||||
|
@@ -55,7 +60,20 @@ protected override void OnNavigatedTo(NavigationEventArgs e) | |||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
LibraryNavView.SelectedItem = LibraryNavView.MenuItems[0]; | ||||||||||||||||||||||||||||
muxc.NavigationViewItem? libraryNavItem = null; | ||||||||||||||||||||||||||||
var launchPage = Settings.LaunchPage; | ||||||||||||||||||||||||||||
if (launchPage.GetNavPageFirstLevel() != "music") | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
libraryNavItem = (muxc.NavigationViewItem?)LibraryNavView.MenuItems[0]; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
var launchPageTag = launchPage.GetNavPageSecondLevel(); | ||||||||||||||||||||||||||||
libraryNavItem = LibraryNavView.MenuItems | ||||||||||||||||||||||||||||
.OfType<muxc.NavigationViewItem>() | ||||||||||||||||||||||||||||
.FirstOrDefault(item => item.Tag?.ToString() == launchPageTag); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
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. Calling GetNavPageSecondLevel() without checking if the launch page actually has a second level will cause an exception for Home, Network, and PlayQueue options.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback 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.
The Copilot comment is valid. The methods
Comment on lines
+70
to
+75
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. With this flow, the second-level page is set each time the user navigates to the parent page. Is this expected? The settings text suggests that this behaviour only applies to app launches. However, in reality, it is set on every page navigation. 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. From my testing, it actually only applies to app launches, not every page navigation. |
||||||||||||||||||||||||||||
LibraryNavView.SelectedItem = libraryNavItem ?? LibraryNavView.MenuItems[0]; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
ViewModel.UpdateSongs(); | ||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,16 +1,19 @@ | ||||||||||||||||||||||||||||||
#nullable enable | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
using CommunityToolkit.Mvvm.DependencyInjection; | ||||||||||||||||||||||||||||||
using Screenbox.Core; | ||||||||||||||||||||||||||||||
using Screenbox.Core.ViewModels; | ||||||||||||||||||||||||||||||
using System; | ||||||||||||||||||||||||||||||
using System.Collections.Generic; | ||||||||||||||||||||||||||||||
using System.Linq; | ||||||||||||||||||||||||||||||
using CommunityToolkit.Mvvm.DependencyInjection; | ||||||||||||||||||||||||||||||
using Screenbox.Core; | ||||||||||||||||||||||||||||||
using Screenbox.Core.Services; | ||||||||||||||||||||||||||||||
using Screenbox.Core.ViewModels; | ||||||||||||||||||||||||||||||
using Screenbox.Helpers; | ||||||||||||||||||||||||||||||
using Windows.UI.Xaml.Controls; | ||||||||||||||||||||||||||||||
using Windows.UI.Xaml.Media.Animation; | ||||||||||||||||||||||||||||||
using Windows.UI.Xaml.Navigation; | ||||||||||||||||||||||||||||||
using NavigationView = Microsoft.UI.Xaml.Controls.NavigationView; | ||||||||||||||||||||||||||||||
using NavigationViewSelectionChangedEventArgs = Microsoft.UI.Xaml.Controls.NavigationViewSelectionChangedEventArgs; | ||||||||||||||||||||||||||||||
using muxc = Microsoft.UI.Xaml.Controls; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// The Blank Page item template is documented at https://go.microsoft.com/fwlink/?LinkId=234238 | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
@@ -33,6 +36,8 @@ public sealed partial class VideosPage : Page, IContentFrame | |||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
private readonly Dictionary<string, Type> _pages; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
private ISettingsService Settings => Ioc.Default.GetRequiredService<ISettingsService>(); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public VideosPage() | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
this.InitializeComponent(); | ||||||||||||||||||||||||||||||
|
@@ -56,7 +61,20 @@ protected override void OnNavigatedTo(NavigationEventArgs e) | |||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
LibraryNavView.SelectedItem = LibraryNavView.MenuItems[0]; | ||||||||||||||||||||||||||||||
muxc.NavigationViewItem? libraryNavItem = null; | ||||||||||||||||||||||||||||||
var launchPage = Settings.LaunchPage; | ||||||||||||||||||||||||||||||
if (launchPage.GetNavPageFirstLevel() != "videos") | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
libraryNavItem = (muxc.NavigationViewItem?)LibraryNavView.MenuItems[0]; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||
var launchPageTag = launchPage.GetNavPageSecondLevel(); | ||||||||||||||||||||||||||||||
libraryNavItem = LibraryNavView.MenuItems | ||||||||||||||||||||||||||||||
.OfType<muxc.NavigationViewItem>() | ||||||||||||||||||||||||||||||
.FirstOrDefault(item => item.Tag?.ToString() == launchPageTag); | ||||||||||||||||||||||||||||||
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. Calling GetNavPageSecondLevel() without checking if the launch page actually has a second level will cause an exception for Home, Network, and PlayQueue options.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
LibraryNavView.SelectedItem = libraryNavItem ?? LibraryNavView.MenuItems[0]; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
ViewModel.UpdateVideos(); | ||||||||||||||||||||||||||||||
|
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.
Having the launch page name as an enum is also problematic in the future. If we add new page types to the app, we have to add the page at the bottom of the enum. The order of this enum is fixed once it is released. We cannot change this order (only append to it) because doing so will break the user's settings.
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.
This is true, but we could also hardcode the enum values, then we could insert new ones in the middle. Though that would not be all that elegant either.