-
-
Notifications
You must be signed in to change notification settings - Fork 54
chore: consolidate NavigationView keyboard and gamepad handling #665
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?
Conversation
splitViewGrid.Children.Add(_overlayRoot); | ||
} | ||
|
||
if (_splitView?.FindDescendant<Border>(b => b.Name == "ContentBackground") is { } contentBackground) |
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 can't find this control? What is supposed to do?
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.
That is probably a regression. It was a custom element in the CustomNavigationView template and was supposed to be the background of the content pane. Setting the background of that Border
helps blend the nav bar transition, as demonstrated in the video in #189. I don't remember when the ContentBackground
was removed. 🤔
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 found it. It was the custom SplitView template #341. I removed it :)
I think code related to ContentBackground
is safe to delete.
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.
Oh yeah, I can see what you’re saying about the background blend effect in the nav bar transition. Maybe we can investigate how to bring it back, unless it requires adding a custom SplitView style.
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 problem was the custom SplitView style doesn't follow the theme change. I think we can add the Border behind the content pane in code behind, without touching the SplitView template. We can listen to theme change events on the NavigationViewEx and change the border background 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.
Or we could just "cheat" and simply remove the background entirely, like the system Settings app 😉.
Gravacao.2025-09-27.161425.mp4
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 problem was the custom SplitView style doesn't follow the theme change. I think we can add the Border behind the content pane in code behind, without touching the SplitView template. We can listen to theme change events on the NavigationViewEx and change the border background accordingly.
Not sure about adding yet another background layer. Right now, I'm already darkening the light dismiss background (only on Xbox) just to fix the layer missing on Xbox. I could probably just always set the content light dismiss layer to collapsed.
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.
Or we could just "cheat" and simply remove the background entirely, like the system Settings app 😉.
Honestly that may be the way. I remember I was against it for some reason. But it looks good now so why not.
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.
Honestly that may be the way. I remember I was against it for some reason. But it looks good now so why not.
It doesn't look too good on the compact mode, probably why the system settings app skips it and switches straight to minimal mode.
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 NavigationView keyboard functionality by moving behavior-based keyboard interactions into a custom control and consolidating styling resources. The change improves maintainability by reducing behavior dependencies and provides a more cohesive API for customizing NavigationView buttons.
- Moves keyboard access keys and accelerators from
NavigationViewKeyboardBehavior
toCustomNavigationView
dependency properties - Consolidates NavigationView styling resources into a dedicated XAML file
- Adds gamepad Y button support for search functionality across all display modes
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
Screenbox/Styles/NavigationView.xaml | New dedicated resource dictionary for NavigationView styling with acrylic backgrounds and animated search button style |
Screenbox/Screenbox.csproj | Project file updates to include new files and remove obsolete behavior class |
Screenbox/Pages/MainPage.xaml.cs | Removes gamepad Y button handling and helper method since functionality moved to custom control |
Screenbox/Pages/MainPage.xaml | Updates to use new resource dictionary and CustomNavigationView properties, removes behavior usage |
Screenbox/Controls/Interactions/NavigationViewKeyboardBehavior.cs | Entire file deleted as functionality moved to CustomNavigationView |
Screenbox/Controls/CustomNavigationView.cs | Major refactor adding keyboard interaction features, overlay improvements, and animation enhancements |
Screenbox/Controls/CustomNavigationView.Properties.cs | New partial class containing dependency property definitions for the custom control |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
We should add an ObservableProperty (or improve the PlayerVisible) to track player visibility, which can be bound to the Frame. This would reduce the visual tree when the play queue is empty. Also it should improve the app cold launch. <controls:CustomNavigationView.Overlay>
- <Frame x:Name="PlayerFrame" />
+ <Frame x:Name="PlayerFrame" Visibility="{x:Bind ViewModel.PlayerVisible, Mode=OneWay}" />
</controls:CustomNavigationView.Overlay> |
474a644
to
3bd3bb3
Compare
Co-authored-by: Copilot <[email protected]>
38f7c05
to
c54fc90
Compare
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
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
/// Identifies the <see cref="SettingsItemStyle"/> dependency property. | ||
/// </summary> | ||
public static readonly DependencyProperty SettingsItemStyleProperty = DependencyProperty.Register( | ||
nameof(SettingsItemStyle), typeof(Style), typeof(NavigationViewEx), new PropertyMetadata(null, OnSettingsItemStylePropertyChanged)); |
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 dependency property is missing the CloseButtonStyle
property registration. There should be a CloseButtonStyleProperty
defined between the close button access key properties and the pane toggle button properties.
Copilot uses AI. Check for mistakes.
Moves the NavigationView keyboard interaction features from the behavior to the custom control.
Initially, I considered converting it to an extension, but since the ListViewExtension PR moves the folder, I consolidated it instead.
CustomNavigationView
for access keys, keyboard accelerators and styles for the back, close, pane toggle, pane search, and settings buttonsNavigationViewKeyboardBehavior
P.S. We should consider renaming the control to something like
NavigationViewEx
.