Fixes #4171 - remove all idles and replace with zero timeouts
Fixes
- Fixes #4171
Proposed Changes/Todos
- [ ] Replace idles with zero duration timeouts.
Pull Request checklist:
- [ ] I've named my PR in the form of "Fixes #issue. Terse description."
- [ ] My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit
CTRL-K-Dto automatically reformat your files before committing. - [ ] My code follows the Terminal.Gui library design guidelines
- [ ] I ran
dotnet testbefore commit - [ ] I have made corresponding changes to the API documentation (using
///style comments) - [ ] My changes generate no new warnings
- [ ] I have checked my code and corrected any poor grammar or misspellings
- [ ] I conducted basic QA to assure all features are working
Only test that fails is Mainloop_Invoke_Or_AddIdle_Can_Be_Used_For_Events_Or_Actions
Its very complicated and hard to understand test - I've commented it out for now in the hopes that if there is a real issue here we can make a simpler test that exhibits it.
The Post method is normally never run in the UIThread and thus the necessity of adding to the idle field which call the wakeup method that will run in the UIThread and execute the idle.
Yes this is also how timeouts work. They also trigger wakeup and run in ui thread
Yes this is also how timeouts work. They also trigger wakeup and run in ui thread
But the problem is when a timeout run some code that will call an asynchronous code the SynchronizationContext.Post method isn't hit and thus no more timeout are added. That's the reason why idle handlers and timeouts are separated.
Are you able to create a minimum repro?
when a timeout run some code that will call an asynchronous code the SynchronizationContext.Post method isn't hit
Surely this can be fixed by making it so that asynchronous code in triggered code calls Post? What bit of code does that for idles - I don't see it.
Ok think I have fixed it, I just updated Invoke to resolve lamda if already on UI thread up front.
Comment this lines in the unit test:
// Goes fine
Action a1 = StartWindow;
yield return new object [] { a1, "Click Me", "Cancel", "Pew Pew", 0, 1, 2, 3, 4 };
and run only with this lines:
// Also goes fine
Action a2 = () => Application.Invoke (StartWindow);
yield return new object [] { a2, "Click Me", "Cancel", "Pew Pew", 0, 1, 2, 3, 4 };
}
You will see that the Post method isn't hit.
Ok they all passing now... do you still think there is a problem? can you create a scenario or test that fails on this branch but works on v2_develop?
One thing that I think we should agree is that, if there is some threading/continuation thing that works with Idles, it should also work with Timeouts. So if it didn't in v2_develop then that needed fixed here in the merge of the two.
I didn't test again with the latest changes. I you already could run it without fail, that great and I don't have knowledge of other issues. Thanks.
I already tested and all is running well. Great job. Thanks.
Looks like the parallel test sometimes fails in Application.Screen access.
I have added a lock in c588e04912a29b8a146c2b885b0e4dd19133ab5a which should help
Looks like the parallel test sometimes fails in
Application.Screenaccess.I have added a lock in c588e04912a29b8a146c2b885b0e4dd19133ab5a which should help
Unless there's a way to make Application.Screen not a static (eg the v2 Instance stuff), any parallel test that relies on it being set needs to move to non-parallel.
Also, will you please update the multitasking.md doc to reflect these changes?
I have done both of these changes in https://github.com/gui-cs/Terminal.Gui/pull/4173
Going to close this PR as I'm now working on #4173 which is super set of both my recent changes PRs.