Terminal.Gui icon indicating copy to clipboard operation
Terminal.Gui copied to clipboard

Fixes #4171 - remove all idles and replace with zero timeouts

Open tznind opened this issue 6 months ago • 4 comments

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-D to automatically reformat your files before committing.
  • [ ] My code follows the Terminal.Gui library design guidelines
  • [ ] I ran dotnet test before 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

tznind avatar Jun 19 '25 19:06 tznind

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.

tznind avatar Jun 20 '25 04:06 tznind

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.

BDisp avatar Jun 20 '25 07:06 BDisp

Yes this is also how timeouts work. They also trigger wakeup and run in ui thread

tznind avatar Jun 20 '25 07:06 tznind

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.

BDisp avatar Jun 20 '25 13:06 BDisp

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.

tznind avatar Jun 20 '25 18:06 tznind

Ok think I have fixed it, I just updated Invoke to resolve lamda if already on UI thread up front.

tznind avatar Jun 20 '25 18:06 tznind

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.

BDisp avatar Jun 20 '25 18:06 BDisp

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.

tznind avatar Jun 20 '25 18:06 tznind

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.

BDisp avatar Jun 20 '25 19:06 BDisp

I already tested and all is running well. Great job. Thanks.

BDisp avatar Jun 20 '25 21:06 BDisp

Looks like the parallel test sometimes fails in Application.Screen access.

I have added a lock in c588e04912a29b8a146c2b885b0e4dd19133ab5a which should help

tznind avatar Jun 21 '25 04:06 tznind

Looks like the parallel test sometimes fails in Application.Screen access.

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.

tig avatar Jun 21 '25 14:06 tig

Also, will you please update the multitasking.md doc to reflect these changes?

tig avatar Jun 21 '25 14:06 tig

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.

tznind avatar Jun 21 '25 16:06 tznind