[Peek] UserSettings load errors not logged

[daverayment]

Microsoft PowerToys version

0.83.0

Installation method

PowerToys auto-update

Running as admin

No

Area(s) with issue?

Peek

Steps to reproduce

This is a code issue with Peek's Peek.UI.UserSettings class. If an IOException occurs when loading the settings JSON, an error is supposed to be logged, but the logging is in the wrong branch of the conditional:

catch (IOException e)
{
    if (retryCount > MaxNumberOfRetry)
    {
        retry = false;
        Logger.LogError($"Failed to Deserialize PowerToys settings, Retrying {e.Message}", e);
    }
    else
    {
        Thread.Sleep(500);
    }
}

The logging will only happen once if all retries have been exhausted, which isn't the intended flow.

A simple fix. I'm happy to submit a PR for it.

There are some other questions about the UserSettings class:

1. It implements IUserSettings which is not used elsewhere. Is this necessary for mocking during testing, or can it be removed? Sorry, I'm not familiar with the testing just yet.
2. It's not async and uses Thread.Sleep with quite a large delay. This can lock things up for a few seconds. Should we use an async method and Task.Delay? Is there a better way of handling IOExceptions in general rather than waiting?
3. There is a lock around the load code, but it is only accessed by one thread. Is this an attempt to prevent an edit of the JSON from interrupting a previous deserialization attempt? If so, that's probably not correct - we should cancel the old load and start anew.
4. The retryCount is really an attemptCount. This makes the if (retryCount > MaxNumberOfRetry) check look confusing. Small nit.
5. LoadSettingsFromJson does a lot: it checks for existence, creates default settings, loads settings, retries, and handles errors. Should some of these responsibilities be shifted into separate methods which do one thing each?

Happy to discuss those here or in a separate issue.

✔️ Expected Behavior

If an IOException occurs while loading the Peek settings file, each retry is logged.

❌ Actual Behavior

An exception is only logged if all retries have been exhausted and the load operation was abandoned.

Other Software

No response