Track when the gui is idle, add g.WaitIdle(duration, timeout)

[zigdon]

Potential fix for #100

[mjarkk]

How about something like this? This way we don't have to use a Mutex

func (g *Gui) busy() func() {
  // g.isbusy = sync.waitgroup
  g.isbusy.Add(1)
  return func() {
    g.isbusy.Done()
  }
}

[zigdon]

Oh, I think I misunderstood the wg idea -- basically each busy call would be

  # in a worker thread
  f := g.busy()
  # do stuff
  f()

then WaitIdle would be instantaneous, rather than waiting for an idle period?

func (g *Gui) WaitIdle(timeout time.Duration) error {
  expire := time.After(timeout)
  wait := make(chan struct{})
  go func() {
    g.isbusy.Wait()
    wait <- struct{}{}
  }()
  select {
    case <- expire:
      return ErrTimeout
    case <- wait:
      return nil
  }
}

I can see how that might work. It might mean we have dangling wait threads if we keep hitting timeouts though.

Though before I figured that out, I tried this approach - busy() just writes to a channel (so doesn't block the workers), and a goroutine monitors that channel (so we have just a single writer to idleTime, meaning we don't need to deal with locking).

The advantage here is that we can still specify for how long we want the gui to be idle before returning.

What do you think?

[mjarkk]

Oh, I think I misunderstood the wg idea -- basically each busy call would be

Correct :)

It might mean we have dangling wait threads if we keep hitting timeouts though.

That's indeed true but i think they should be very short if the returned method from .busy() is always called after stuff is done.

One note on your example code to prevent deadlocks we should defer closing the channel:

func (g *Gui) WaitIdle(timeout time.Duration) error {
  expire := time.After(timeout)
  wait := make(chan struct{})
  defer close(wait)
  // ...

And check if the channel is closed before sending something over it:

func IsClosed(ch <-chan struct{}) bool {
    select {
    case <-ch:
        return true
    default:
    }
    return false
}

The advantage here is that we can still specify for how long we want the gui to be idle before returning.

But do we want that? I think this method should return at the moment gocui becomes idle because this seems like a fancier way of just calling <-time.After(time.Millisecond * 50).
Maybe we should rephrase the method name to something like WaitForIdle

[zigdon]

Hmm, can't quite get it to work - take a look at this branch for what I tried.

The main problem seems to be that since everything is async, the gui actually is idle when we run the test, since all the actions are still pending. Which would basically lead to having to write 'wait(time) && waitIdle()', at which point we're back to this implementation that supports the wait directly.

Maybe what I'm trying to do isn't really possible with the current architecture -- the gui doesn't have enough visibility into the event queues to be able to tell when events are incoming.

Maybe we can create a new gocuiEvent sync, that will have an ID associated with it, and allow testingscreen to send it and be able to tell when it was processed? Since this really only matters when you're trying to run tests (and be deterministric about the state of the gui) putting the sync support there might make more sense anyway.

[mjarkk]

That's a bummer, i kinda already suspected this to happen but hoped it wouldn't.

Maybe we can create a new gocuiEvent sync

Good idea. Now that you are saying this maybe we can add a onComplete field (func()) to gocuiEvent and if its defined call it after the event is handled.
One problem tough, we currently send the simulated key events to tcell.SimulationScreen and we cannot really control what that's doing so maybe we should start with a method called SendKeySync that returns on completion that skips tcell.

[dankox]

So after reading thru it a bit, I was trying to come up with similar solution but without locking as much as possible.

The idea is, that the mainloop is a single routine which is always synced. I've created one more event for idle which is similar to user event. The only difference is that it is used in the WaitUntil function directly and this special event counts towards idleTime if non other appear.

Check it here: https://github.com/dankox/gocui/tree/feature/wait-idle-time The specific commit: https://github.com/awesome-gocui/gocui/commit/ad6d3c2c7d27a7c94aee7e6bcbfdef668fa32d00 (also the refresh rate in WaitUntil could be configurable? or as a parameter to the function?)

THIS IS FIXED, SEE NEXT POST The later test works without a problem, but I was looking at the first test and tried to use the WaitUntil in it, and I find out that sometimes it behaves strangely. It returns not finished view (probably tcell simulation has also some timer when it provides the content). This would have to be synced somehow if used in such cases.

Here you can see with times shown before and after the WaitUntil (meaning that it did wait for 50ms). The screen returned might differ from time to time and sometimes it passes without problem.

=== RUN   TestTestingScreenReturnsCorrectContent
timer before wait: 2021-10-18 14:46:37.573882 +0200 CEST m=+0.051616837
timer after wait: 2021-10-18 14:46:37.596146 +0200 CEST m=+0.073880228
Expected view content to be: "Hello world!" got: "             \n"    testing_test.go:83: 66 <nil>
--- FAIL: TestTestingScreenReturnsCorrectContent (0.07s)

timer before wait: 2021-10-18 14:45:22.393468 +0200 CEST m=+0.000863287
timer after wait: 2021-10-18 14:45:22.462619 +0200 CEST m=+0.070012006
Expected view content to be: "Hello world!" got: "Hello wor    \n"    testing_test.go:83: 66 <nil>
--- FAIL: TestTestingScreenReturnsCorrectContent (0.07s)

=== RUN   TestTestingScreenReturnsCorrectContent
timer before wait: 2021-10-18 14:57:18.445202 +0200 CEST m=+0.054155211
timer after wait: 2021-10-18 14:57:18.465855 +0200 CEST m=+0.074807177
--- PASS: TestTestingScreenReturnsCorrectContent (0.07s)

Anyway, just wanted to provide some additional ideas. So feel free to use or change or discard :)

