Fix Cursor Animation on widget.Entry

[fpabl0]

Description:

Fixes #1778

It seems that #1778 appears, because fade animation is running when user is typing.
Cursor should be static when user is interacting with the Entry (as normally cursor animation).
Implementation in this PR could be improved, but that's the idea basically.

Checklist:

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

[fpabl0]

This is how the cursor is shown with this PR:

fix-cursor-demo.mp4

[AlbinoGeek]

This is a pleasing change, as this makes the cursor consistent with the OS cursor in Gnome (which will not flash or fade while the user is actively typing, although it has a much shorter pause than your video does.) (Also, it's a little jumpy, but again, I get what you're trying to do, and like the direction.)

[fpabl0]

@AlbinoGeek

although it has a much shorter pause than your video does.

Currently, it pauses 500 ms, should it be less?

it's a little jumpy

What do you mean?

[fpabl0]

@AlbinoGeek I have decreased the pause time, what do you think now?

cursor_anim_1.mp4

Testing this feature, I think there is a bug in cursor animation, even without the changes in this draft:

cursor_anim_bug.mp4

It seems that the problem is in the fyne.Animation implementation (it seems that the animation time is shared maybe, and when cursor animation start, sometimes its defined interval is almost done, so first 'animation' is very short). What do you think @andydotxyz ?

[andydotxyz]

sometimes its defined interval is almost done, so first 'animation' is very short). What do you think @andydotxyz ?

It's possible depending on how long the scheduler takes to kick it off.
The end time of the duration is calculated when the animation starts, maybe there is a delay somehow.

You can see in internal/animation/animation.go line 19, each animation sets it's own end time and ticks towards it on a standard FPS refresh.

[fpabl0]

@andydotxyz I have seen the animation implementation, and I think first start and end parameter values should be set just before tickAnimation (maybe checking if start and end are equal to 0, then set their values), otherwise there is a chance that user requests animation the tick has just passed, so it's animation will run after 1/60 seconds (but Fyne has already set the start and end fields), so there is 1/60 seconds less for the first animation step. What do you think? Am I missing something?

[andydotxyz]

What do you think? Am I missing something?

Your summary is correct, we could miss the first frame of animation. But is that going to lead to visual issues such as you have reported? The cursor should fade out over half a second and back in over another half, meaning that the first frame is a 60th of the animation, or 30th of the fade. Maybe there is another issue at play?

[fpabl0]

But is that going to lead to visual issues such as you have reported?

You are right. It isn't the root of the problem. In fact, that doesn't even make it better. Not sure, what it is.
Btw, do you like this change? I have inverted the cursor animation to make a smooth transition between a pause and the animation, however that breaks some tests - some of them expects the cursor is "visible" but now the cursor is not, I think that's because I inverted the animation:

Is it safe to replace them?

[andydotxyz]

I have inverted the cursor animation to make a smooth transition between a pause and the animation

It was coded originally to fade it in. With the inversion you added it appears immediately then fades out.
The test driver just runs a frame of animation at 1.0, i.e. the last frame, so that is why the images changed.

I'm not sure why this improves the pause code, perhaps there is something else at play?
Instead of writing lots of code in entry would it make more sense for us to add Animation.[Un]Pause()?
It sounds like we might not get to a complete solution for tomorrow's code freeze, but as it's not a blocker that should not be a problem.

[andydotxyz]

I have fixed the initial state of the cursor, and removed some code that was accidentally overriding the colour on refresh.
This should fix the issue you saw with the animation being sort. On develop branch now.

[fpabl0]

This should fix the issue you saw with the animation being sort.

Yes, that fixed it. Thanks :)

I'm not sure why this improves the pause code, perhaps there is something else at play?

The problem is that if I don't invert it, it looks like a jumpy transition, see:

jumpy_cursor.mp4

Inverted animation looks smooth instead:

smooth_cursor.mp4

Instead of writing lots of code in entry would it make more sense for us to add Animation.[Un]Pause()?

Hmm I am not sure, the pause in this PR is temporary so after some time the animation starts without any action. Animation.pause would do the same as the current Animation.stop, wouldn't it?

[andydotxyz]

Animation.pause would do the same as the current Animation.stop, wouldn't it?

Not really. Stop resets the animation so start runs from the beginning. Pause would remember the current state so it could be resumed (at least that’s what was in my head when I wrote the message).

It seems that the animation you want to start is from 1.0 but on entering we want it to start from 0.0. Maybe stopping the enter animation and starting a new one that is the opposite upon the timeout lapsing would solve this without changing the current animation / tests?

