Add validation for cutoff score in ImportPeptideSearchDlg

[bspratt]

No description provided.

[chambm]

If properties like CutOffScore are only supposed to be used by tests and not by other Skyline application code, then I wonder whether it should be a string that gets/sets textCutoff.Text. Other than that, PR looks good.

[bspratt]

I don't understand. The user can only deal in strings, we have to convert strings like textCutoff to values like CutoffScore, and gracefully deal with users typing in strings that don't make sense. We're pretty good about that, but for whatever reason this particular control never got any such validation code. The problem has been lurking for at least ten years.

[chambm]

I meant we wouldn't need the somewhat awkward SetCutoffControlText if CutoffScore was a string that gets/sets the control's text directly instead of trying to parse it. Then test code would set the text, just like a user would.

[bspratt]

I see. Keep in mind that the test would have to think about L10N when trying to represent fractional values, though. Probably even more awkward.

[chambm]

Yes, devs would have to remember to do the .ToString(CultureInfo.CurrentCulture) bit after the number (or use a helper function). Which I admit is easy to forget (and I have done so).

However the double CutoffScore is much closer to the way the rest of Skyline does this testing. The difference is things like transition settings UI use a nullable property like double? CutoffScore and test that when setting CutoffScore to null, it triggers the Resources.MessageBoxHelper_ValidateDecimalTextBox__0__must_contain_a_decimal_value error. Is there a reason not to use ControlUtil.ValidateDecimalTextBox here? Testing a null control value is probably close enough to an invalid value like "12%@$@": they both should generate the same error.

[bspratt]

Yes, I should use that more centralized code for consistency, will do.

[brendanx67]

Yes, please do. This is what I was suggesting in email. And this function is where we could benefit from expanding what we accept as a decimal number. In the days where I had written all of the code, I was asked while demoing at PNNL to type garbage into a text box for a decimal number. I complied confidently knowing all such text boxes were validated by one function.

…

On Tue, Oct 17, 2023 at 7:01 AM Brian Pratt ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pwiz_tools/Skyline/FileUI/PeptideSearch/BuildPeptideSearchLibraryControl.cs <#2752 (comment)>: > @@ -643,5 +656,15 @@ private void comboInputFileType_SelectedIndexChanged(object sender, EventArgs e) { UpdatePerformDDASearch(); } + + #region test_support + + // For test purposes, lets us test error handling + public void SetCutoffControlText(string cutoffControlText) Yes, I should use that more centralized code for consistency, will do. — Reply to this email directly, view it on GitHub <#2752 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACYBWUEVKVUEH65KGZ4WMHLX722YHAVCNFSM6AAAAAA6CY5PUWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMOBSHEYTOMJRG4> . You are receiving this because your review was requested.Message ID: ***@***.***>

[bspratt]

I'll have to add some more centralized code for dealing with a decimal textbox that is allowed to be empty under some circumstances, as this one is.

[bspratt]

Looks like it would be quite a lot of refactoring to centralize parsing of doubles - typical pattern is to use ValidateDecimalTextBox to verify that a text box can be understood as a double, then parse again later with a bare double.Parse(). So, a more L10N-tolerant handling of doubles is a nice idea but perhaps not worth the effort.

[chambm]

And handling it as an optional input seems tricky too with just a double?. Using null as "invalid input" means it can't also be used for "user left field empty".

[brendanx67]

Please don’t merge this until I can have a look at it.

…

On Tue, Oct 17, 2023 at 8:58 AM Matt Chambers ***@***.***> wrote: And handling it as an optional input seems tricky too with just a double?. Using null as "invalid input" means it can't also be used for "user left field empty". — Reply to this email directly, view it on GitHub <#2752 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACYBWUFOQZ26KBGVBCA3FPLX73IODAVCNFSM6AAAAAA6CY5PUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRWHE4TAMBXHE> . You are receiving this because your review was requested.Message ID: ***@***.***>

[brendanx67]

Close, but I would prefer a little code clean-up to make it clearer that the standard pattern is being employed. Put the MessageBoxHelper in the NextPage() function and use it the set a private double? _cutoffScore.