style: add ListView style and improve the extension #624
Conversation
dosubot
bot
added
size:L
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a unified ListView style and migrates several extension files from the Controls folder to a dedicated Extensions folder, streamlining the UI consistency and code organization. Key changes include updating XML namespace references in multiple XAML files, adding the new MediaListViewStyle with adjusted visual properties, and moving extension implementations to the project root.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Screenbox/Styles/ItemContainer.xaml | Updated ListView styles and XML namespace; revised attached property usage. |
| Screenbox/Screenbox.csproj | Removed old Controls\Extensions file inclusions and added new Extensions folder references. |
| Multiple XAML pages (SongsPage, Search pages, etc.) | Replaced old extension namespace with the new one and applied the MediaListViewStyle. |
| Screenbox/Extensions/*.cs | Updated namespaces and added improved implementations for ListView and other extensions. |
| Screenbox/Controls/PlaylistView.xaml | Updated namespace reference and ListView style application. |
| Screenbox/Controls/Extensions/ListViewExtensions.cs | Removed redundant file in favor of the new implementation in the Extensions folder. |
| Screenbox/Extensions/ApplicationViewExtensions.cs | Added copyright header and updated the namespace. |
| <!--<Setter Property="extensions:ListViewExtensions.ItemMinHeight" Value="48" />--> | ||
| <Setter Property="extensions:ListViewExtensions.ItemMargin" Value="0,2,0,2" /> | ||
| <Setter Property="extensions:ListViewExtensions.ItemCornerRadius" Value="{StaticResource MediaItemCornerRadius}" /> | ||
| <!--<Setter Property="extensions:ListViewExtensions.ItemIsFocusEngagementEnabled" Value="True" />--> |
There was a problem hiding this comment.
There are commented-out Setter lines (e.g., for ItemMinHeight) within the MediaListViewStyle. If these settings are no longer needed, consider removing them to reduce confusion and keep the XAML clean.
| <!--<Setter Property="extensions:ListViewExtensions.ItemMinHeight" Value="48" />--> | |
| <Setter Property="extensions:ListViewExtensions.ItemMargin" Value="0,2,0,2" /> | |
| <Setter Property="extensions:ListViewExtensions.ItemCornerRadius" Value="{StaticResource MediaItemCornerRadius}" /> | |
| <!--<Setter Property="extensions:ListViewExtensions.ItemIsFocusEngagementEnabled" Value="True" />--> | |
| <Setter Property="extensions:ListViewExtensions.ItemMargin" Value="0,2,0,2" /> | |
| <Setter Property="extensions:ListViewExtensions.ItemCornerRadius" Value="{StaticResource MediaItemCornerRadius}" /> |
Copilot uses AI. Check for mistakes.
| <Setter Property="ItemContainerStyle" Value="{StaticResource MediaListViewItemStyle}" /> | ||
| </Style> | ||
|
|
||
| <!-- ListViewItem with larger MinHeight and focus engagement requirement --> | ||
| <!-- https://learn.microsoft.com/en-us/windows/apps/design/input/gamepad-and-remote-interactions#focus-engagement --> | ||
| <contract13NotPresent:Style | ||
| x:Key="MediaListViewItemStyle" | ||
| BasedOn="{StaticResource DefaultListViewItemStyle}" | ||
| TargetType="ListViewItem"> | ||
| <Setter Property="MinHeight" Value="44" /> | ||
| <Setter Property="Margin" Value="0,2,0,2" /> | ||
| <Setter Property="IsFocusEngagementEnabled" Value="True" /> | ||
| </contract13NotPresent:Style> | ||
| <!-- ListViewItem featuring a larger MinHeight, and that requires focus engagement for improved gamepad interactivity and navigation --> | ||
| <contract13Present:Style | ||
| x:Key="MediaListViewItemStyle" |
There was a problem hiding this comment.
[nitpick] There appear to be two definitions for 'MediaListViewItemStyle' using different contracts (contract13NotPresent and contract13Present). It may help clarity to differentiate these styles more explicitly—either by renaming one of them or by adding a detailed comment describing their distinct purposes.
Copilot uses AI. Check for mistakes.
| private static void ItemMinHeightOnContainerContentChanging(ListViewBase sender, ContainerContentChangingEventArgs args) | ||
| { | ||
| if (args.Phase > 0 || args.InRecycleQueue) return; | ||
| if (!IsApiContract13Present && ItemMarginProperty != null) |
There was a problem hiding this comment.
Maybe we should switch the operation sequence
| /// <returns>The current value of the ItemMinHeight attached property on the specified <see cref="ListViewBase"/>.</returns> | ||
| public static double GetItemMinHeight(ListViewBase element) | ||
| { | ||
| return (double)element.GetValue(ItemMinHeightProperty); |
There was a problem hiding this comment.
Nitpick: I see the default value for ItemMinHeight property is null. But here we are doing an unsafe cast. If that property is ever be null, an exception will be thrown here.
There was a problem hiding this comment.
🤔, I thought the (... != null) guarded against that?
Wouldn't that also apply to the Thickness and CornerRadius as well?
There was a problem hiding this comment.
Oops I missed this reply. Generally, if the property has a value type (like a double), then the default value should be the default value of the type (e.g. 0d). Unless there is an explicit need for a nullable value type, the property should not be initialized as null. If you need to null-check it for something, then you should declare the type as nullable (like double?). Value type and null should not mix.
United600
force-pushed
the
listview-extension
branch
from
2eccbb5 to
d806380
Compare
| listViewBase.ContainerContentChanging -= ItemCornerRadiusOnContainerContentChanging; | ||
| listViewBase.Unloaded -= OnListViewBaseUnloaded; | ||
|
|
||
| if (ItemCornerRadiusProperty != null) |
There was a problem hiding this comment.
Also I missed this, but these conditions don't make sense. These are doing null check against the DependencyProperty objects, not the nullable values. Should be GetItemCornerRadius() != null instead. Same for other places.
huynhsontung
force-pushed
the
listview-extension
branch
from
a664f29 to
c62fa48
Compare
|
The CI is broken again 😔 |
This PR introduces a shared ListView style to enforce (and simplify) consistent UI across multiple XAML files. Certain properties remain in the container style and haven't been migrated yet, as they're still pending performance evaluation.
Controlsdirectory to the project rootApplicationViewExtensionsheader to include the appropriate copyright noticeListViewExtensionsattached propertiesMediaListViewStyleto standardize the styleListView