I think that the current code could be simplified by doing this - and we should be able to avoid the specific ticking code as well I would think.

[fpabl0]

Stop resets the animation so start runs from the beginning

You are right! My mistake. However for the cursor animation start and stop are enough, because we want to always restart from the beginning, not from animation was paused.

It seems that the animation you want to start is from 1.0 but on entering we want it to start from 0.0. Maybe stopping the enter animation and starting a new one that is the opposite upon the timeout lapsing would solve this without changing the current animation / tests?

Yes, you are right, that would solve the tests :)

I think that the current code could be simplified by doing this

I don't understand well how is the addition of Animation.pause would simplify current code, as we need to restart the animation from the beginning.

[andydotxyz]

I don't understand well how is the addition of Animation.pause would simplify current code, as we need to restart the animation from the beginning.

I assumed that the complicated code in this PR was to manage the pause / resume? It’s almost a custom animation ticker in entry_cursor_anim.go

[fpabl0]

I assumed that the complicated code in this PR was to manage the pause / resume? It’s almost a custom animation ticker in entry_cursor_anim.go

Yes, but it is not a common pause, it is a temporary pause that will auto-start the animation from the beginning after some time (not from where it was when it was paused). That's why I think Pause/Resume functions in fyne.Animation would not simplify this code as it is different? Although I think Pause/Resume function would be a great addition :), a function to know check the animation status would be great too!

[andydotxyz]

Yes, but it is not a common pause, it is a temporary pause that will auto-start the animation from the beginning after some time (not from where it was when it was paused). That's why I think Pause/Resume functions in fyne.Animation would not simplify this code as it is different?

I understand this - but surely something like pausing it when typing starts, then after the time has lapsed unpause would help? The only custom code you should need is determine when to call UnPause()...

[fpabl0]

@andydotxyz

I understand this - but surely something like pausing it when typing starts, then after the time has lapsed unpause would help? The only custom code you should need is determine when to call UnPause()...

To be honest, I don't understand what do you mean.
This is the function TemporaryPause:

func (a *entryCursorAnimation) TemporaryPause() {
	a.counter.Reset()
	a.mu.Lock()
	defer a.mu.Unlock()
	if a.anim == nil {
		return
	}
	a.anim.Stop() // 👉 here, you say it could be replaced by a.anim.Pause(), but I am not sure what is the difference?
	if !a.inverted {
		a.anim = a.createAnim(true)
	}
	if a.paused {
		return
	}
	a.paused = true
	a.cursor.FillColor = theme.PrimaryColor()
	a.cursor.Refresh()
}

And then, the animation is restarted here:

a.counter.Inc(1)
if a.counter.Value() == cursorPauseTimex10ms {
	// >>>> lock
	a.mu.Lock()
	if a.anim != nil {
		a.anim.Start() // 👉 here, you say could be replaced by  a.anim.Unpause(), but it just a different name, isn't it?
	}
	a.paused = false
	a.mu.Unlock()
	// <<<< unlock
}

[andydotxyz]

Difference between stop and pause:

Animation is running from 0.0 to 1.0, at 0.5 we want to interrupt it:

• Calling Stop at this point will halt the animation and reset it's values
• Calling Pause would (when implemented) halt the animation but have no further effect

Then when we want to restart it, we call the counterpart:

• Start will begin the animation exactly as before, starting at 0.0
• Unpause would resume the animation where it had paused, at 0.5 (or the frame right after it)

This is, in effect, the same as the difference between stop and pause when watchng a movie or listening to music - pause is a temporary interruption that does not reset your progress.

[fpabl0]

@andydotxyz Yes, that's right, but here we want to restart always from 0.0 (not from it was paused). Maybe TemporaryPause is not a good name, it should be TemporaryStop as it effectively stops the animation and reset its values, what do you think?

[fpabl0]

@andydotxyz

why does it need 2 new types, cursorTicker and realTicker

There aren't really needed, they just have sense for testing, otherwise it would be difficult to test (as you can see in entry_cursor_anim_test.go there is a type mockTicker that implements cursorTicker for testing).

why is time.After not sufficient

Because if I use it, it will not be efficient, as the timer should be stopped when user types, and time.After does not give us that functionality. Quoting golang time package documentation:

"The underlying Timer is not recovered by the garbage collector until the timer fires. If efficiency is a concern, use NewTimer instead and call Timer.Stop if the timer is no longer needed."