[dankox]

So I realized what was the problem with the first test failing from time to time. It was the implementation of idleEvent.

Because the re-draw (or flush()) was triggered for every event, it could start to redraw the content and when getting content in testing package, the content could be obtained only partially (because it was partially written there).
However, for idleEvent there is no need to re-draw, so when the gocui is idle flush() won't be called (check commit https://github.com/dankox/gocui/commit/cdeb923d66b46b27a3f77ae94a887d4ccb2b52b6)

[dankox]

Ok, I just had to try the approach which is here, and here I have simplified version without those busy ticks: https://github.com/dankox/gocui/tree/feature/wait-idle-time-2

So the thing is, both of these approaches are doing kinda the same but have one problem and that is the g.lastTime or g.idleTime variables are not synced. Therefore not safe. But it works the same way, without having to mark every event with a tick.
The only time when we know event was triggered is if flush() is going to be called. Otherwise it would just wait for event to happen, hence no need to update anything and just mark time when all events are handled which can be checked in the WaitIdle routine.

However, it really needs to be synced (I didn't do it, because it was kinda working) with a lock (which I'm not sure if it's a big performance hit in the MainLoop, but on the other hand chan are synchronized in similar fashion on lower level, so I guess it might not be such a big hit or maybe it could be guarded by special variable like gocui.UseIdleTimer).

Anyway, the other implementation in the above post works and is synced thru channel so I guess it really depends what feels better (the above solution has the refreshRate which is not as precise, meaning it can have mistake by 10ms or whatever is it set to).

[mjarkk]

I think we can use the atomic package to store the time. The atomic package can't change time.Time but if we encode it as unix time we can modify the value using the atomic package

[dankox]

@mjarkk you are right, atomic is probably the best for it. I updated the last branch: https://github.com/dankox/gocui/commit/01b7b39ac84bcbfd0c7f4fb1758abb7a9fb0b157

I've added it to synchronize the time store/load and add a public function to be able to get "last update time".
I feel like this could be used by developers if needed for animation smoothness. Although not sure how much important it is in terminal UI as that one is pretty fast anyway, but it's kinda common tactic in game engines where you have delta time and calculate movement of object according to it (so it appears smooth even if the game lags).

[mjarkk]

I cannot get to like the timeout driven detecting idle approve tough i can see why we would want to go that route. If @dankox and @zigdon think this is a good idea i'm oke with @dankox's code (feature/wait-idle-time-2) as it looks quality wise good to me.

@zigdon do you have any input on @dankox's code?

[zigdon]

So our definition of idle is 'main loop is waiting'? That might be alright, I'd want to write more tests against it to make sure it behaves the way we expect. It still falls to the 'we can't know if there are events in the queue', meaning if you wanted to use it in a test you'd have to have a sleep somewhere, but there might not be a way around that without editing tcell, as discussed.

As for the code itself, I have a few nits, would be happy to comment in a PR:

• I'm not sure we need to use atomic if we only write to idleTime in one place, do we? If we don't we can keep it as a time.Time type.
• waitIdle always waits at least duration, even if the loop is already idle -- we should not assume it's not.

If I get a chance today I'll see if I can run some more tests.

[dankox]

So, let me first address the questions/objections:

@mjarkk I'm not sure what problem is with the timeout, if you could explain it, I would be glad. However, I did add the implementation of timeout=-1 (or less than 0) to not use timeout at all, as I find it more useful (check the _examples/active.go file).

@zigdon That is correct, when MainLoop is waiting for events that is when it's idle. If it receives events, it will consume them all. Not just one event, there is later consumeevents() function after first event is received which will consume all subsequent events until none is there. So, if we update g.lastTime after all events are handled then there is no more events in the queue.
Does it mean that no more events couldn't come? Nope. However, there is no solution for such thing because MainLoop is always running in separate goroutine when events are send (either events from tcell or user events).

Regarding the sleep somewhere, if you just run g.WaitIdle(time.Second, -1), you have a sleep which will wait until the UI will be idle for 1 second at least. That is a good indication that probably all the events are already resolved. If not, there is some problem with system (where it puts thread to sleep for more than a second) or some user event has long time to get resolved (which might block the MainLoop).

1. Atomic is required, because we store the time in the variable and read it in separate go routine, so it's not synchronized and can lead to incorrect values (especially the reading one). That's the reason golang has atomic package to update simple variables like int64, otherwise it wouldn't be necessary if such variables would be "atomic" by default. I think it would lead to "undefined behaviour" known from C.
2. It waits at least duration, because I assumed (and similar implementation is in this PR) that the duration starts from the point when WaitIdle is called. However, it's quite easy to update it to just check from the last update and can wait only for the "rest of the duration" time.

[dankox]

Now to the real stuff :)

