WPF Refactoring #1: Add abstraction layer for MEF

[tom-englert]

Problem

The WPF-UI implementation is very inconsistent, e.g. often favoring the UI-first anti-pattern, with parts of the logic implemented in the UI instead of the view model.

Solution

I'd like to contribute iterative cleanup of some of the UI, starting with this PR that adds an abstraction layer to MEF, so DI can be used without a hard coded dependency to the container implementation.
This will allow to use existing tooling and maybe later update the container to some more modern implementation (e.g .Microsoft.Extensions.DependencyInjection)

Please leave some feedback here if such a cleanup is appreciated.

[siegfriedpammer]

I appreciate your support.

Given that most parts of the UI are 13 years old and were hacked together on a weekend, I don't see a problem with the UI-first approach.

However, I agree that it's time to clean it up. I am really looking forward to having proper DI instead of static state everywhere.

@christophwille what do you think?

[christophwille]

We looked at cleaning up the non-decompiler parts (UI) on and off over the years, and only ever managed to do minor improvements. Help definitely welcome especially around standardizing approaches, modern best practices (MEF, DI).

[tom-englert]

OK, I'll do some review then and now, and try to provide step by step PR's, so it's trackable what has been changed.

• What about naming conventions, formatting and use of newer language features? I it OK to normalize/update this in the touched files?
• INotifyPropertyChanged implementation is duplicated everywhere, would using AOP (https://github.com/Fody/PropertyChanged) be an option?
• Should we create an issue or discussion as a central hook for the PRs?

[christophwille]

Fody is not an option.

All net80 targets can use latest language features, all I know is that primary ctors are a touchy subject (like please don't use those).

[siegfriedpammer]

What about naming conventions, formatting and use of newer language features? I it OK to normalize/update this in the touched files?

• Naming convention is the usual .NET naming convention except that fields do not use any kind of prefix
• Use of newer language features: In general all language features can and should be used, however there are some exceptions:
  • primary constructors should only be used, if there is exactly one - the primary - constructor
  • file-scoped namespace declarations should not be applied to existing files - unless it's a refactoring that is applied to all files and the changes are all in one commit, which is then added to .git-blame-ignore-revs

[christophwille]

INPC - if base classes don't cut it, one of the current and active source generators? @siegfriedpammer or would that be a no-go?

[siegfriedpammer]

source generators are fine!

[christophwille]

• Should we create an issue or discussion as a central hook for the PRs?

If you already have a plan, surely a good idea.

[christophwille]

Is that really a good idea? That will then be applied to all the VSIX projects, ilspycmd, the installer, et cetera. We have fewer MEF projects than non-MEF projects.

[tom-englert]

GlobalPackageReferences are compile-time only by default, and this is just an analyzer helping to make consistent export definitions.

[christophwille]

@tom-englert please let us know when you consider this "first refactoring" ready for review / merge

[tom-englert]

@christophwille this is a first consistent step and ready for review

[siegfriedpammer]

This should be moved out of the namespace.

[tom-englert]

fixed

[siegfriedpammer]

May I ask you to clean your commits up before we merge this once you are done? For example 4a933f6 and 22ad795 could be merged into one commit so that the file you added and then removed, never is visible in the commit history...

Thank you very much for helping us with the UI clean up! I really appreciate it!

[tom-englert]

@siegfriedpammer OK, I'll ping you when I completed this.

[tom-englert]

@siegfriedpammer History is now cleaned up

[siegfriedpammer]

Ah cool, because this class was moved to ILSpyX, which does not reference WPF, we no longer have to deal with the "Accessibility.dll/namespace ambiguity" 👍

[christophwille]

Can we please keep third-party-tooling-specific files out of the repository?

[tom-englert]

Removing this would make the life harder for anyone using ReSharper, getting false positive warnings everywhere, making it impossible to write compliant code.
Is this what you want?

[christophwille]

"make the life harder for anyone using X" where do we draw the line then on which tool is important enough for its custom configuration to go into the repo? Nobody in 10+ years ever asked for R#.

[christophwille]

If you simply keep a copy for yourself and assume it unchanged?

[tom-englert]

Added the missing rules to .editorconfig, so that's available to everybody now, independent of the tooling

[ltrzesniewski]

Nobody in 10+ years ever asked for R#.

That's because they all use Rider now 😅

On a more serious note, I'd like to vote for being as friendly as possible towards R#/Rider users. Almost everyone at my work prefers Rider over VS, and no one uses plain VS without R# anymore.

[tom-englert]

@ltrzesniewski works now with .editorconfig instead of R#.settings, so that should be fine with Rider, too

[christophwille]

Nobody in 10+ years ever asked for R#.

That's because they all use Rider now 😅

On a more serious note, I'd like to vote for being as friendly as possible towards R#/Rider users. Almost everyone at my work prefers Rider over VS, and no one uses plain VS without R# anymore.

and nobody on the core team uses anything besides VS...

[siegfriedpammer]

Thank you very much for the great work!