UI threading issues

[Deadpikle]

The ShowsUIOnMainThread thing is a mess. Originally intended in the 0.x/1.x series (which were WinForms only) to allow the forms to be shown on a separate thread and handled separately, the code has gotten more and more gross over the years such that it is confusing to understand in the first place and has associated bugs. I don't know why more people haven't complained.

• The code is just confusing and hard to understand and is not well documented. I question my own understanding of it, and I'm the one that wrote a chunk of it.
• [x] #349
• [x] Set ShowsUIOnMainThread to false -> Start sample -> Close main window -> Update window no longer responds to button clicks or anything since the marshaling to the UI thread func now hangs/fails which is definitely not the intended behavior (something is up with the synchronization context; probably the UI thread no longer exists or something or the close of one window and the opening of another triggers some condition where the app starts shutting down, which probably brings us into the issue of #349 again with the race condition for the download window)
  • This affects both WPF and WinForms
  • On macOS (Avalonia UI) the update/change log window closes before the download window shows up which kills the app in this scenario.

The idea of this is "nice" on WinForms (why should this lib block all other windows) but breaks down due to the above problems.

Need to figure out some solutions to this. If this grows too much, then maybe rewriting this to be sane is a great reason to start the journey to a 3.x with major fixes for all this. (Which I personally do not have time for and may not for many months.)

If anyone reads this who has time, I would appreciate help. Especially if you can find ways to do this without breaking changes.

[Deadpikle]

Been thinking about this a lot the last few days. This is really caused by ShowsUIOnMainThread at the end of the day. Although this is likely fixable and could be attributed to code-bloat and poor programming over the years, mostly by yours truly, I have been reconsidering whether this option is worth the effort long-term.

By sheer number of downloads of 2.x, we are looking at around 41k-ish downloads of WinForms-based NetSparkleUpdater and 65k-ish downloads of WPF/Avalonia. (WPF alone is almost the same 41k and is 38k.) Considering that a majority of users do not want ShowsUIOnMainThread in the general case based on the issues some users are experiencing, at the very least the UIFactory should be asked for a default value, which should default to true for WPF and Avalonia.

I am seriously considering ripping out the ShowsUIOnMainThread option entirely and adding a sample for WinForms on how to do this from the user end of things so that those who want to avoid making WinForms forms model still have an easy way to set that up.

Benefits of this include:

• Cleans up this code base considerably in both understanding and lines of code
• Removes the confusion around what this code does every time someone looks at it
• Reduces accidental first-time-code-bugs when running this lib, especially from Avalonia or WPF
• Allows devs more control over how the UI is handled from their app's perspective

Cons include:

• Removes an option that has been around since the 0.x days (technically 0.x came after 1.x but that's a story) -- See here for the original commit where UseSyncronizedForms was added (and actually seems to mean the opposite of what ShowsUIOnMainThread now means since things have clearly changed -- looks like the lib originally always used a separate thread for the update window to keep it detached from the main Form)
• Forces devs to implement threading things themselves (I think this may be better for devs in the end, but worth noting)

This would be considered a breaking change and thus start the 3.0 breaking changes process, which may need to be much smaller than originally envisioned unless some others help step up and join in the dev since my time has been more limited recently (not being mean, just realistic :smile:).

I could force a default to true on WPF and Avalonia and call it a bug-fix, but that seems disingenuous at best.

Sometimes developers just make mistakes, and hindsight at what should have happened is best. We all learn over time.

I welcome input from anyone on this.

---

Edit (as I don't want to alert watchers):

For 3.x:

• I think the core lib will have ShowsUIOnMainThread ripped out. WPF likely don't want this anyway, and Avalonia users can't use it.
• So, we'll add a syncronized forms option to the WinForms UIFactory IF we determine it's necessary. (original commit for this here: https://github.com/NetSparkleUpdater/NetSparkle/commit/225ab0b79ee12ca4efb0a09d63e87112ef8f74a4 as inspiration/the original "why this was added")
  • but the original reason for this option was "So no window in background or multiple windows will be shown." which is not actually a bug for us since we handle this in other ways now. so um maybe we don't need this at all
  • Will need to add a sample for this if it's added.
• Then we will just do the talking without any "shows UI on main thread" stuff in the base library, greatly simplifying code and logic. UI's can handle the threading themselves.
• However, because netsparkle can start background threads for downloads, etc., we will probably need to keep the syncronization context around and use that to talk to the main thread, or something similar. if we have to, we can send funcs to the UI factory to talk to whatever the UI thread is
• But ultimately we want to rip out the ShowsUIOnMainThread option and NOT provide it to users, basically, as it is just a mess/confusing for both outside devs AND library devs, and we will not give end users that option.
• TODO: need to investigate original lib to see what it did, my comment above says it used a separate thread for the update window and I want to see what that means/remind myself and document it.

See also if needed:

• https://stackoverflow.com/questions/11625208/accessing-ui-main-thread-safely-in-wpf
• https://stackoverflow.com/questions/4866906/how-do-i-get-the-gui-thread-of-winform

[kenjiuno]

I agree that ShowsUIOnMainThread needs to be refined.

Probably it will help if developers can bring their own dispatcher like this.

  public Action<Action> Dispatcher { get; set; }

The problem is that developer cannot control the lifetime of background thread which NetSparkle will launch implicitly.

Sometimes it needs to use Form.ShowDialog() in order to use message loop inside ShowDialog about WinForms app.

public void ShowCannotDownloadAppcast(SparkleUpdater sparkle, string appcastUrl)
{
    _newUpdateErrorForm().Using(it => it.ShowDialog());
}

Just to make sure Using is like this:

internal static class UsingExtension
{
    internal static R Using<T, R>(this T it, Func<T, R> func) where T : IDisposable
    {
        using (it)
        {
            return func(it);
        }
    }
}

If developers can bring their own control method, the problem can be mitigated.

WinForms version of Dispatcher example:

_sparkle = new SparkleUpdater(
    provideHostDataRepository.AppcastUrl.Value,
    new DSAChecker(SecurityMode.Unsafe)
)
{
    Dispatcher = action =>
    {
        synchronizationContext.Send(
            () =>
            {
                action();
            },
            null
        );
    },
    UIFactory = newUI(),
    ShowsUIOnMainThread = false,
...

WPF version of Dispatcher example:

_sparkle = new SparkleUpdater(
    provideHostDataRepository.AppcastUrl.Value,
    new DSAChecker(SecurityMode.Unsafe)
)
{
    Dispatcher = action =>
    {
        Dispatcher.Invoke(
            () =>
            {
                action();
            }
        );
    },
    UIFactory = newUI(),
    ShowsUIOnMainThread = false,
...

I think this expansion will be easier to implement without breaking current workable implementation.

if (ShowsUIOnMainThread)
{
    _syncContext.Post(delegate
    {
        UpdateAvailableWindow.Close();
        UpdateAvailableWindow = null;
    }, null);
}
else if (Dispatcher != null)
{
    Dispatcher(
        () =>
        {
            UpdateAvailableWindow.Close();
            UpdateAvailableWindow = null;
        }
    );
}
else
{
    UpdateAvailableWindow.Close();
    UpdateAvailableWindow = null;
}