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

Fix TestMethodEditTutorial #3395

Merged
merged 2 commits into from
Mar 12, 2025

Conversation

nickshulman
Copy link
Contributor

PR #3393 broke TestMethodEditTutorial because the wrong spectrum ends up being selected when the selected Precursor in the Targets tree changes to something that was found in a different library.

PR #3393 broke TestMethodEditTutorial because the wrong spectrum ends up being selected when the selected Precursor in the Targets tree changes to something that was found in a different library.
@nickshulman nickshulman requested a review from brendanx67 March 12, 2025 11:43
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.

This is too simplistic and breaks other functionality. It seems like we are going to need to store whether the selections in these combo boxes were set by the user and persist them when they are and clear them when they are not.

@nickshulman
Copy link
Contributor Author

It sounds like the way to fix the unit test while keeping your original fix in place is to give this thing a more robust way of deciding whether the currently selected spectrum is still in the list of choices.
TestMethodEditTutorial fails because it goes from selecting a precursor in the Targets tree with one spectrum option "Yeast (GPM)" and then selects a different precursor in the Targets tree where the two choices are "Yeast (Atlas)" and "Yeast (GPM)" and the spectrum that should be selected is "Yeast (Atlas)". If Skyline knew that that "Yeast (GPM)" was a completely different spectrum from the previous "Yeast (GPM)" then the "comboSpectrum.Items.IndexOf(selectedSpectrum) == -1" would cause it to do the right thing.

@brendanx67
Copy link
Contributor

I am going to try my idea of whether the user explicitly selected the selected value in the combo or not.

…ugh changes in Targets selection

- the new code is better because it remembers what the user selected instead of what was selected before a toolbar update which can already have lost what the user selected if the selection goes through a precursor with no match for any user selected library
- tested this extensively with the GroupStudies1 tutorial and a Koina generated library
@brendanx67 brendanx67 dismissed their stale review March 12, 2025 18:32

The current PR adresses my concerns

@nickshulman
Copy link
Contributor Author

Way back in 2010, the way that this worked was that Skyline knew that if the selectedIndex was non-zero it meant the user had explicitly chosen a spectrum:

// Unless the current selected index is the one matching the one currently
// in use by the precursor (zero), then try to stay viewing the in-use spectrum (zero)
int selectedIndex = comboSpectrum.SelectedIndex;

I bet this got broken in 2022 when code was added to set "selectedSpectrumIndex" to -1:
aa8be89

@nickshulman nickshulman merged commit 3c95dfb into master Mar 12, 2025
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants