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

Analyzer Warning Mitigation #1039

Merged
merged 34 commits into from
Apr 14, 2024

Conversation

koal44
Copy link
Contributor

@koal44 koal44 commented Apr 6, 2024

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.

… IconElement and IcounSourceElement classes instead
- Removed commented-out Toolbox attributes
- Replaced single-line commented-out code with multi-line
- Add stylecop supression of 1502 "element should not be on a single line"
- Wrap with braces where needed
WPF0108, WPF0060, SA1642, SA1623
- Use string, double, int rather than String, Double, Int32, etc
- remove 'this' keyword when context is obvious
There were some issue when having a class called ContextMenu that wasn't actually a contextmenu but  a ResourceDictionary
There's still a lot of IDE0028 warnings as the auto fix really wants to use the C#12 collection expressions, which btw will trigger SA1010 warnings (DotNetAnalyzers/StyleCopAnalyzers#3687)
Justification: Menu was not a control.Menu, it's more like a hack to change private api settings.
… dependency properties. Also fixed some enum warnings
@koal44 koal44 requested a review from pomianowski as a code owner April 6, 2024 09:59
@github-actions github-actions bot added themes Topic is related to managing themes controls Changes to the appearance or logic of custom controls. styles Topic is related to styles PR Pull request navigation Changes to navigation related controls. icons Fonts and icons updates gallery WPF UI Gallery dotnet tray labels Apr 6, 2024
@koal44
Copy link
Contributor Author

koal44 commented Apr 6, 2024

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
Copy link
Contributor

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

@koal44
Copy link
Contributor Author

koal44 commented Apr 7, 2024

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 pomianowski self-assigned this Apr 14, 2024
@pomianowski
Copy link
Member

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

@pomianowski pomianowski merged commit c0ace5b into lepoco:development Apr 14, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
controls Changes to the appearance or logic of custom controls. dotnet gallery WPF UI Gallery icons Fonts and icons updates navigation Changes to navigation related controls. PR Pull request styles Topic is related to styles themes Topic is related to managing themes tray
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants