Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Peek] UserSettings load logging fix and refactor #36111

Merged

Conversation

daverayment
Copy link
Contributor

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

This PR corrects the following issues with Peek's UserSettings class:

  • A default value was manually set for CloseAfterLosingFocus in the constructor. This should be set by instantiating the PeekProperties class instead, which should be the single source for settings defaults.
  • The exception logging was in the wrong if block, meaning an error was only logged if all 6 failed attempts had been made to load the settings file.
  • retryCount was misnamed, as it represented the current attempt number, not the number of retries. This made the loop exit check difficult to understand. I'm unsure, but I think the intention was to try the load 5 times, but it ended up being 6 because of this.
  • A check was made for whether the settings file exists, and a new settings file created if it was absent. However, this is already handled by GetSettingsOrDefault.
  • (This is a bit pedantic, I admit.) The filename “settings.json” was hard-coded when setting up the FileSystemWatcher. I’ve made the SettingsUtils.DefaultFileName string public and used this instead, guaranteeing they'll always be consistent. It’s very unlikely "settings.json" would ever be changed, but you never know...

Updates were also made to the following:

  • Added a smaller number of retries with an exponential back-off delay instead of always waiting 500ms between retry attempts. This means temporary file lock issues won’t hold the application up for as long. There are now 4 attempts instead of 6 in the previous code.
  • Set the failsafe defaults using PeekProperties defaults if the settings file could not be read.
  • Moved the lock to only cover the critical section which applies the settings.
  • Added warning suppression for _watcher declaration. This is required because the FileSystemWatcher is set up in another class.
  • Simplified flow and separated out some functionality into separate methods.

Validation Steps Performed

The application was tested before and after the change, confirming that:

  • A new settings file is still created if one doesn't exist.
  • Defaults are used if the file could not be read after all retries have been exhausted or if a non-IOException happened.
  • Logging failed attempts is fixed and now correctly happens regardless of the number of attempts made to load the file.

@crutkas crutkas added Needs-Review This Pull Request awaits the review of a maintainer. in for .87 labels Nov 28, 2024
Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! We should probably apply these patterns to the other utilities we have around. I also like that we're now logging which retry number it is.
Thank you for the contribution!

@jaimecbernardo jaimecbernardo merged commit 438ee39 into microsoft:main Nov 28, 2024
11 checks passed
@daverayment
Copy link
Contributor Author

Thank you for the review and merge, @jaimecbernardo

I'm happy to take on the work for the other C# utilities which follow the same settings load pattern. It sounds like refactoring it into a common routine to remove any repetition would be a way forward. I will add a separate Issue to cover it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in for .87 Needs-Review This Pull Request awaits the review of a maintainer.
Projects
Development

Successfully merging this pull request may close these issues.

3 participants