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.

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

Fix ListView to be compatible when ListView.View is a GridView #979

Merged
merged 9 commits into from
Mar 19, 2024

Conversation

koal44
Copy link
Contributor

@koal44 koal44 commented Feb 28, 2024

The UI library currently breaks all ListView controls that use the GridView view. This PR adds support for GridView and showcases it in the GalleryApp

Pull request type

  • Bugfix
  • Feature

What is the current behavior?

The current behavior is that UI library interferes with how different derived types of BaseView are dynamically stylized, which renders types like GridView incompatible with this library. Although you can workaround this by voiding the library's styles, something like:

<ListView Style="{x:Null}">
   <ListView.Resources>
      <Style TargetType="ListViewItem"> <Setter Property="Background" Value="Transparent" /> </Style>
   </ListView.Resources>
   <ListView.View>
      <GridView><GridViewColumn DisplayMemberBinding="{Binding ...}" /></GridView>
   </ListView.View>
</ListView>

Digging through older framework code, it appears that when View changes, ListView uses the current View to change its own DefaultStyleKey and also apply new keys to its viewitem children. When the View is null, the DefaultStyleKey will point to ListBox and ListBoxItem (theme styles) and for GridView it'll be GridViewStyle and GridViewItemContainerStyle. When this library directly set a style on ListView and ListViewItem it broke the unorthodox approach the framework used to dynamically set styles to different views.

My first attempt was to go with the flow of the WPF framework. For example, you should be able to style ListBox and see that refelcted in a ListView with null View, but various attempts at this failed.

Since there is actually only one derived type of BaseView - namely GridView, there are effectively only two scenarios that exist - nullview and gridview, so we can use a more standard approach and use xaml styles to handle these two different cases.

What is the new behavior?

  • Extend ListView so that we can subscribe to changes in View
  • Use triggers in ListView.xaml and ListViewItem.xaml to adapt to different View states
  • Add a sample to the the Gallery app

Other information

image

@koal44 koal44 requested a review from pomianowski as a code owner February 28, 2024 00:54
@koal44
Copy link
Contributor Author

koal44 commented Feb 28, 2024

Just wanted to add there seems to be a few open issues relevant to this bugfix: #918 #436 #198

@koal44 koal44 mentioned this pull request Mar 1, 2024
1 task
src/Wpf.Ui/Controls/ListView/ListView.cs Show resolved Hide resolved
@@ -0,0 +1,60 @@
namespace Wpf.Ui.Controls;

public class ListView : System.Windows.Controls.ListView
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


public class ListView : System.Windows.Controls.ListView
{
public string ViewState
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing docs for pubplic property

@pomianowski
Copy link
Member

Hey @koal44 , thanks for your contribution to the project. I would add some documentation and improve the formatting

koal44 added 3 commits March 14, 2024 00:18
Removed the custom class ui:ListView as its purpose was to expose the ViewState, which can be simplified by directly binding to ListView.View with a ValueConverter.
@koal44 koal44 force-pushed the fix/gridview-compatibility branch from 29cb0d0 to 96d10c0 Compare March 14, 2024 12:35
@koal44
Copy link
Contributor Author

koal44 commented Mar 14, 2024

I force pushed the last commit so that it aligned better with Wpf.Ui/development.

The latest commit removes the custom ui:ListView class that was previously introduced in this PR. The purpose of the class was to expose the ListView's view state (either "Default" or "GridView") but XAML can bind just as easily to the standard ListView.View property with a ValueConverter.

I didn't mention it before, but the XAML style for GridView was adapted from the Aero2 theme with slight changes to fit this project's aesthetics. The GridViewColumnHeader's padding and margins were eyeballed to fit with the GalleryApp and should probably be reviewed so that alignment is consistent between header columns and data columns for all user apps.

@compil3
Copy link

compil3 commented Mar 15, 2024

Any idea when this will be released?
Would love to have this fixed instead of a work around that doesn't really work.


namespace Wpf.Ui.Controls;

public enum ListViewViewState
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing docs for public api

@koal44
Copy link
Contributor Author

koal44 commented Mar 19, 2024

Ok, the recent update should address reviewer concerns about documentation and VS tooling. I also re-implemented the custom ui:ListView class as it should leave the original framework's ListView unchanged and unbugged.

@pomianowski pomianowski merged commit 804bf79 into lepoco:development Mar 19, 2024
2 of 4 checks passed
@koal44 koal44 deleted the fix/gridview-compatibility branch March 20, 2024 02:24
@david-risney
Copy link

Looking forward to these ListView PRs showing up in a release! This is great, thank you!

@pomianowski
Copy link
Member

Looking forward to these ListView PRs showing up in a release! This is great, thank you!

Hey, already released in the version 3.0.3

@BingGitCn
Copy link

Tried setting the IsEnabled property to false?

@david-risney
Copy link

Looking forward to these ListView PRs showing up in a release! This is great, thank you!

Hey, already released in the version 3.0.3

I'm trying out the latest version and its looking great! Thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants