Skip to content

Conversation

@VictoriousRaptor
Copy link
Contributor

Changes

Tests

  • Invalid preview hotkey in settings.json won't crash.
  • Invalid preview hotkey or toggle hotkey can't be set.

Check if preview hotkey is a valid KeyGesture when setting and loading
Ban more printable characters
try
{
var converter = new KeyGestureConverter();
var key = (KeyGesture)converter.ConvertFromString(Settings.PreviewHotkey);
Copy link
Member

Choose a reason for hiding this comment

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

probably this is better? ConvertFromInvariantString(String)

Correct me if i am wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make a difference?

Copy link
Member

@jjw24 jjw24 Feb 26, 2023

Choose a reason for hiding this comment

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

Probably CultureInfo doesn't matter here, we are not dealing with datetime, decimal or float types.

@VictoriousRaptor VictoriousRaptor self-assigned this Feb 26, 2023
/// </summary>
/// <param name="validateKeyGestrue">Try to validate hotkey as a KeyGesture.</param>
/// <returns></returns>
public bool Validate(bool validateKeyGestrue = false)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this option? Shouldn't we always validate?

If needed can we set validate to true by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This option is for KeyGesture class and currently we use it for the preview hotkey but not for the toggle hotkey or custom query hotkey. We only need to validate it for preview hotkey.

/// <summary>
/// Designed for Preview Hotkey and KeyGesture.
/// </summary>
public bool ValidateKeyGesture { get; set; } = false;
Copy link
Member

Choose a reason for hiding this comment

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

Same here, should it be true by default?

@jjw24 jjw24 added the bug Something isn't working label Feb 26, 2023
@jjw24 jjw24 added this to the 1.14.0 milestone Feb 26, 2023
@jjw24 jjw24 enabled auto-merge February 27, 2023 22:01
@jjw24 jjw24 merged commit f78ad1c into Flow-Launcher:dev Feb 27, 2023
@jjw24 jjw24 mentioned this pull request Mar 2, 2023
@VictoriousRaptor VictoriousRaptor deleted the FixPreviewHotkey branch April 22, 2023 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

I opened Flow Launcher and it give me this error

3 participants