search

lepoco / wpfui

WPF UI provides the Fluent experience in your known and loved WPF framework. Intuitive design, themes, navigation and new immersive controls. All natively and effortlessly.
https://wpfui.lepo.co
MIT License
7.04k stars 667 forks source link

Analyzer Warning Mitigation #1039

Closed koal44 closed 2 months ago

koal44 commented 3 months ago

There are a large number of changes here with the overall goal of reducing the thousands of analyzer warnings to a more manageable amount.The majority of modifications are minor and involve routine enhancements such as standardizing documentation and enforcing syntax conventions, but there are some more notable changes.

Minor API Changes: For instance, removing the obsolete forceBackground parameter when theme switching required breaking some public Apply methods

Regression Risks: The extensive nature of these changes carries some risk as even seemingly benign changes can lead to bugs. A case in point involved a regression issue with the display of ContentDialog. The analyzer's recommendation to use SetCurrentValue instead of the CLR setter for the ContentProperty of a ContentPresenter was found to inadvertently skip the template reevaluation, preventing the dialog from showing itself.

I've reviewed the GalleryApp and everything seems okay, but I'd still recommend a thorough re-vetting of the entire library.

koal44 commented 3 months ago

Some additional comments.

Many controls had protected fields, which i changed to protected properties, but in most cases private fields would be my recommendation. There probably aren't many instances of sub-classing the controls, but I didn't think it was my call to make, so I left them as protected.

There were a few kinds of warnings that i didn't attempt. 1) Unused expression values (IDE0058) that were involved in async operations 2) Moving pinvokes to native class (CA1060) 3) Using LibraryImportAttribute over DLLImportAttribute (SYSLIB1054)

syntax-tm commented 3 months ago

You do realize that the warnings and suggestions you see are your personal code style settings, right?

koal44 commented 3 months ago

I'm not sure I do realize. There may be some warnings that are user specific -- for example the ReSharper warning suppressions I see in the code -- other warnings are very much project-wide configurable. If you look at the NuGet packages for Wpf.Ui, you can see installed there StyleCop.Analyzers-1.2.0 and WpfAnalyzers-4.1.1. Also there's a solution-wide .editorconfig file that pomianowski has obviously been tinkering around with, with comments like:

# DocumentationTextMustEndWithAPeriod: Let's enable this rule back when we shift to WinUI3 (v8.x). If we do it now, it would mean more than 400 file changes.
dotnet_diagnostic.SA1629.severity = none

Buried in some of those warnings are actual bugs. For example, CLR accessors pointing to the wrong DP (presumably due to sloppy copy-paste) or DP default values set to reference types. With so many warnings, it is easy to ignore actual red flags. In any case, enforcing rules is just good practice; switch a certain setting on and ensure that all contributors document their public members, etc.

pomianowski commented 2 months ago

Hey @koal44, thanks for taking the time and organizing the code, a huge step in the right direction