Also, I don't think you have to keep replacing a.anim, just Start() and Stop() should be enough.

Yes, I though it too, but in this way is how the cursor animation is implemented currently in entry.go:

	if focused {
		r.cursor.Show()
		if r.cursorAnim == nil {
			r.cursorAnim = makeCursorAnimation(r.cursor)
			r.cursorAnim.Start()
		}
	} else {
		if r.cursorAnim != nil {
			r.cursorAnim.Stop()
			r.cursorAnim = nil
		}
		r.cursor.Hide()
	}

And that's maybe because Focus event is triggered on every click and that could lead to multiple calls to Start when animation is already started.
Also anyway, I have to replace anim to ensure that the cursor fade starts as it is now and thus not change the test images (as you recommended before).

[andydotxyz]

Yes, I though it too, but in this way is how the cursor animation is implemented currently in entry.go:

That is to handle the situation when the widget loses focus so we don't want the animation any more.

Also anyway, I have to replace anim to ensure that the cursor fade starts as it is now

Calling Start() on an animation will always start it from the beginning, it does not have to have been newly created.

[fpabl0]

@andydotxyz

That is to handle the situation when the widget loses focus so we don't want the animation any more.

I know and that is what I am trying to do. If you look entry.go, you will see this:

	if focused {
		r.cursor.Show()
		r.content.entry.cursorAnim.start()
	} else {
		r.content.entry.cursorAnim.stop()
		r.cursor.Hide()
	}

That is equivalent to the current implementation. Sorry, but I don't understand what difference you are trying to tell me.

Calling Start() on an animation will always start it from the beginning, it does not have to have been newly created.

I know, I am not creating it again and again, I am just creating the first one (beginning from dim to opaque) and then when user starts typing create a second one (beginning from opaque to dim). The second one is kept until user unfocus the Entry. The first one is needed to avoid changing the test images. And the second one is needed because if I use the first one (dim to opaque) the cursor will look jumpy in the transition.

[andydotxyz]

That is equivalent to the current implementation. Sorry, but I don't understand what difference you are trying to tell me.

Yes I think I misread it sorry, it is creating only as many animations as are needed.

There aren't really needed, they just have sense for testing, otherwise it would be difficult to test (as you can see in entry_cursor_anim_test.go there is a type mockTicker that implements cursorTicker for testing).

The way that animations work in test at the moment is that they always tick the final element of the animation - you can tick them at other positions if required for testing. The more fine grained animation test approach you have made probably suits better in our overall test support if it's needed.

i.e. Animation.Tick(0.5) marks it's half-time point.

[fpabl0]

The way that animations work in test at the moment is that they always tick the final element of the animation - you can tick them at other positions if required for testing. The more fine grained animation test approach you have made probably suits better in our overall test support if it's needed.

I see, but I am not testing specifically the fyne.Animation, I am testing the cursor animation that has its own custom go routine.

[andydotxyz]

I am testing the cursor animation that has its own custom go routine.

Maybe this is where I am lost again. I thought it was a regular animation but with a delay so it's not always running.
It seems like this could be tested without the new types?

[fpabl0]

I thought it was a regular animation but with a delay so it's not always running.

Yes, it is. entryCursorAnim is only managing the fyne.Animation, when it should be in a running, interrupted or stopped state. For Interrupted state it is necessary something like a timeout to restart the animation automatically after some time. The new types are useful to test this situation (InterruptedState).

It seems like this could be tested without the new types?

Yes, it is possible, however it will introduce delays in tests, and I think it is undesired, am I wrong?

[andydotxyz]

Yes, it is possible, however it will introduce delays in tests, and I think it is undesired, am I wrong?

I don't really understand why it has to add delays if you remove the abstraction - but I think this relates to your tests wishing to test every tick of animation frame, which is why I suggested it may relate to the wider how-do-we-test-animations question. In the main toolkit when animations are concerned we test the start (before it is started) and end (what happens immediately after start) because that is how the test driver handles animations at this time.

I am not sure what your tests are trying to solve that cannot take the same simplified approach.

[fpabl0]

but I think this relates to your tests wishing to test every tick of animation frame

Sorry, I don't understand how my tests do what you say. My tests are just testing that the internal fyne.Animation is effectively stopped, when TemporaryStop is called, and automatically restarted after cursorInterruptTime.

I have updated the way tests are performed. cursorTicker interface and mockTicker types were removed. cursorTicker now is the previous realTicker. CursorTicker is needed because time.Ticker does not support Reset() function before Go 1.15.

[stuartmscott]

Would I be right in condensing this PR into the following?;

Only blink the cursor if the Entry has focus, but the user has not interacted with it for X ms

[fpabl0]

Yes, basically that is the goal of this PR.

[stuartmscott]

Hmmm, then I think it could be simplified;

1. Cursor is visible and Animation is started when Focus is gained
2. Cursor is hidden and Animation is stopped when Focus is lost
3. Whenever user inputs data into Entry (eg. TypedRune) record time.Now() as lastInputTime
4. On each Animation tick compare time.Now() with lastInputTime; if less that threshold (X ms) make Cursor fully opaque, else calculate Cursor colour as before.

[fpabl0]

Hmmm, then I think it could be simplified;

Yes, that's a possibility (Andy suggested a similar idea), the only problem with it, is that it will restart the blink in an unknown animation frame that will cause a jumpy animation. However, I think it is possible to nullify it by waiting for the expected initial frame before letting the animation calculate Cursor color as before. I will try it to see if it's possible. Thanks :)

[andydotxyz]

it will restart the blink in an unknown animation frame

I don't think this is true. Animation has no pause, each time you call Start() it will be at the beginning.

[stuartmscott]

No Pablo is right to point out that limitation in the suggestion I made where the animation is only started when the Entry gains focus and runs continuously but gets essentially "ignored" if there has been input in the last Xms. When the last input time is greater than the threshold the animation will no longer be "ignored" and this could cause the cursor to go from fully opaque to anywhere along the animation curve and could be jarring.

Perhaps the solution to this is the addition of animation.Reset() which could be called whenever the cursor is forced opaque to ensure when the animation resumes it does so from the start.

[andydotxyz]

Oh, I missed the “ignored” bit.
If you leave it running but set the cursor to opaque won’t you be continually fighting the current animation ticks?

If you call Stop() when it is to be ignored and Start() where you proposed a Resume call you don’t need a new API

[stuartmscott]

If you leave it running but set the cursor to opaque won’t you be continually fighting the current animation ticks?

Not fighting, the animation tick would have an if comparing current time to lastInputTime and either setting the colour to the animation colour or fully opaque and reseting the animation to frame 0.

If you call Stop() when it is to be ignored and Start() where you proposed a Resume call you don’t need a new API

Well then you need a timer to know when to resume the animation, I was trying to avoid this for simplicity and just using the animation ticks as the timer

[fpabl0]

Perhaps the solution to this is the addition of animation.Reset() which could be called whenever the cursor is forced opaque to ensure when the animation resumes it does so from the start.

That would be a nice addition :)

If you call Stop() when it is to be ignored and Start() where you proposed a Resume call you don’t need a new API

I don't think it is possible to call anim.Stop() inside the animation tick function, is it?

Not fighting, the animation tick would have an if comparing current time to lastInputTime and either setting the colour to the animation colour or fully opaque and reseting the animation to frame 0.

Agreed, however it would not be that simple, because when the Entry initially get focus, the current animation (the one in develop branch) runs, but when the animation is restarted after been interrupted on TypedRune, it needs to be inverted (swap start and end colors) to avoid jumpy animation. So it means we have to create two animations at least once, then we can just reset the inverted one until the Entry lose its focus.

[stuartmscott]

I think instead of the "inverted" complication we might just want to change the animation to always be a fade out, so the cursor immediately appears then fades out, and when the animation reverses it fades back in, and then repeats. I think users would expect the cursor to immediately appear when they tap the Entry so it feels responsive.

[fpabl0]

I think instead of the "inverted" complication we might just want to change the animation to always be a fade out, so the cursor immediately appears then fades out, and when the animation reverses it fades back in, and then repeats. I think users would expect the cursor to immediately appear when they tap the Entry so it feels responsive.

Yes, that was the first possibility, unfortunately it would mean a lot of failed tests :(, because now the cursor would not be visible based on the way how animations are tested (Tick(1.0) when anim.Start).

[andydotxyz]

Not fighting, the animation tick would have an if comparing current time to lastInputTime and either setting the colour to the animation colour or fully opaque and reseting the animation to frame 0.

Oh, sorry I thought you were talking about stock color animations. It could indeed be done if we code up the color animation code with an if as you suggest. Cunning.

I don't think it is possible to call anim.Stop() inside the animation tick function, is it?

You should be able to call Stop() from anywhere you like.

[stuartmscott]

unfortunately it would mean a lot of failed tests

The tests can manually tick the animation to whatever they need

[fpabl0]

Closed in favor of #1968.