I've tried those implementation only based on this PR. Didn't really check the issue (which is my fault), so I just thought WaitIdle is required for some use case.

However, if the real deal is to know when all the events in tests were processed already, then that's something else.
Now I'm going to just assume, because it's not really described in the issue what specific use case it is, but I assume that the idea is, in test I would send some keys and the only thing which I want to know is, when all those keys were already processed.
I guess this is required for the reading of the content and not getting partial output.

If that is correct, I can imagine different solution, where user would send specific "key" at the end of all the other keys/events which could have a function in it and do whatever is needed. Or maybe even have specific "exit" (like @mjarkk mentioned) which would be a function which would be called after flush() in the MainLoop. If defined it would be called, if not, it wouldn't.

So all in all, I would say, that the question is what are we trying to solve and maybe an exact scenario (or multiple scenarios) could be provided so the problem is better understandable (at least for me :)

[zigdon]

I just realized that we've drifted far enough from this particular PR, it might make sense to move this discussion to #100 ?

[mjarkk]

@dankox Oops i meant the duration argument in the WaitIdle method and the way we just wait for a timer.
I would rather like WaitIdle to return at the moment the gui becomes idle over waiting on a timer.
Tough it seems like what i would like is quite hard in the current state in gocui

[dankox]

@mjarkk oh I see now... I'll update you on this in the issue.

As @zigdon said, lets move to the issue for further discussion. For now I would just abandon the previous implementations.

[mjarkk]

@zigdon shall we close this PR?

[zigdon]

Closing this request, I think we're coming up with a better implementation in #100