Make start animations idempotent and improve runner performance

[dweymouth]

Description:

Fixes #(issue)

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

Where applicable:

• Public APIs match existing style and have Since: line.
• Any breaking changes have a deprecation path or have been discussed.
• Check for binary size increases when importing new modules.

[dweymouth]

Hmm, all the tests seem to be timing out. Deadlock? Maybe some unit test assumption was broken because the animations in fyne_demo work fine.

[andydotxyz]

Hmm, all the tests seem to be timing out. Deadlock? Maybe some unit test assumption was broken because the animations in fyne_demo work fine.

Yes it could be. There are assumptions in the Unit Test regarding threading and timing. Fixing them is a good thing if you can figure out where they are ;). I could try to take a look over the coming days if it would help.

[dweymouth]

I could try to take a look over the coming days if it would help.

Yes, I think that would be helpful. I am pretty confident my actual code changes haven't introduced the possibility of deadlock. And I gave up, at least for now, trying to track down the assumptions in the test that may need updating.

[andydotxyz]

Confirmed - container package tests time out locally as well (linux and Darwin). Investigating.

[andydotxyz]

tabs.go:441 calls r.positionAnimation.Stop() but the animation is already locked so it cannot obtain.

[andydotxyz]

Aha, animation.Start() was holding the mutex so calling Stop from within the callback would lock.

I updated it it to this and that seemed to work. I hope this helps:

// Start registers the animation with the application run-loop and starts its execution.
func (a *Animation) Start() {
	a.mutex.Lock()
	if a.state == AnimationStateRunning {
		return
	}
	a.state = AnimationStateRunning
	a.mutex.Unlock()

	d := CurrentApp().Driver().(interface{ StartAnimationInternal(*Animation) })
	d.StartAnimationInternal(a)
}

[Jacalz]

Should this not use the mutex?

[dweymouth]

Switched to atomic so we're good here

[Jacalz]

With the change proposed above, it seems like it would be a good fit to use sync/atomic (maybe Int32, or Uint32). I suppose it would be quite clean to do a .Swap() to running state and return if the old state was running. This should use a single atomic operation, I think:

old := a.state.Swap(AnimationStateRunning)
if old == AnimationStateRunning {
    return
}

[coveralls]

coverage: 64.602% (-0.07%) from 64.674%
when pulling e47e3b2 on dweymouth:animation-fix
into 8d62571 on fyne-io:develop.

[dweymouth]

OK I think there is still a possible timing issue where an animation could end up running twice if it is stopped and then immediately started again. I wonder if it's possible to move the runner to the public animation package but make it non-exported? It's hard to get communication right between the animation instance and the runner.