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

#387 Patch pack-uris in CLR strings #390

Merged
merged 5 commits into from
Feb 9, 2025

Conversation

warappa
Copy link
Contributor

@warappa warappa commented Feb 7, 2025

Add support for WPF applications/libraries which use pack-uris in their regular .NET strings (aka. "outside of XAML").
WPF-UI is an example of a library using this approach inside its ThemesDictionary and ControlsDictionary.

Main changes

  • Scans every method body for strings containing pack-uris
  • Replaces IComponentConnector patching but reuses its string-patching functionality
  • Add integration test GivenLibraryWithWpfPackUrisInClrStrings_MergedWpfApplicationRunsSuccessfully which runs new WPFPackUrisInClrStringsApplicationCore application

Misc changes

  • Add NuGet packages Microsoft.NET.Test.Sdk and NUnit3TestAdapter for Visual Studio's Test Explorer to actually be able to execute the tests

Note

The full test execution increased on my machine from 3.5 minutes to 4.5 minutes. I guess due to the thorough string inspections.
Pre-existing tests are still successful.

@warappa
Copy link
Contributor Author

warappa commented Feb 7, 2025

For reference: Test-run results screenshots of before and after this PR changes:

Before (e8e56da)
grafik

After
grafik

@KirillOsenkov
Copy link
Collaborator

I'm worried about the overhead it adds for rewriting non-WPF assemblies. Every method is visited.

Could we perhaps not add the XAML step if we don't have a reference to PresentationCore? This way if we're not a WPF app we don't pay the price.

@warappa
Copy link
Contributor Author

warappa commented Feb 8, 2025

I will look into it.

@KirillOsenkov
Copy link
Collaborator

Thanks! Let me know when it's ready, I'll merge.

@warappa
Copy link
Contributor Author

warappa commented Feb 9, 2025

I think for the issue at hand this should be it!

Besides that I played around with the sources. Would there be interest for PRs regarding a general solution maintenance like central package management, tune .editorconfig (eg. use 2 spaces in csproj files), newer C# features, integration projects building if ILPack sources change (not the case now!) etc. And maybe some performance related stuff like Regex tuning, ordinal string comparison where possible etc.?

@KirillOsenkov KirillOsenkov merged commit 83df21f into gluck:master Feb 9, 2025
1 check passed
@KirillOsenkov
Copy link
Collaborator

Thanks!

I'd like to keep the changes to the project to the absolute necessary minimum. I took over the maintenance to unify various forks, and changes will make maintenance difficult for other forks out there. Also I just don't have the time.

@warappa
Copy link
Contributor Author

warappa commented Feb 9, 2025

Thank you for your explaination. I understand your sentiment and also your problem with limited time.
As I have done some investigations already, I will propose a few smaller PRs which should be easier to assess. Some of them touch on improving collaboration (like reliable scenario test-runs).
I hope my initiative sits well with you and that you understand that this is in no way meant to negate your concerns.

@KirillOsenkov
Copy link
Collaborator

You can certainly send PRs, and I appreciate the help. Just be mindful of maintainer burden. Things like reformatting every file would not be worth it.

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.

2 participants