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

Disentangle core lib from UI libs #600

Closed
Deadpikle opened this issue Jul 21, 2024 · 3 comments
Closed

Disentangle core lib from UI libs #600

Deadpikle opened this issue Jul 21, 2024 · 3 comments

Comments

@Deadpikle
Copy link
Collaborator

Deadpikle commented Jul 21, 2024

If we want other libraries to be able to use our user interfaces for their own software update processes that may not use the core of NetSparkleUpdater at all, we should remove the use of NetSparkle from the user interface libraries. The user interfaces can be passed just the information they need.

For example:

  • Many of the IUIFactory methods take a SparkleUpdater instance and then do nothing with it. This shouldn't be necessary. If end users want to override things with their own UIFactory and pull in things that NetSparkleUpdater has, they would be free to use their own instance(s) of NetSparkle as they please, but our core offering would still work.
  • UpdateAvailableWindowViewModel takes a SparkleUpdater instance and only uses it to talk to the ReleaseNotesUpdater. The ReleaseNotesUpdater only uses the ISignatureVerifier, so we can probably remove the need for sending a SparkleUpdater instance to ReleaseNotesUpdater entirely by just using the interfaces.
  • If someone wanted to just use the UI, pulling in one of the UI libraries is going to also pull in the core library. The core library (as of writing) is <200 KB, but it is also going to pull in ASMResolver (about 1 MB), NetSparkleUpdater.Chaos.NaCl (~200 KB), and System.Text.Json (~600 KB -- see System.Text.Json does not need to be included all the time #601). So, pulling in about 2 MB (~1.5 MB w/o S.T.Json) more, untrimmed. Not a ton, but not negligible. What can be done about this? (The UI dlls themselves are very small.)
  • If someone wanted to just use the UI, and we successfully made 0 references to NetSparkle in the UI libs, but then removed the reference from NuGet, that would break people's projects in an un-happy way and probably unexpected way, as they would need to manually go into their projects and then add a reference to the core library (whereas right now their project file only needs to reference the UI nuget).
  • Consider not passing AppCastItem to the UI end of things at all: what would that look like? Is that possible? (AKA the UI hardly knows about NetSparkle at all.)
  • Consider combining WPF/Avalonia backend view models and helpers into shared code for easier refactoring/use/etc.

Related: #77

@Deadpikle
Copy link
Collaborator Author

Deadpikle commented Aug 2, 2024

NOTE: When working on #605, to get things compiling again, I intentionally messed up AppCastItem use. AppName and AppVersionInstalled no longer exist on AppCastItem, so the UIs had to be fixed. As I didn't want to start this issue's refactor immediately, I intentionally swapped out the vars used to something different and noted it in TODOs to fix later. So, if you build from source before a new preview is available, this will be a bug until that's fixed!

This is working again

@Deadpikle
Copy link
Collaborator Author

Deadpikle commented Aug 21, 2024

Very much considering just removing the new AsmResolverAccessor in lieu of always using AssemblyDiagnosticsAccessor by default, which would remove the extra lib from NetSparkle. It's not a huge loss, and if the default assembly path finding works, then users will probably be OK. Need to test that out in a real app as before, with reflection, we basically just loaded the running DLL, but now that we are trim-safe and don't use reflection here, we need the actual path to the actual DLL.

Notably, I don't know if this will work on macOS when inside a .app file. May need to put this in the release notes and make sure users know about this breaking change.

Can also make a tweak and see if https://stackoverflow.com/questions/4764680/how-to-get-the-location-of-the-dll-currently-executing is trim safe. (System.Reflection.Assembly.GetExecutingAssembly().Location) Don't know right now. I doubt it. Reflection and trimming don't play nice together.

@Deadpikle
Copy link
Collaborator Author

Decided not to use AsmAssemblyAccessor. No real benefit to using this compared to AssemblyDiagnosticsAccessor and removes ~1 MB from the library.

For now, the UI cleanup is done. The UI is still attached to the core lib, but things are significantly smaller now and make use of less of the core NetSparkle now, so I'm happy with things for the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant