Investigate suspicious preference write delay

[Jacalz]

Checklist

• I have searched the issue tracker for open issues that relate to the same feature, before opening a new one.
• This issue only relates to a single feature. I will open new issues for any other features.

Is your feature request related to a problem?

Noted in one of the race fixes PRs, this sleep looks suspicious. Sleeping to avoid race conditions is almost always a hacky solution around what really is lack of synchronization. We should look at if this sleep is correct.

fyne/app/preferences.go

Lines 50 to 63 in 4a875d9

	func (p *preferences) resetSavedRecently() {
	go func() {
	time.Sleep(time.Millisecond * 100) // writes are not always atomic. 10ms worked, 100 is safer.
	p.prefLock.Lock()
	p.savedRecently = false
	changedDuringSaving := p.changedDuringSaving
	p.changedDuringSaving = false
	p.prefLock.Unlock()
	if changedDuringSaving {
	p.save()
	}
	}()
	}

Is it possible to construct a solution with the existing API?

Yes

Describe the solution you'd like to see.

We need to find out how these writes are happening and in what way operations need to be synchronized. Perhaps running them on the same thread does not cause any problems? Maybe we just need some kind of locking inbetween?

[Jacalz]

Original issue: #2449

[andydotxyz]

I have looked into it and I think it needs to remain.
The issue is not actually that we are worried about file writes overlapping and the atomicity of a write (as one of the comments implied).
The thing that is a problem is that we want to avoid the loop with writing a file when a file change monitor is in place.

If you remove this delay then the file write happens, watcher kicks, data is re-read, change is notes, write happens etc etc.
So the (recently changed to savedRecently) flag is literally noting if we did recently save and should ignore the file change event.

[Jacalz]

Ah, okay. What I'm thinking about then is if we perhaps can call the write on the same thread and instead have some kind of way to keep track of when the last write happened, like we do for cleaning the canvas?

[andydotxyz]

I'm not sure I am following. For a period of time we need to ignore callbacks, and reset after that time.
We need to remember if a change event occurs internally during that delay.

I have a small fix that can remove the need to track if preferences are loading because that is now synchronous - but I believe the timer is still required.

[Jacalz]

Alright. I'm not sure I quite know what I'm talking about. I just find the problem interesting and want to have a look at it :)

[Jacalz]

That PR does seem very sensible. I'm sure there are more things that we can tidy up that code now that it is synchronous. That is what I'm interested in looking at. For some reason, I kind of find refactoring and cleanup more interesting to implement than new APIs :)

[andydotxyz]

I'll leave the rest to you because fundamentally I cannot see a way to have a delay in here because of the change -> write -> notify -> read -> change loop I mentioned above.

[Jacalz]

Yeah, I'm sure you're right. Just want to have a look :)