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

[Settings > PT Run > PluginAdditionalOptions] Fix crash on empty number box #32832

Merged
merged 4 commits into from
May 13, 2024

Conversation

htcfreek
Copy link
Collaborator

@htcfreek htcfreek commented May 12, 2024

Summary of the Pull Request

The settings app crashes when the user clears the number box and presses enter or moves the focus away from it. This happens because the value is not a valid number and converting this to double produces double.NaN. The double.NaN then leads to the app crash.

To fix this we now check the value of the number box against NaN and if the check is true we send a NotifyPropertyChanged() command. This lets the NumberBox reload its binding value and resets the NumberBox to the last valid number input.

We should consider this PR in a HotFix if we need one after releasing v0.81.0!

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@htcfreek htcfreek changed the title `[Settings > PT Run > PluginAdditionalOptions] Fix crash on empty number box [Settings > PT Run > PluginAdditionalOptions] Fix crash on empty number box May 12, 2024
@htcfreek htcfreek self-assigned this May 12, 2024
@htcfreek htcfreek added Issue-Bug Something isn't working Product-Settings The standalone PowerToys Settings application Needs-Review This Pull Request awaits the review of a maintainer. revisit-0.82.0 Severity-High Bugs that we consider a blocking issue for release (crashes stuff outside of PT) labels May 12, 2024

This comment has been minimized.

@Jay-o-Way
Copy link
Collaborator

"severity high" + "wait for 0.82" ?

@htcfreek
Copy link
Collaborator Author

"severity high" + "wait for 0.82" ?

Issue has "Severity highd".

@jaimecbernardo marked other PRs last week with "wait for 0.82". So I thought the time frame for 0.81 is closed. @jaimecbernardo, is it closed?

@jaimecbernardo
Copy link
Collaborator

"severity high" + "wait for 0.82" ?

Issue has "Severity highd".

@jaimecbernardo marked other PRs last week with "wait for 0.82". So I thought the time frame for 0.81 is closed. @jaimecbernardo, is it closed?

We can still get some simpler ones in. This one I think can go in. We're waiting for "Patch Tuesday" tomorrow to receive latest .NET and only after that build the release candidate, so this can still get in.

Help me out understand something: this doesn't affect any current PowerToys Run plugin, right? I don't think we're using this NumberBox one ourselves, or are we? @htcfreek

@htcfreek
Copy link
Collaborator Author

@jaimecbernardo

  1. We don't use it in any of our built-in plugins.
  2. This has no impacts on any configuration files or plugin code. It only improves the value handling in the PluginOption ViewModel of the PT Run Settings page.
  3. It simply resets empty input to the last know valid value.

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! Thank you for the contribution!
Thanks for the answers @htcfreek . That's what I thought as well.

@jaimecbernardo jaimecbernardo merged commit ee91e4d into microsoft:main May 13, 2024
10 checks passed
@htcfreek htcfreek deleted the PT_RunSettingsFix branch May 13, 2024 15:45
@htcfreek htcfreek removed Needs-Review This Pull Request awaits the review of a maintainer. revisit-0.82.0 labels May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug Something isn't working Product-Settings The standalone PowerToys Settings application Severity-High Bugs that we consider a blocking issue for release (crashes stuff outside of PT)
Projects
Development

Successfully merging this pull request may close these issues.

3 participants