-
Notifications
You must be signed in to change notification settings - Fork 181
Fix ArgumentException when FontName in setting file is not valid #469
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
Fix ArgumentException when FontName in setting file is not valid #469
Conversation
|
why not implement it in the configmanager? if it would be the right place? |
I wasn't sure how to approach it better. Now moved it into ConfigManager. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an ArgumentException that occurs when the FontName specified in the configuration file is invalid, which was blocking application startup. The fix adds validation during configuration loading to detect invalid font names and fall back to the default monospace font.
Key Changes:
- Added font name validation in the configuration loading process
- Implemented fallback mechanism to default monospace font when validation fails
- Added logging to inform users when font fallback occurs
| var fontFamily = new FontFamily(settings.Preferences.FontName); | ||
| settings.Preferences.FontName = fontFamily.Name; |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FontFamily instance is created but only used to validate the font name and get its canonical name. Consider disposing of the FontFamily instance or use a more efficient validation approach to avoid potential resource leaks.
| var fontFamily = new FontFamily(settings.Preferences.FontName); | |
| settings.Preferences.FontName = fontFamily.Name; | |
| using (var fontFamily = new FontFamily(settings.Preferences.FontName)) | |
| { | |
| settings.Preferences.FontName = fontFamily.Name; | |
| } |
| _logger.Warn($"Specified font '{settings.Preferences.FontName}' not found. Falling back to default: '{FontFamily.GenericMonospace.Name}'. {ex.Message}"); | ||
| settings.Preferences.FontName = FontFamily.GenericMonospace.Name; |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FontFamily.GenericMonospace.Name is accessed multiple times (here and on line 215). Consider storing it in a variable to avoid repeated property access.
| _logger.Warn($"Specified font '{settings.Preferences.FontName}' not found. Falling back to default: '{FontFamily.GenericMonospace.Name}'. {ex.Message}"); | |
| settings.Preferences.FontName = FontFamily.GenericMonospace.Name; | |
| var genericMonospaceName = FontFamily.GenericMonospace.Name; | |
| _logger.Warn($"Specified font '{settings.Preferences.FontName}' not found. Falling back to default: '{genericMonospaceName}'. {ex.Message}"); | |
| settings.Preferences.FontName = genericMonospaceName; |
| } | ||
| catch (ArgumentException ex) | ||
| { | ||
| _logger.Warn($"Specified font '{settings.Preferences.FontName}' not found. Falling back to default: '{FontFamily.GenericMonospace.Name}'. {ex.Message}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to log the exception in the warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I'll add using to dispose of the FontFamily instance as suggested by Copilot.
| using var fontFamily = new FontFamily(settings.Preferences.FontName); | ||
| settings.Preferences.FontName = fontFamily.Name; | ||
| } | ||
| catch (ArgumentException ex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm a little nit picky here, but you can remove the "ex" variable, because we don't use it anyway :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right. Let me just remove it, then.
Fixes #468
The check for invalid font name and falling back to default font might be better placed in LogExpert.Config.ConfigManager but I've added it in LogWindow where the exception was thrown blocking application startup.