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.

Sign up for GitHub

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

Jump to bottom

* changed Run Peptide Search to keep wizard form close button visible #3337

Merged
merged 3 commits into from
Jan 21, 2025
Merged

* changed Run Peptide Search to keep wizard form close button visible #3337

merged 3 commits into from
Jan 21, 2025

Conversation

chambm
Copy link
Member

@chambm chambm commented Jan 16, 2025 •
edited
Loading

  • changed Run Peptide Search to keep wizard form close button visible, but intercept it while search is running and display an explanation message to click cancel search button

image

@chambm chambm requested a review from brendanx67 January 16, 2025 16:59
@chambm
Copy link
Member Author

chambm commented Jan 16, 2025

I thought about avoiding the as WizardPageControl complexity by making ALL the controls (for each TabPage) inherit from WizardPageControl instead of UserControl, but some of those controls are used elsewhere (FullScanSettomgsControl and TransitionsEttingsControl) so that seemed unwise. I could have wrapped those controls in a WizardPageControl, but that seemed more complex than the WizardPageControl cast. But perhaps if more logic is added into WizardPageControl that applies to all the controls, then that latter approach would make sense.

@chambm
* changed Run Peptide Search to keep wizard form close button visible…
f527cae 
…, but intercept it while search is running and display an explanation message to click cancel search button
@chambm chambm force-pushed the Skyline/work/20250116_dont_disable_ControlBox_in_SearchControl branch from f01df0b to f527cae Compare January 16, 2025 17:08
brendanx67
brendanx67 requested changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@brendanx67 brendanx67 left a comment

Choose a reason for hiding this comment

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

A bit of feedback and at least one change requested.

pwiz_tools/Skyline/FileUI/PeptideSearch/ImportPeptideSearchDlg.cs Outdated
@@ -1486,6 +1491,13 @@ public void CloseWizard(DialogResult result)

protected override void OnFormClosing(FormClosingEventArgs e)
{
// Ask current WizardPageControl if wizard is in a good state to close
if ((GetPageControl(wizardPagesImportPeptideSearch.SelectedTab) as WizardPageControl)?.CanWizardClose() == false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please encapsulate this logic in a function. Looks ugly to me in the if clause.

pwiz_tools/Skyline/FileUI/PeptideSearch/PeptideSearchResources.resx Outdated Show resolved Hide resolved
pwiz_tools/Skyline/FileUI/PeptideSearch/ImportPeptideSearchDlg.cs Outdated Show resolved Hide resolved
pwiz_tools/Skyline/FileUI/PeptideSearch/SearchControl.cs Outdated Show resolved Hide resolved
pwiz_tools/Skyline/Controls/WizardPages.cs Show resolved Hide resolved
@chambm
* changed confirmation dialog to yes/no with yes answer cancelling th…
3fd254d 
…e search and closing the form when the search cancellation finishes

* added test for the "no" branch to TestDdaSearch
@chambm chambm requested a review from brendanx67 January 16, 2025 21:25
@chambm
Copy link
Member Author

chambm commented Jan 16, 2025

Updated screenshot and addressed the review points.

brendanx67
brendanx67 approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@brendanx67 brendanx67 left a comment

Choose a reason for hiding this comment

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

One comment where I am not even positive "== false" is correct, except that you seem to be testing it. Easy to change to what I am suggesting. So, approving these changes.

pwiz_tools/Skyline/FileUI/PeptideSearch/ImportPeptideSearchDlg.cs Outdated
private bool CanWizardClose()
{
var wizardPageControl = GetPageControl(wizardPagesImportPeptideSearch.SelectedTab) as WizardPageControl;
return wizardPageControl?.CanWizardClose() == false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this return statement confusing and prefer "?? true" to express the default case when wizardPageControl is null.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I forgot to invert the logic when I made it a function. As written, it's really functioning as a "CannotCloseWizard()" function. I'll invert here and where it's used because CanWizardClose() is more natural.

@chambm chambm merged commit 0de1783 into master Jan 21, 2025
13 checks passed
@chambm chambm deleted the Skyline/work/20250116_dont_disable_ControlBox_in_SearchControl branch January 21, 2025 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickshulman nickshulman nickshulman left review comments

@brendanx67 brendanx67 brendanx67 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chambm @nickshulman @brendanx67