WPF Refactoring #3274
WPF Refactoring #3274
Conversation
tom-englert
commented
Sep 3, 2024
tom-englert
commented
- Decouple AssemblyListPane from MainWindow
- Move menu/toolbar logic from MainWindow to separate service.
tom-englert
force-pushed
the
dev/WpfRefactoring
branch
from
c6697c5 to
d29d3dc
Compare
tom-englert
force-pushed
the
dev/WpfRefactoring
branch
from
f910778 to
b63b8c9
Compare
|
Do you have a "todo list" of what you intend to change or is it more like finding more sore thumbs and then deciding to fix them? Trying to get an idea how big of a thing this will be. |
|
@christophwille I think this was the biggest part (maybe bigger than I have expected), moving the business logic out of the views and reducing component dependencies. Next steps are to get rid of the singletons and replace them with DI, and to migrate the DI framework to Microsoft.Extensions.DependencyInjection Of course there will always be some obstacles left that will be fixed when stumbling upon them. |
|
Also it should be stable at any time, so it would be great if someone could do some smoke testing with the current version. |
|
Ok, then let's first take on this PR as-is, and do the DI separately next. The PR is way big anyways. |
|
btw. what was the intention behind the ILSpyX project? Just to make sure I don't break things. |
|
It is code that can be shared between frontends. Eg the VS code extension or the Avalonia based one. It says so on the tin https://github.com/icsharpcode/ILSpy/blob/master/ICSharpCode.ILSpyX/PackageReadme.md |
| }); | ||
| } | ||
|
|
||
| public async void NavigateOnLaunch(string navigateTo, string[] activeTreeViewPath, ISettingsProvider spySettings, List<LoadedAssembly> relevantAssemblies) |
There was a problem hiding this comment.
Please don't use async void unless it's absolutely necessary. And, if it's necessary call-sites need a comment hinting at the fact and we need to make sure it does not cause problems.
There was a problem hiding this comment.
Fully agree, however this is not new code, it is just moved from MainWindow.xaml.cs an I did not want to introduce too much functional changes in this PR, but just decouple UI and model, so maybe we can go with this and clean it up later since this PR is already huge.
|
@tom-englert thank you very much for your work. However, can we please in the future split the work into smaller PRs? 144 changed files is a bit much... currently I cannot allocate the necessary time to thoroughly review PRs of this size. Thanks! |
|
@siegfriedpammer just take every single commit as an individual PR, maybe then it's easier to follow. (Stacking PRs that depend on each other would not have made it easier, and if I had to wait between related refactorings until parts are reviewed, I would have lost track of what I wanted to do.) E.g. the first three commits are about moving the tree view logic out of The other commits should be mostly self explanatory by their comment, however if there are still questions just give me some feedback. |
tom-englert
force-pushed
the
dev/WpfRefactoring
branch
from
b63b8c9 to
0b477f7
Compare
So when this refactoring is finished, there will be another big pile of code that no longer depends on WPF and can be moved to ILSpyX |
|
Minor comment: please add a file/license header to any new files. Thanks! |
|
I quickly took a look at all your changes and they seem fine. Thank you for doing all this work and sorry again for taking so long to review. |
|
That's all fine - this was a really big chunk with all the side effects it created. |
