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

WPF Refactoring #3274

Merged
merged 14 commits into from
Sep 20, 2024
Merged

Conversation

tom-englert
Copy link
Contributor

  • Decouple AssemblyListPane from MainWindow
  • Move menu/toolbar logic from MainWindow to separate service.

@christophwille
Copy link
Member

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.

@tom-englert
Copy link
Contributor Author

@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.

@tom-englert
Copy link
Contributor Author

Also it should be stable at any time, so it would be great if someone could do some smoke testing with the current version.

@christophwille
Copy link
Member

Ok, then let's first take on this PR as-is, and do the DI separately next. The PR is way big anyways.

@tom-englert
Copy link
Contributor Author

btw. what was the intention behind the ILSpyX project? Just to make sure I don't break things.

@christophwille
Copy link
Member

christophwille commented Sep 13, 2024

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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@siegfriedpammer
Copy link
Member

siegfriedpammer commented Sep 14, 2024

@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!

@tom-englert
Copy link
Contributor Author

@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 MainWindow.xaml.cs into AssemblyTreeModel.cs and MenuService.cs, which looks like a huge change, but if you compare the old MainWindow.xaml.cs with the new AssemblyTreeModel.cs and MenuService.cs you will see that there is not too much new code, it just has moved.
And most of the other file changes are only side effects because the path to reach the tree model has changed, so this should be easy to review.

The other commits should be mostly self explanatory by their comment, however if there are still questions just give me some feedback.

@tom-englert
Copy link
Contributor Author

tom-englert commented Sep 15, 2024

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

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

@siegfriedpammer siegfriedpammer merged commit d8cfb8e into icsharpcode:master Sep 20, 2024
5 checks passed
@siegfriedpammer
Copy link
Member

Minor comment: please add a file/license header to any new files. Thanks!

@siegfriedpammer
Copy link
Member

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.

@tom-englert
Copy link
Contributor Author

That's all fine - this was a really big chunk with all the side effects it created.

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.

3 participants