-
Notifications
You must be signed in to change notification settings - Fork 39
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
Prevent Application Crash when Numeric Config Entries Contain Dynamic String Interpolations #96
base: master
Are you sure you want to change the base?
Prevent Application Crash when Numeric Config Entries Contain Dynamic String Interpolations #96
Conversation
… expected to be integer, but was dynamic (i.e. contains something like `${someIntVar}`)
…pository the key "audio.device" is referenced.
@@ -121,7 +121,7 @@ int AudioCapture::GetInitialAudioDeviceIndex(const AudioDeviceMap& deviceList) | |||
// Check if configured device is a number or string | |||
try | |||
{ | |||
audioDeviceIndex = _config->getInt("device", -1); | |||
audioDeviceIndex = _config->getInt("audio.device", -1); |
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 found references to "audio.device"
elsewhere, so I presumed "device"
was not correct.
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.
It is correct, as the audio subsystem only gets a view of the audio
config key, thus only the subkeys are used to access the data. See this line here:
frontend-sdl-cpp/src/AudioCapture.cpp
Line 20 in 9d93ead
_config = app.config().createView("audio"); |
SettingsWindow::SettingsWindow(ProjectMGUI& gui, Poco::Logger & logger) | ||
: _gui(gui) | ||
, _audioCapture(ProjectMSDLApplication::instance().getSubsystem<AudioCapture>()) | ||
, _userConfiguration(ProjectMSDLApplication::instance().UserConfiguration()) | ||
, _commandLineConfiguration(ProjectMSDLApplication::instance().CommandLineConfiguration()) |
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.
Pass logger from parent
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.
There is no need to pass loggers around. Each class creates its own Logger instance with a distinct name. If multiple instances of a class exist, they share the same instance.
See this example:
frontend-sdl-cpp/src/AudioCapture.h
Line 87 in 9d93ead
Poco::Logger& _logger{ Poco::Logger::get("AudioCapture") }; //!< The class logger. |
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.
Loggers can also have hierarchical names, e.g. GUI.SettingsWindow
. In larger applications, this makes it possible to set specific logging options for a whole subsystem, as all loggers under GUI
would get the same config if you set it for this path.
I'd recommend reading the documentation on loggers, subsystems and applications on pocoproject.com, it's quite sophisticated.
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.
In this case, the logger in the SettingsWindow
class should be declared in the header as:
Poco::Logger& _logger{ Poco::Logger::get("SettingsWindow") }; //!< The class logger.
You can then refer to it as usual.
src/gui/SettingsWindow.cpp
Outdated
try { | ||
value = _userConfiguration->getInt(property, defaultValue); | ||
_suppressPropertyWarnings.erase(property); | ||
} catch (Poco::SyntaxException & ex) { | ||
if (_suppressPropertyWarnings.find(property) == _suppressPropertyWarnings.end()) { | ||
_suppressPropertyWarnings.insert(property); | ||
_logger.warning("Encountered a non-integral property for '" + property + "', defaulting to zero and it will be clobbered if settings are saved."); | ||
} | ||
} |
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.
This gets called repeatedly, so it was necessary to leverage some mechanism to keep track of the syntax exceptions in order to suppress successive warnings.
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 don't think such a complex logic is necessary here. If a user really messes up the value, use the default and if the user saves it, just save the default value. Logging the syntax error should be enough, as is the warning level.
IMO this is basically the same as just removing the setting from the file, which is also a valid thing to do.
Poco::Logger& _logger{Poco::Logger::get("ProjectMGUI")}; //!< The class logger. | ||
|
||
MainMenu _mainMenu{*this}; | ||
SettingsWindow _settingsWindow{*this}; //!< The settings window. | ||
SettingsWindow _settingsWindow{*this, _logger}; //!< The settings window. |
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.
NOTE: Changing field order might introduce some unintended side-effects, depending on the compiler.
…val with a safety wrapper as well.
int values[2] = { | ||
_userConfiguration->getInt(property1, defaultValue1), | ||
_userConfiguration->getInt(property2, defaultValue2)}; | ||
_safeInt(property1, defaultValue1), | ||
_safeInt(property2, defaultValue2) | ||
}; |
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.
Instead of this "_safeInt" function, it'd be way easier to simply catch the exception and use the default values if something is wrong, like this:
int values[2];
try
{
values[0] = _userConfiguration->getInt(property1, defaultValue1);
values[1] = _userConfiguration->getInt(property2, defaultValue2);
}
catch (Poco::SyntaxException&)
{
values[0] = defaultValue1;
values[1] = defaultValue2;
}
This will also make sure that both values are using the default if any property fails, and not an undefined mix of both.
Initialization isn't strictly necessary because either code path sets both values if successful.
SettingsWindow::SettingsWindow(ProjectMGUI& gui, Poco::Logger & logger) | ||
: _gui(gui) | ||
, _audioCapture(ProjectMSDLApplication::instance().getSubsystem<AudioCapture>()) | ||
, _userConfiguration(ProjectMSDLApplication::instance().UserConfiguration()) | ||
, _commandLineConfiguration(ProjectMSDLApplication::instance().CommandLineConfiguration()) |
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.
In this case, the logger in the SettingsWindow
class should be declared in the header as:
Poco::Logger& _logger{ Poco::Logger::get("SettingsWindow") }; //!< The class logger.
You can then refer to it as usual.
std::set<std::string> _suppressPropertyWarnings; | ||
|
||
Poco::Logger & _logger; |
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.
std::set<std::string> _suppressPropertyWarnings; | |
Poco::Logger & _logger; | |
Poco::Logger& _logger{ Poco::Logger::get("SettingsWindow") }; //!< The class logger. |
I figured out how to run a separate instance of the visualizer on each screen by leveraging dynamic
${application.argv[n]}
options in theprojectMSDL.properties
file, however this was causing the GUI to crash as it expects to read integer values for the screen, width, and height fields. This change prevents the GUI from crashing, ignores the setting, and logs a warning about the errant property.There is also a change renaming a reference from a
"device"
property ⇒"audio.device"
, this may be incorrect but the application seems to accept it and it's consistent with appearances of"audio.device"
elsewhere in the repository.This PR allows me to... (toggle pattern)
...with properties...
Running 3 instances seems lightweight enough, my rig isn't even breaking a sweat at ~5% CPU usage, GPU load is ~35% on an RTX 3090.