Conversation
A new test method named `DefaultIfNull_WithSingleValueChange_ShouldEmitOnce` was added to the `ObservableExtensionsTests` class in the `ObservableExtensionsTests.cs` file. This test sets up a `TestScheduler` and a `ReactivePropertySlim<string?>`, creates a hot observable with a series of recorded notifications, subscribes to update the reactive property, starts the scheduler, applies the `DefaultIfNull` extension method, and asserts that the actual messages match the expected ones using `ReactiveAssert.AreElementsEqual`.
Refactor AlignmentEicModel.cs to use a hot observable (spotChromatogramsHot) created with the Publish method. Update various observables and model instances to derive from spotChromatogramsHot instead of spotChromatograms. Add spotChromatogramsHot.Connect() to the Disposables collection for proper resource management.
Converted `_containerOx` to a hot observable using `Publish` and connected it in the `MatchResultCandidatesModel` constructor. Updated `Candidates` property to use `hotContainerOx` and initialize with an empty list as a default value. Added `using` directive for `Reactive.Bindings.Extensions`. Adjusted namespace and class declaration formatting for better readability. Made minor formatting changes to enhance code clarity.
The RefreshUI method calls have been removed for the originalChromUC, alignedChromUC, and pickingUC objects. These calls were previously used to refresh the UI for these instances but are no longer necessary.
Refactored code to use file-scoped namespaces instead of block-scoped namespaces. Updated `namespace` declaration from `namespace CompMs.App.Msdial.Model.DataObj { ... }` to `namespace CompMs.App.Msdial.Model.DataObj;`. Reformatted `AlignedChromatograms` class to align with the new namespace declaration.
Ensure the observable replays the last item to new subscribers and maintains a single subscription to the underlying sequence.
Introduce FileProp class to encapsulate properties related to AnalysisFileBean, including observable SolidColorBrush and optional RetentionIndexHandler. Update LoadPeakProperty method to use FileProp instances, creating a dictionary cls2brsh for class-to-brush mapping and a list fileProps for structured data management. Replace direct handling of brushes and handlers with FileProp properties for improved code structure and readability.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors reactive streams and improves performance in various parts of the Result View while cleaning up legacy code. Key changes include:
- Introducing “hot” observables with Replay/RefCount for caching and shared subscriptions.
- Refactoring models and UI view components to improve consistency and performance.
- Adding unit tests to validate reactive property behavior.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/MSDIAL5/MsdialGuiAppTests/Utility/ObservableExtensionsTests.cs | Added a new test to verify the DefaultIfNull operator emits correctly. |
| src/MSDIAL5/MsdialGuiApp/ViewModel/PeakCuration/AlignedChromatogramModificationViewModelLegacy.cs | Removed RefreshUI calls after constructing UI controls. |
| src/MSDIAL5/MsdialGuiApp/View/PeakCuration/AlignedPeakCorrectionWinLegacy.xaml.cs | Refactored file property handling and encapsulated properties in a new FileProp class. |
| src/MSDIAL5/MsdialGuiApp/Model/Loader/AlignmentEicLoader.cs | Updated reactive stream handling with Replay(1).RefCount(). |
| src/MSDIAL5/MsdialGuiApp/Model/Information/MatchResultCandidatesModel.cs | Refactored the use of container observables for hot subscription support. |
| src/MSDIAL5/MsdialGuiApp/Model/DataObj/AlignedChromatograms.cs | Migrated to file-scoped namespace for consistency. |
| src/MSDIAL5/MsdialGuiApp/Model/Chart/AlignmentEicModel.cs | Updated reactive handling to use a hot observable and adjusted observable subscriptions. |
Comments suppressed due to low confidence (2)
src/MSDIAL5/MsdialGuiApp/ViewModel/PeakCuration/AlignedChromatogramModificationViewModelLegacy.cs:59
- The RefreshUI() calls have been removed from UI control initialization. Please verify that the new reactive stream setup correctly triggers UI updates to avoid potential regressions in user interface refresh behavior.
originalChromUC.RefreshUI();
src/MSDIAL5/MsdialGuiApp/View/PeakCuration/AlignedPeakCorrectionWinLegacy.xaml.cs:70
- [nitpick] The variable name 'cls2brsh' is not very descriptive. Consider renaming it to 'classToBrushObservable' or a similarly clear name to improve code readability.
var cls2brsh = filePropertiesModel.ClassProperties.ToDictionary(
Bujee415
approved these changes
Apr 24, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR Classification
Code cleanup, performance optimization, and bug fixes.
PR Summary
This PR refactors reactive streams for improved performance, enhances code maintainability, and fixes potential null handling issues while adding test coverage for reliability.
spotChromatogramsHot,hotContainerOx) and added.Replay(1).RefCount()for shared subscriptions and caching.spotChromatogramswithspotChromatogramsHotfor consistency and performance.FilePropclass for better organization.DefaultIfNull_WithSingleValueChange_ShouldEmitOnceto validate reactive property behavior.