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

Prevent Unexpected Exits Before Applying Bad New Config #535

Closed
BobTB opened this issue Feb 9, 2024 · 2 comments
Closed

Prevent Unexpected Exits Before Applying Bad New Config #535

BobTB opened this issue Feb 9, 2024 · 2 comments

Comments

@BobTB
Copy link

BobTB commented Feb 9, 2024

Issue Description

When modifying the configuration file for GlazeWM, I've encountered a significant usability issue that affects workflow efficiency. If there happens to be a typo or any form of syntax error in the configuration file, GlazeWM immediately attempts to apply these changes and, upon encountering the error, exits. This behavior not only disrupts the current session but also requires manually reorganizing and tiling all windows again, which is time-consuming and frustrating.

Expected Behavior
Ideally, GlazeWM should validate the configuration file for errors before attempting to apply any changes.

Steps to Reproduce
Introduce a typo or syntax error in the GlazeWM configuration file.
Attempt to apply or reload the configuration.
Observe that GlazeWM exits and disrupts the current session.

@BobTB
Copy link
Author

BobTB commented Feb 9, 2024

I dont have c# installed anywhere but I think this simple fix could work:

Changing the DeserializeUserConfig method to manage exceptions internally rather than throwing a fatal error. As a result, when an error occurs during the parsing of the user configuration file, the program will not halt; instead, it will catch the exception, leaving the existing configuration intact. A notification will be displayed to inform the user of the error, thereby preserving the program's state without disrupting the workflow.

Furthermore, the code alteration entails a check within the Handle method of the EvaluateUserConfigHandler class for a null return value from DeserializeUserConfig. This null signifies that the new configuration has not been loaded successfully. If deserializedConfig is indeed null, the method will bypass the update to the current configuration, allowing the application to continue operating with the previously loaded settings. This approach ensures continuity of the application's operation despite the encountered configuration error.

private UserConfig DeserializeUserConfig(string userConfigPath)
{
  UserConfig deserializedConfig = null;
  try
  {
    var userConfigLines = File.ReadAllLines(userConfigPath);
    var input = string.Join(Environment.NewLine, userConfigLines);

    var deserializeOptions = JsonParser.OptionsFactory((options) =>
      options.Converters.Add(new BarComponentConfigConverter())
    );

    deserializedConfig = YamlParser.ToInstance<UserConfig>(input, deserializeOptions);
  }
  catch (Exception exception)
  {
    NotifyUserOfConfigError(FormatConfigError(exception));
  }
  return deserializedConfig; // Will return null if deserialization fails
}

private void NotifyUserOfConfigError(Exception exception)
{
  // Extract line and position information from the exception message if available.
  string detailedError = "";
  if (exception is JsonException jsonException && jsonException.LineNumber.HasValue && jsonException.BytePositionInLine.HasValue)
  {
    detailedError = $"(Line: {jsonException.LineNumber.Value}, Position: {jsonException.BytePositionInLine.Value}) ";
  }

  // Notify the user of the error without exiting the application.
  MessageBox.Show(
    $"Failed to parse user config. {detailedError}{exception.Message}\n\nPlease correct the config file.",
    "Config Error",
    MessageBoxButtons.OK,
    MessageBoxIcon.Error
  );
}

public CommandResponse Handle(EvaluateUserConfigCommand command)
{
  var userConfigPath = _userConfigService.UserConfigPath;

  if (!File.Exists(userConfigPath))
    InitializeSampleUserConfig(userConfigPath);

  var deserializedConfig = DeserializeUserConfig(userConfigPath);

  if (deserializedConfig == null)
  {
    // Configuration failed to load, so keep the current configuration and exit the method.
    return CommandResponse.Ok;
  }

  // ... rest of the method ...
}

@BobTB BobTB changed the title Validate Configuration Before Applying to Prevent Unexpected Exits Prevent Unexpected Exits Before Applying Bad New Config Feb 9, 2024
@lars-berger
Copy link
Member

This has been fixed now in v3 👍 No more crashes on bad config reloads

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants