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

[Run Plugin] Settings crashes if Numberbox is empty #32738

Closed
8LWXpg opened this issue May 6, 2024 · 9 comments
Closed

[Run Plugin] Settings crashes if Numberbox is empty #32738

8LWXpg opened this issue May 6, 2024 · 9 comments
Assignees
Labels
Cost-Small Small work item - 0-8 hours of work Issue-Bug Something isn't working Product-Settings The standalone PowerToys Settings application Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-High Bugs that we consider a blocking issue for release (crashes stuff outside of PT) Status-Reproducible This issue was reproduced by a maintainer

Comments

@8LWXpg
Copy link

8LWXpg commented May 6, 2024

Microsoft PowerToys version

0.80.1

Installation method

PowerToys auto-update, WinGet

Running as admin

Yes

Area(s) with issue?

PowerToys Run

Steps to reproduce

  • Install a plugin that has settings in Numberbox
  • Click Numberbox
  • Hit backspace
  • Hit enter
  • PowerToys settings crashes and exit

✔️ Expected Behavior

Fallback to default or NumberBoxMin.

❌ Actual Behavior

Crash and exit.

Other Software

No response

@8LWXpg 8LWXpg added Issue-Bug Something isn't working Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams labels May 6, 2024
@Aaron-Junker Aaron-Junker added Product-Settings The standalone PowerToys Settings application Severity-High Bugs that we consider a blocking issue for release (crashes stuff outside of PT) Cost-Small Small work item - 0-8 hours of work labels May 6, 2024
@8LWXpg
Copy link
Author

8LWXpg commented May 7, 2024

Note: I'm unable to hit a breakpoint under UpdateSettings with the steps mentioned above. But once the Numberbox is set to any numeric value, I am able to reach the breakpoint as intended.

code as following:

public void UpdateSettings(PowerLauncherPluginSettings settings)
{
    _count = (int?)(settings?.AdditionalOptions?.FirstOrDefault(x => x.Key == Count)?.NumberValue) ?? 5;
}

@htcfreek
Copy link
Collaborator

htcfreek commented May 7, 2024

Okay this is strange.We set NumberBoxMax and NumberBoxMin to double.Max and double.Min.
So theoretically it shouldn't accept the empty value.

@8LWXpg

  • What plugin do you use that produces the crash?
  • Can you please share a screen video?

@8LWXpg
Copy link
Author

8LWXpg commented May 7, 2024

Made a minimum example

out.mp4

@htcfreek
Copy link
Collaborator

htcfreek commented May 7, 2024

@8LWXpg
The PluginAdditionalOptions allow to define min and max values for the number box setting. Can you try if explicitly definig them fixes the bug? (If yes I think we have a problem with the is Null check in the settings app code.)

And maybe a bugreport would help.


@htcfreek htcfreek added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 7, 2024
@8LWXpg
Copy link
Author

8LWXpg commented May 7, 2024

I tested with following option, still has the same issue:

public IEnumerable<PluginAdditionalOption> AdditionalOptions =>
[
    new ()
    {
        PluginOptionType = PluginAdditionalOption.AdditionalOptionType.Numberbox,
        Key = Setting,
        DisplayLabel = "Test NumberBox",
        NumberValue = 5,
        NumberBoxMin = 2,
        NumberBoxMax = 10,
    },
];

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Team-Response An issue author responded so the team needs to follow up and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels May 7, 2024
@htcfreek
Copy link
Collaborator

htcfreek commented May 7, 2024

@8LWXpg
Does the number box ignore the min and max settings? Because normally it should fallback to the min value.

@htcfreek htcfreek added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Team-Response An issue author responded so the team needs to follow up labels May 7, 2024
@8LWXpg
Copy link
Author

8LWXpg commented May 7, 2024

Works with numbers but not when the input box is empty.

out.mp4

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Team-Response An issue author responded so the team needs to follow up and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels May 7, 2024
@htcfreek htcfreek self-assigned this May 11, 2024
@htcfreek
Copy link
Collaborator

htcfreek commented May 11, 2024

I did some tests and created a test app. The following is happening when clearing the number box and pressing enter:

winui_bug_numberBox
For the input number boxes the Minimum is set to 10 and the Maximum is set to 50.

This only happens if you clear the value. If the value resets because you type a number outside of min/max (e.g. 5000) everything is handled correctly.

@Aaron-Junker , @niels9001
It feels confusing that ...

  • the shown value in the input NumberBox (with min/max set) is different form the real value in the ViewModel variable.
  • the number box des not handle empty input correctly if bind against a double.

I think the best idea is to move the min/max validation into the ViewModel and also add a NaN validation there. Thoughts and ideas?


Explanation what happens:

If you clear the number box the value form the number box is empty as it is a normal text box with number features. That causes the view model variable to initialize with its default value (int) or beeing NaN (double). If the value of the view model variable is not between the minimum and maximum as defined in the NumberBox properties, the NumberBox shows the minimum or maximum value that is defined in XAML code instead of the view model variable value.

@htcfreek htcfreek added Status-In progress This issue or work-item is under development Status-Reproducible This issue was reproduced by a maintainer and removed Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams Needs-Team-Response An issue author responded so the team needs to follow up labels May 12, 2024
@jaimecbernardo jaimecbernardo added Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed Status-In progress This issue or work-item is under development labels May 13, 2024
@jaimecbernardo jaimecbernardo added this to To do in 0.81 Release via automation May 13, 2024
@jaimecbernardo jaimecbernardo moved this from To do to Done in 0.81 Release May 13, 2024
@htcfreek
Copy link
Collaborator

This fix is included in the v0.81.0 release.

@jaimecbernardo jaimecbernardo added this to the PowerToys 0.81 milestone Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cost-Small Small work item - 0-8 hours of work Issue-Bug Something isn't working Product-Settings The standalone PowerToys Settings application Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-High Bugs that we consider a blocking issue for release (crashes stuff outside of PT) Status-Reproducible This issue was reproduced by a maintainer
Projects
Development

No branches or pull requests

4 participants