From a37a532b7e83c68dfe14a0ffc9d5b6b340e7e96f Mon Sep 17 00:00:00 2001 From: Brian Pratt Date: Mon, 16 Oct 2023 13:44:42 -0700 Subject: [PATCH 1/3] Add validation for cutoff score in ImportPeptideSearchDlg --- .../BuildPeptideSearchLibraryControl.cs | 23 +++++++++++++++++++ .../PeptideSearch/ImportPeptideSearchDlg.cs | 5 ++++ .../Skyline/Properties/Resources.Designer.cs | 9 ++++++++ pwiz_tools/Skyline/Properties/Resources.resx | 3 +++ .../Skyline/TestFunctional/DdaSearchTest.cs | 10 ++++++-- 5 files changed, 48 insertions(+), 2 deletions(-) diff --git a/pwiz_tools/Skyline/FileUI/PeptideSearch/BuildPeptideSearchLibraryControl.cs b/pwiz_tools/Skyline/FileUI/PeptideSearch/BuildPeptideSearchLibraryControl.cs index 0c9e71d55f..1c6e5db16a 100644 --- a/pwiz_tools/Skyline/FileUI/PeptideSearch/BuildPeptideSearchLibraryControl.cs +++ b/pwiz_tools/Skyline/FileUI/PeptideSearch/BuildPeptideSearchLibraryControl.cs @@ -288,6 +288,19 @@ public double? CutOffScore set { textCutoff.Text = value.HasValue ? value.Value.ToString(CultureInfo.CurrentCulture) : string.Empty; } } + public bool ValidateCutoffScoreText() + { + try + { + var test = CutOffScore; + return true; + } + catch (FormatException) + { + return false; + } + } + public bool IncludeAmbiguousMatches { get { return cbIncludeAmbiguousMatches.Checked; } @@ -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) + { + textCutoff.Text = cutoffControlText; + } + + #endregion } } diff --git a/pwiz_tools/Skyline/FileUI/PeptideSearch/ImportPeptideSearchDlg.cs b/pwiz_tools/Skyline/FileUI/PeptideSearch/ImportPeptideSearchDlg.cs index 016ae5432c..e2565d2232 100644 --- a/pwiz_tools/Skyline/FileUI/PeptideSearch/ImportPeptideSearchDlg.cs +++ b/pwiz_tools/Skyline/FileUI/PeptideSearch/ImportPeptideSearchDlg.cs @@ -485,6 +485,11 @@ private void NextPage() { _pagesToSkip.Clear(); + if (!BuildPepSearchLibControl.ValidateCutoffScoreText()) + { + MessageDlg.Show(this, Resources.ImportPeptideSearchDlg_NextPage_Invalid_cutoff_score); + return; + } ImportPeptideSearch.IsDDASearch = BuildPepSearchLibControl.PerformDDASearch; ImportFastaControl.IsDDASearch = BuildPepSearchLibControl.PerformDDASearch; if (!BuildPepSearchLibControl.UseExistingLibrary) diff --git a/pwiz_tools/Skyline/Properties/Resources.Designer.cs b/pwiz_tools/Skyline/Properties/Resources.Designer.cs index 4671f1a4ee..78a418c60d 100644 --- a/pwiz_tools/Skyline/Properties/Resources.Designer.cs +++ b/pwiz_tools/Skyline/Properties/Resources.Designer.cs @@ -16745,6 +16745,15 @@ public static string ImportPeptideSearchDlg_NextPage_Import_FASTA__required_ { } } + /// + /// Looks up a localized string similar to Invalid cutoff score. + /// + public static string ImportPeptideSearchDlg_NextPage_Invalid_cutoff_score { + get { + return ResourceManager.GetString("ImportPeptideSearchDlg_NextPage_Invalid_cutoff_score", resourceCulture); + } + } + /// /// Looks up a localized string similar to No isolation windows are configured, but are required for deconvoluting DIA raw files by DIA-Umpire in preparation for DDA search. Go back to full scan settings to configure the isolation scheme.. /// diff --git a/pwiz_tools/Skyline/Properties/Resources.resx b/pwiz_tools/Skyline/Properties/Resources.resx index 6625aac8cb..2a60794253 100644 --- a/pwiz_tools/Skyline/Properties/Resources.resx +++ b/pwiz_tools/Skyline/Properties/Resources.resx @@ -13911,4 +13911,7 @@ If you choose Disable, you can enable Auto-select later with the "Refine > Ad Error: Failure attempting to save mProphet features file {0}. + + Invalid cutoff score + \ No newline at end of file diff --git a/pwiz_tools/Skyline/TestFunctional/DdaSearchTest.cs b/pwiz_tools/Skyline/TestFunctional/DdaSearchTest.cs index f55f7bd97f..a455f4fdfc 100644 --- a/pwiz_tools/Skyline/TestFunctional/DdaSearchTest.cs +++ b/pwiz_tools/Skyline/TestFunctional/DdaSearchTest.cs @@ -230,11 +230,17 @@ private void TestSearch() importPeptideSearchDlg.BuildPepSearchLibControl.PerformDDASearch = true; importPeptideSearchDlg.BuildPepSearchLibControl.DdaSearchDataSources = SearchFiles.Select(o => (MsDataFileUri)new MsDataFilePath(o)).Take(1).ToArray(); importPeptideSearchDlg.BuildPepSearchLibControl.WorkflowType = ImportPeptideSearchDlg.Workflow.dia; // will go back and switch to DDA - importPeptideSearchDlg.BuildPepSearchLibControl.CutOffScore = 0.9; importPeptideSearchDlg.BuildPepSearchLibControl.IrtStandards = IrtStandard.AUTO; - Assert.IsTrue(importPeptideSearchDlg.ClickNextButton()); + importPeptideSearchDlg.BuildPepSearchLibControl.SetCutoffControlText(@"12%"); // Intentionally bad }); + var errMsgDlg = ShowDialog(() => importPeptideSearchDlg.ClickNextButton()); + OkDialog(errMsgDlg, errMsgDlg.OkDialog); + RunUI(() => + { + importPeptideSearchDlg.BuildPepSearchLibControl.CutOffScore = 0.9; + Assert.IsTrue(importPeptideSearchDlg.ClickNextButton()); + }); // With only 1 source, no add/remove prefix/suffix dialog // We're on the "Match Modifications" page. Add M+16 mod. From 60d27c77d3bf00547b995cb52ded890db4072dfc Mon Sep 17 00:00:00 2001 From: Brian Pratt Date: Tue, 17 Oct 2023 11:52:40 -0700 Subject: [PATCH 2/3] use our standard decimal textbox validation code --- .../BuildPeptideSearchLibraryControl.cs | 32 +++++++------------ .../PeptideSearch/ImportPeptideSearchDlg.cs | 3 +- .../Skyline/Properties/Resources.Designer.cs | 9 ------ pwiz_tools/Skyline/Properties/Resources.resx | 3 -- .../Skyline/TestFunctional/DdaSearchTest.cs | 9 +++--- 5 files changed, 18 insertions(+), 38 deletions(-) diff --git a/pwiz_tools/Skyline/FileUI/PeptideSearch/BuildPeptideSearchLibraryControl.cs b/pwiz_tools/Skyline/FileUI/PeptideSearch/BuildPeptideSearchLibraryControl.cs index 1c6e5db16a..be607606a1 100644 --- a/pwiz_tools/Skyline/FileUI/PeptideSearch/BuildPeptideSearchLibraryControl.cs +++ b/pwiz_tools/Skyline/FileUI/PeptideSearch/BuildPeptideSearchLibraryControl.cs @@ -281,24 +281,25 @@ public void AddSearchFiles(IEnumerable fileNames) SearchFilenames = BuildLibraryDlg.AddInputFiles(WizardForm, SearchFilenames, fileNames, PerformDDASearch); } + // For test purposes, lets us test error handling + public string CutOffScoreText + { + set { textCutoff.Text = value; } // Can be a null or empty string under some circumstances + } + + public bool NeedsCutoffScore => comboInputFileType.SelectedIndex > 0; + public double? CutOffScore { // Only valid when Skyline performs the search - get { return comboInputFileType.SelectedIndex > 0 ? double.Parse(textCutoff.Text) : (double?) null; } + get { return NeedsCutoffScore ? double.Parse(textCutoff.Text) : (double?) null; } set { textCutoff.Text = value.HasValue ? value.Value.ToString(CultureInfo.CurrentCulture) : string.Empty; } } - public bool ValidateCutoffScoreText() + public bool ValidateCutoffScore() { - try - { - var test = CutOffScore; - return true; - } - catch (FormatException) - { - return false; - } + var helper = new MessageBoxHelper(this.ParentForm); + return !NeedsCutoffScore || helper.ValidateDecimalTextBox(textCutoff, out _); } public bool IncludeAmbiguousMatches @@ -657,14 +658,5 @@ 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) - { - textCutoff.Text = cutoffControlText; - } - - #endregion } } diff --git a/pwiz_tools/Skyline/FileUI/PeptideSearch/ImportPeptideSearchDlg.cs b/pwiz_tools/Skyline/FileUI/PeptideSearch/ImportPeptideSearchDlg.cs index e2565d2232..2a48854399 100644 --- a/pwiz_tools/Skyline/FileUI/PeptideSearch/ImportPeptideSearchDlg.cs +++ b/pwiz_tools/Skyline/FileUI/PeptideSearch/ImportPeptideSearchDlg.cs @@ -485,9 +485,8 @@ private void NextPage() { _pagesToSkip.Clear(); - if (!BuildPepSearchLibControl.ValidateCutoffScoreText()) + if (!BuildPepSearchLibControl.ValidateCutoffScore()) { - MessageDlg.Show(this, Resources.ImportPeptideSearchDlg_NextPage_Invalid_cutoff_score); return; } ImportPeptideSearch.IsDDASearch = BuildPepSearchLibControl.PerformDDASearch; diff --git a/pwiz_tools/Skyline/Properties/Resources.Designer.cs b/pwiz_tools/Skyline/Properties/Resources.Designer.cs index 78a418c60d..4671f1a4ee 100644 --- a/pwiz_tools/Skyline/Properties/Resources.Designer.cs +++ b/pwiz_tools/Skyline/Properties/Resources.Designer.cs @@ -16745,15 +16745,6 @@ public static string ImportPeptideSearchDlg_NextPage_Import_FASTA__required_ { } } - /// - /// Looks up a localized string similar to Invalid cutoff score. - /// - public static string ImportPeptideSearchDlg_NextPage_Invalid_cutoff_score { - get { - return ResourceManager.GetString("ImportPeptideSearchDlg_NextPage_Invalid_cutoff_score", resourceCulture); - } - } - /// /// Looks up a localized string similar to No isolation windows are configured, but are required for deconvoluting DIA raw files by DIA-Umpire in preparation for DDA search. Go back to full scan settings to configure the isolation scheme.. /// diff --git a/pwiz_tools/Skyline/Properties/Resources.resx b/pwiz_tools/Skyline/Properties/Resources.resx index 2a60794253..6625aac8cb 100644 --- a/pwiz_tools/Skyline/Properties/Resources.resx +++ b/pwiz_tools/Skyline/Properties/Resources.resx @@ -13911,7 +13911,4 @@ If you choose Disable, you can enable Auto-select later with the "Refine > Ad Error: Failure attempting to save mProphet features file {0}. - - Invalid cutoff score - \ No newline at end of file diff --git a/pwiz_tools/Skyline/TestFunctional/DdaSearchTest.cs b/pwiz_tools/Skyline/TestFunctional/DdaSearchTest.cs index a455f4fdfc..caf2b86f99 100644 --- a/pwiz_tools/Skyline/TestFunctional/DdaSearchTest.cs +++ b/pwiz_tools/Skyline/TestFunctional/DdaSearchTest.cs @@ -224,20 +224,21 @@ private void TestSearch() // Add the test xml file to the search files list and try to // build the document library. - RunUI(() => + var errMsgDlg = ShowDialog(() => { Assert.IsTrue(importPeptideSearchDlg.CurrentPage == ImportPeptideSearchDlg.Pages.spectra_page); importPeptideSearchDlg.BuildPepSearchLibControl.PerformDDASearch = true; importPeptideSearchDlg.BuildPepSearchLibControl.DdaSearchDataSources = SearchFiles.Select(o => (MsDataFileUri)new MsDataFilePath(o)).Take(1).ToArray(); importPeptideSearchDlg.BuildPepSearchLibControl.WorkflowType = ImportPeptideSearchDlg.Workflow.dia; // will go back and switch to DDA + importPeptideSearchDlg.BuildPepSearchLibControl.CutOffScoreText = @"12%"; // Intentionally bad importPeptideSearchDlg.BuildPepSearchLibControl.IrtStandards = IrtStandard.AUTO; - importPeptideSearchDlg.BuildPepSearchLibControl.SetCutoffControlText(@"12%"); // Intentionally bad + Assert.IsTrue(importPeptideSearchDlg.ClickNextButton()); }); - var errMsgDlg = ShowDialog(() => importPeptideSearchDlg.ClickNextButton()); - OkDialog(errMsgDlg, errMsgDlg.OkDialog); + OkDialog(errMsgDlg, errMsgDlg.OkDialog); // Expect complaint about bad cutoff score RunUI(() => { + Assert.IsTrue(importPeptideSearchDlg.CurrentPage == ImportPeptideSearchDlg.Pages.spectra_page); // Should not have advanced importPeptideSearchDlg.BuildPepSearchLibControl.CutOffScore = 0.9; Assert.IsTrue(importPeptideSearchDlg.ClickNextButton()); }); From 5fdd3936d024cd58c12a24a8e9acafaf9265be7a Mon Sep 17 00:00:00 2001 From: Brian Pratt Date: Thu, 19 Oct 2023 12:36:21 -0700 Subject: [PATCH 3/3] use a backing variable for CutOffScore --- .../BuildPeptideSearchLibraryControl.cs | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/pwiz_tools/Skyline/FileUI/PeptideSearch/BuildPeptideSearchLibraryControl.cs b/pwiz_tools/Skyline/FileUI/PeptideSearch/BuildPeptideSearchLibraryControl.cs index be607606a1..a7f777c4c6 100644 --- a/pwiz_tools/Skyline/FileUI/PeptideSearch/BuildPeptideSearchLibraryControl.cs +++ b/pwiz_tools/Skyline/FileUI/PeptideSearch/BuildPeptideSearchLibraryControl.cs @@ -287,19 +287,34 @@ public string CutOffScoreText set { textCutoff.Text = value; } // Can be a null or empty string under some circumstances } - public bool NeedsCutoffScore => comboInputFileType.SelectedIndex > 0; + public bool NeedsCutoffScore => comboInputFileType.SelectedIndex > 0; // Only needed if Skyline is conducting the search + + private double? _cutoffScore; // May be null when Skyline is not doing the search public double? CutOffScore { // Only valid when Skyline performs the search - get { return NeedsCutoffScore ? double.Parse(textCutoff.Text) : (double?) null; } - set { textCutoff.Text = value.HasValue ? value.Value.ToString(CultureInfo.CurrentCulture) : string.Empty; } + get { return NeedsCutoffScore ? _cutoffScore : null; } + set + { + _cutoffScore = value; + textCutoff.Text = _cutoffScore.HasValue ? _cutoffScore.Value.ToString(CultureInfo.CurrentCulture) : string.Empty; + } } public bool ValidateCutoffScore() { + if (!NeedsCutoffScore) + { + return true; // Doesn't matter what's in the text box, we won't use it + } var helper = new MessageBoxHelper(this.ParentForm); - return !NeedsCutoffScore || helper.ValidateDecimalTextBox(textCutoff, out _); + if (helper.ValidateDecimalTextBox(textCutoff, out var cutoffScore)) + { + _cutoffScore = cutoffScore; + return true; + } + return false; } public bool IncludeAmbiguousMatches