Skyline: Remove BuildLibraryNotification and switch to LongWaitDlg #3350
Conversation
brendanx67
commented
Jan 28, 2025
brendanx67
commented
- All other building operations now use LongWaitDlg, e.g. background proteome, etc.
- Maintaining this non-blocking build of spectral libraries has been a pain
- And it resulted in 3 paths (Peptide Settings, Import Peptide Search wizard, and command-line) that differed and only Peptide Settings, which was the most complicated, being spread between PeptideSettingsUI, BiblioSpecLiteBuilder, SkylineWindow, and BuildLibraryNotification really worked as expected.
- Stop using ModifyDocument during library build completion. Change the RT combo box selection instead.
- Tests have been improved to test more thoroughly
- All other building operations now use LongWaitDlg, e.g. background proteome, etc. - Maintaining this non-blocking build of spectral libraries has been a pain - And it resulted in 3 paths (Peptide Settings, Import Peptide Search wizard, and command-line) that differed and only Peptide Settings, which was the most complicated, being spread between PeptideSettingsUI, BiblioSpecLiteBuilder, SkylineWindow, and BuildLibraryNotification really worked as expected. - Tests have been improved to test more thoroughly
… to PeptideSettingsUI - Stop using ModifyDocument during library build completion. Change the RT combo box selection instead.
| return; | ||
| } | ||
| // A failure has occurred | ||
| // CONSIDER: Should this ever cause the wizard to be closed and force the user to start over? |
There was a problem hiding this comment.
I don't understand the "Should this ever cause the wizard to be closed" part of this comment. It looks like it always causes the wizard to be closed. Is there a typo?
There was a problem hiding this comment.
Think of CONSIDER as not quite as forceful as a TODO. This code maybe should be changed, but I am leaving it as it was when I found it. I am not sure why we want to close the wizard. That may always be something a user would prefer we not do. But this code existed this way before. I am not changing the CloseWizard part. Just saying it should be reconsidered in the future.
| if (status.IsCanceled) | ||
| lib = null; | ||
| if (status.IsError) | ||
| throw status.ErrorException; |
There was a problem hiding this comment.
I don't think there is anyone on the callstack who is going to catch this exception. This is going to end up being an unhandled exception.
This should probably be:
MessageDlg.ShowException(status.ErrorException);
If you were going to throw the exception, then I think it should be:
Helpers.WrapAndThrowException(status.ErrorException);
so that the callstack is preserved.
There was a problem hiding this comment.
Funny. This pattern with LongWaitDlg was repeated 4 more times in the function below and then some extra exception handling for the last 2 times that added a more specific message. I think I cleaned everything up with 2 new local utility functions. Please have a look at that. It does make me wonder how all of this was handled inside LibraryBuildNotification, and it gives me the strong feeling that the error handling here is never tested, since nothing failed when there would have been unhandled exceptions in the error cases.
brendanx67
deleted the
Skyline/work/20250124_remove_build_lib_notification
branch
