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

Remove ShowsUIOnMainThread, refactor UI coordination #609

Merged
merged 42 commits into from
Aug 17, 2024

Conversation

Deadpikle
Copy link
Collaborator

@Deadpikle Deadpikle commented Aug 11, 2024

This param just doesn't belong in the core lib and should be entirely handled at the UI lib level if it exists at all. Furthermore, Avalonia never supported it, and WPF users probably didn't want it, and it was broken in some cases for WinForms, so it was not good to even have it available for those UI libraries.

Refactors UI stuff for easier code maintenance, better separation of concerns, etc.


NOTE: This is definitely a WIP and depending on how this goes I might close this PR and start over. May need to do bigger refactoring than initially expected to make this fully work, perhaps even making the UIs fully rely on events rather than direct callbacks or something. Hm hm hmm....getting rid of the new Thread for the progress/update/etc windows is cleaner code, but then on WinForms if the main form is closed, the other forms are closed, which is behavior we don't want (we want user to be able to update the software w/o the main form open). Logically this should be handled in the UI side of the lib.

Basically we have a catch-22 right now: If we keep the UI interfaces the way they are, and the core lib can talk directly to UI objects, we need to keep the sync context around and/or marshal everything through the UI funcs so we make sure to talk to the UI on the right thread. But then events and things are gross, as an event being triggered really shouldn't be dependent on thread, yet that's how the code is set up now.

If we made the UI just use events, and essentially the UI and core lib talk back and forth that way, the UI libs would be more repetitive code between them, but it makes the core lib a lot cleaner.

Hm hm hmm...need to think through more. Might close this PR and restart, not sure.


Design goals:

  • Work out of the box just fine for most users.
    • Currently attempting this by making the default behavior just be posting to _syncContext.Post. This works in our samples. If user makes their UIFactory implement IUIThreadManager, then they can handle this themselves.
  • Allow for flexibility if end user needs it for some reason
  • Don't increase main user's difficulty in implementing their own UI as much as possible
  • Keep UI logic out of the core lib as we can
  • Not pull our hair out

To do/try:

  • Update README to encourage user that SparkleUpdater should really be started on the main thread (not required technically but they'll have to make sure all the threading things are done properly if sparkle makes callbacks on unexpected threads (aka from the automated background thread loop)!) -- tell them to see samples
  • Make SparkleUpdater assume it is on main thread. Remove all calls to PerformActionOnUIThread/etc. (Will pull back in if 100% necessary, but it shouldn't be at this point.)
  • Fix events and show update window call in worker loop to be on main thread via sync context -- otherwise we'll crash and burn as it does as of this typing
  • Fix order of operations for things (or some other fix) so that they are done more reasonably/easily/users can run async things in the events - see Async event handlers #571 for use case; might just add Async events and prioritize those instead of following Async event handlers #571
  • Fix/make possible for winforms to have main window closed and updates still work again (probably requires another thread or something, idk right now) -- fixed by adding multithreading sample to show user how to do it
  • Add WinForms sample for user to see how to do things in a multithreaded env where every window is on its own thread
  • Make sure in samples that only one release notes window/etc. is shown at a time
  • BUG: WPF sample (need to check others) -- if update window closed, it never opens again (aka a new one is never created).

What we did: Instead of making SparkleUpdater super gross by handling whatever sort of threading the user wants to throw at us, we simplified by forcing end users to do complex cases themselves via events. They can even use the built in UI objects, but they can't use the UIFactory stuff with multiple threads -- it just won't work, and SparkleUpdater isn't meant to handle it. If they want to show the UI on threads that aren't the main thread OR (on winforms) make it so the main form can be closed by other update forms stick around, they'll need to follow the NetSparkle.Samples.Forms.Multithread sample and use events to handle showing/hiding the UI themselves.

May want to revisit the "UIs are handled by events" after this change as that might not be to bad to pull off at this point...and would simplify the core lib more...not sure on that one.


Closes:

Closes #470
Closes #469
Closes #397
Closes #349

@Deadpikle Deadpikle mentioned this pull request Aug 16, 2024
@Deadpikle Deadpikle merged commit f3a7c82 into develop Aug 17, 2024
3 checks passed
@Deadpikle Deadpikle deleted the feature/ui-thread-changes branch August 17, 2024 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant