Skip to content

Fixed Unnecessary Chromatogram Loading and Improved Reactive Stream Handling in Result View#541

Merged
Bujee415 merged 9 commits intomasterfrom
improve/aligned-eic-useless-loadings
Apr 24, 2025
Merged

Fixed Unnecessary Chromatogram Loading and Improved Reactive Stream Handling in Result View#541
Bujee415 merged 9 commits intomasterfrom
improve/aligned-eic-useless-loadings

Conversation

@YukiMatsuzawa
Copy link
Copy Markdown
Contributor

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.

  • Reactive Streams: Introduced "hot" observables (spotChromatogramsHot, hotContainerOx) and added .Replay(1).RefCount() for shared subscriptions and caching.
  • AlignmentEicModel.cs: Replaced spotChromatograms with spotChromatogramsHot for consistency and performance.
  • MatchResultCandidatesModel.cs: Refactored constructor to use "hot" observables and optimized property initialization.
  • AlignedPeakCorrectionWinLegacy.xaml.cs: Encapsulated file properties into a new FileProp class for better organization.
  • ObservableExtensionsTests.cs: Added unit test DefaultIfNull_WithSingleValueChange_ShouldEmitOnce to validate reactive property behavior.

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.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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(

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Bujee415 Bujee415 merged commit 18b95cb into master Apr 24, 2025
9 checks passed
@Bujee415 Bujee415 deleted the improve/aligned-eic-useless-loadings branch April 24, 2025 02:37
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.

3 participants