Preferences don't all save when written in `CloseIntercept`

[pztrn]

Checklist

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

Describe the bug

When using fyne.App.Run() to start an application, only first written preference is saved in Preferences store.

How to reproduce

1. Launch example code.
2. Take a look at ${XDG_CONFIG_HOME}/fyne/some.test/preferences.json
3. Be surprised that only width is saved.

Screenshots

No response

Example code

package main

import (
        "fyne.io/fyne/v2"
        "fyne.io/fyne/v2/app"
)

var (
        a fyne.App
        wnd fyne.Window
)

func main() {
        a = app.NewWithID("some.test")
        wnd = a.NewWindow("Test")

        wnd.Resize(fyne.Size{
                Width: float32(a.Preferences().IntWithFallback("mainwindow/width", 800)),
                Height: float32(a.Preferences().IntWithFallback("mainwindow/height", 600)),
        })

        wnd.SetCloseIntercept(closeIntercept)

        wnd.Show()

        a.Run()
}

func closeIntercept() {
        windowSizes := wnd.Canvas().Size()

        a.Preferences().SetInt("mainwindow/width", int(windowSizes.Width))
        a.Preferences().SetInt("mainwindow/height", int(windowSizes.Height))

        wnd.Close()
}

Fyne version

2.2.3 and today's develop at b71c0f2

Go compiler version

1.18.4

Operating system

macOS

Operating system version

macOS 12.5

Additional Information

This problem does not appear when using fyne.Window.ShowAndRun.

[andydotxyz]

Is it possible this is some sort of app-close race condition? If you add a delay to the end of main does the additional time allow all preferences to save?

[pztrn]

Adding time.Sleep(time.Second) allowed preferences to save.

[andydotxyz]

OK thanks. Looks like we have to wait before returning if preferences are pending

[WetDesertRock]

I did some investigation into this today. It appears that this is actually due to the fact that the preferences have a 100ms debounce time to prevent rapid repeat saves (https://github.com/fyne-io/fyne/blob/master/app/preferences.go#L133 and https://github.com/fyne-io/fyne/blob/master/app/preferences.go#L28)

Right now this is acting as saving before the debounce, and then saving again if there was a change during the debounce. My thought is that whatever is exiting the app isn't waiting for the time.Sleep finishes.

The best solution imo is having the app do a ForceSave() right before exit. This new public method would ignore the debounce. If people agree I'll take a stab at getting this implemented this week.

[pztrn]

ForceSave sounds like a crutch that will eventually shoot us (developers) not only in leg :)

Maybe it's better to go with a flag like "ShutdownRequested" which set to true when closing application and check for it. And preferences saving should be done after close intercept function, IMO.

[andydotxyz]

I agree that baking that into public API isn't a smart move - but in addition it would be a breaking change so we can't do that (at least not before 3.0). Bear in mind also that we aim for a clean, intent based API - and ForceSave exposes internal workings that don't make sense to the end user/developer - so it's not really matching our approach.

Also saving Preference on shutdown does make sense, how best to do that may not be obvious but we can't do it by adding to the interface

[WetDesertRock]

Yeah, given your API goals that is definitely not a good route.

Looking at how the programs, I noticed that we already have an app level "cleanup" function. app.Stop() method calls a.settings.stopWatching(), meaning that if the applications exits in some way other than that method (very common probably), that function is never called. This might be benign. If so, why do we care to call stopWatching in the first place?This means we could change how the application quits. Right now there is roughly two primary flows, the logic originating within the driver (user hits the exit button provided by the OS window chrome), and the logic originating within the app (developer calls app.Quit()):

Application controlling the quit:

• driver.Quit -> program exit
• app.Quit -> driver.Quit() -> program exit

A backwards compatible (and somewhat confusing) flow would involve some back and forth between the driver and app:

• driver.Quit -> app.Quit() -> program exit
• app.Quit -> driver.Quit -> app.Quit() (exits early) -> program exit

This means that no matter what, the app.Quit already runs. To prevent this from looping infinitely, we would use a shutdownRequested (or terminating) flag stored within the app that gets set the first time it runs.

So something like this:

func (a *fyneApp) Quit() {
	if a.terminating { // make atomic, prevent circular logic
		return
	}

	a.terminating = true

	for _, window := range a.driver.AllWindows() {
		window.Close()
	}
	a.driver.Quit()

	a.settings.stopWatching()
	a.prefs.flush() // we can now make non-public API calls.
	atomic.StoreUint32(&a.running, 0)
}

Its a bit round-about, but it definitely allows more flexibility. If I were to design 3.0, I'd propose that the only way to exit the application is app.Quit, and driver.Quit only executes driver specific functions. So the TriggerStarted and TriggerStopped lifecycle hooks would be called by the app, not the driver. Perhaps there is a way to do that now, but right now it feels to me like this circular Quit logic is the best.

The only other solution I can think of would either involve public API changes, or adding a time.Sleep(110 * time.Millisecond)