Closed FichteFoll closed 3 years ago
I tried running the tests locally, but there appears to be a problem with UnitTesting in my setup. Looks like tests passed on CI anyway.
Nice find. I did think about this case, but missed it in one condition.
There can only be 3 states:
We can totally switch between these three cases if we consider them properly. I'm in a hurry right now, but maybe this is already enough?
This will result in different behavior from the threaded implementation. Suppose that we start, stop, wait 50 milliseconds, and start again. The callback should run at t=150, 250, …, but it will actually run at t=100, 200, … because it will “re-use” the callback from the first start rather than creating a new one timed from the second (most recent) start.
There are actually infinitely many possible states:
The callback should run at t=150, 250, …, but it will actually run at t=100, 200
I don't see a problem with that. In the context of showing an acitvity indicator in the status bar, the user will not care whether the update happens 50ms earlier or later and neither does the developer. What they care about is the indicator being shown at all and updated at a consistent interval. And I don't think we documented this particular behavior either.
Implement the cancellation logic inside the scheduling class instead of a different single-purposed class with a single additional boolean that allows cancelling (or reverting) a cancellation.