Skyline: Convert WebClient use to HttpClient #3636
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
brendanx67
commented
Oct 10, 2025
brendanx67
commented
- WebClient is based on old technology and is deprecated
- A new wrapper class HttpClientWithProgress is introduced to support the IProgressMonitor interface
added 2 commits
October 10, 2025 21:02
- WebClient is based on old technology and is deprecated - A new wrapper class HttpClientWithProgress is introduced to support the IProgressMonitor interface
- Moved all user messages from CommonUtil/Properties/Resources.resx to CommonResources/MessageResources.resx so they can be accessed in tests (Resources.resx conflicts with Common/Properties/Resources.resx)
nickshulman
reviewed
Oct 12, 2025
| { | ||
| if (_progressMonitor is SilentProgressMonitor spm) | ||
| return spm.CancellationToken; | ||
| // IProgressMonitor doesn't expose CancellationToken directly; use None and rely on IsCanceled checks |
There was a problem hiding this comment.
There are some places where we have to use an IProgressMonitor but we also want a CancellationToken, we explicitly pass in both the IProgressMonitor and a CancellationToken like in KoinaModel.PredictBatches
| if (IsCanceled && null != x) | ||
| // Treat any OperationCanceledException as cancellation, regardless of whether | ||
| // the user clicked Cancel or the operation itself threw the exception | ||
| if (null != x && (x is OperationCanceledException || x.InnerException is OperationCanceledException)) |
There was a problem hiding this comment.
I think it would be better to keep this the way it was where the user will see "The operation was cancelled" if they did not actually press the cancel button.
Otherwise, I worry that there may be some weird bugs where the user sees no error message and the thing just fails.
If there's a case where we want it to behave like this I think we should put a catch block in the specify performWork method which cancels the progressMonitor if it caught an OperationCanceledException.
Do you know of a scenario where an OperationCanceledException is getting thrown by someone else that we want to treat as the user canceled?
- Much more manual testing with disconnecting WiFi at various key moments
…k/20251010_webclient_replacement
…'s review) - but improve test handling of this case to avoid a special test string that our translators translate and is not common practice in production code
…lationToken for HttpClient functions that support cancellation. (review comment by Nick) - introduced a new interface IProgressMonitorWithCancellationToken to avoid needing to pass around a CancellationToken separately
…rehensive baseline coverage
- moved RInstallerTest to using new HttpClientWithProgress test support - moved TestAsynchronousDownloadClient to PythonInstallerLegacyDlgTest which is now the only class to use it - Added notes about DRY coding to STYLEGUIDE.md since LLMs tend to be repetitive
- implemented LongWaitDlg.CancelClickedTestException as a quick way to simulate a Cancel button click during testing
…ds with new tools developed during unit testing of HttpClientWithProgress, rather than short-circuiting much higher with testing implementations of interfaces TestDdaSearchMsFraggerBadFasta TestRInstaller TestStartPageShowPathChooser TestToolStore TestToolUpdates This covers the cases I was testing manually by turning my WiFi off and clicking the Cancel button
…k/20251010_webclient_replacement
- improve message testing for both ToolUpdatesDlg and HttpClientWithProgress
…ize them into just HttpClientTestHelper
…ive organization - Create todos/ directory with README.md documenting lifecycle - Move TODO-20251010_webclient_replacement.md to todos/completed/ (ready for merge) - Move all backlog TODOs to todos/backlog/: - TODO-panorama_webclient_replacement.md - TODO-tools_webclient_replacement.md - TODO-remove_async_and_await.md - TODO-utf8_no_bom.md - Create TODO-skyp_webclient_replacement.md for last WebClient in core Skyline.exe - Add .gitkeep files for active/ and archive/ directories - Update WORKFLOW.md with complete directory-based TODO lifecycle - Add completion summary to TODO-20251010_webclient_replacement.md This structure enables: - Better organization and team visibility - Git-tracked documentation of completed work - Seamless LLM context switching - Clean workspace with preserved history
- analyze-bom-git.ps1: Scans Git-tracked files for UTF-8 BOM presence - Reports statistics and creates files-with-bom.txt catalog - Fast Git-based scanning (vs full directory tree) - Created during webclient_replacement when LLMs removed BOMs - fix-crlf.ps1: Converts modified Git files from LF to CRLF - Scopes to 'git status' working tree only - Verifies conversion completed successfully - Addresses LLM preference for Linux-style LF line endings Both scripts were created Oct 2025 during LLM-assisted development to handle text encoding consistency issues. See STYLEGUIDE.md and todos/backlog/TODO-utf8_no_bom.md for project standards.
…com:ProteoWizard/pwiz into Skyline/work/20251010_webclient_replacement
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.