Invoke GUI update calls in SetupDefaultSearch() descendants

[DasSkelett]

Problem

With the restructuring to the search done in #3323 we added a call to SetupDefaultSearch(), which indirectly modifies GUI controls through other functions it calls.
As we all know, updating GUI components from another thread basically doesn't work.

In this case it manifests itself through freezes at the end of the repository update for some users.

Changes

Now ManageMods.Filter() and ManageMods.SetSearches(), both called in SetupDefaultSearch(), wrap every GUI-related work and call in a Util.Invoke().

The diff looks worse than the actual change 😄

I wonder if there's some static code analysis tool that can catch this? At least my IDE doesn't :/

I can't confirm the effectiveness of this patch, as my low core count tablet/laptop/convertible doesn't reproduce the hang/freeze (the runtime decides to run it all in a single thread there, I guess) and this only seems to affect Windows. I might dust off my Windows installation on my PC tomorrow to see if it experiences the bug.

Fixes #3379

@linuxgurugamer and @daantimmer, do you want to give this fix a try, after re-enabling the repo update on startup so it hits the critical code path?
If you click on the "Checks" tab in between the pull request header and body, there should be an "Artifacts" button on the right side, where you can download a ckan.exe built from this PR code.

[HebaruSan]

I wonder if there's some static code analysis tool that can catch this? At least my IDE doesn't :/

Yeah, that would be a dream come true. That's why I mentioned async/await in chat, but I don't think that quite maps properly to the GUI threading problem. Thanks for working on this, I'll review it shortly.

[HebaruSan]

This looks like what's needed. Would be wise to have confirmation from somebody who has experienced the issue, so I'm happy to wait a day or three for that.

[daantimmer]

I will have a look shortly and get back to you

…

On Mon, 24 May 2021, 01:20 HebaruSan, ***@***.***> wrote: ***@***.**** approved this pull request. This looks like what's needed. Would be wise to have confirmation from somebody who has experienced the issue, so I'm happy to wait a day or three for that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3380 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB7IZXMLDMV7DBH3BLOMHOTTPGETBANCNFSM45ME6ULA> .

[daantimmer]

I wonder if there's some static code analysis tool that can catch this? At least my IDE doesn't :/

That's why I mentioned async/await in chat

That won't help. You must update a GUI element in the GUI thread. Async/await does not do this for you. To update a GUI from a different worker thread you have to use Invoke. I always do: GuiElement.Invoke(() => expression);

[daantimmer]

@DasSkelett I can confirm this fixes my issue

[DasSkelett]

An r/KSP Discord user just reported that they couldn't see the "clear search" cross:

The debug build from this PR fixed it for them as well, so it was also caused by the background thread GUI modifications.
Funny how different the effects can be.

[HebaruSan]

You must update a GUI element in the GUI thread.

Right, the problem is that the cognitive load for doing this by hand across an entire application is unsustainable/impracticable. If we tagged all of our code that updates a GUI property directly as async, then at least we could potentially get a compile error if we try to call it from a background thread that isn't async. And we could easily check whether all our instances of DoWhatever().Result were using Invoke. I wonder if we could make our own [GUIOnly] attribute work somehow...