Enable automatic graph updates when values change#655
Conversation
…pertychanged # Conflicts: # src/Common/ChartDrawing/Chart/ScatterControlSlim.cs
Modified the registration of the `PointBrush` dependency property to include `FrameworkPropertyMetadataOptions.AffectsRender`. This change ensures that updates to the `PointBrush` will trigger a re-render of the control.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for property change notification tracking in chart controls, enabling reactive updates when nested properties change. The main enhancement allows chart controls to automatically update visualizations when properties on bound data objects implement INotifyPropertyChanged.
Key Changes:
- Implemented a comprehensive property accessor system with support for typed and dynamic property access
- Added
ValueHolderandNotifiableDataPointclasses to track nested property changes - Enhanced
ScatterControlSlimandAnnotatorto respond to property changes in data items - Added extensive unit tests for the new expression helper functionality
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Common/ChartDrawing/Helper/ExpressionHelper.cs | Added new methods for generating property getter expressions with extensive documentation |
| src/Common/ChartDrawing/Helper/PropertiesAccessor.cs | Factory for creating appropriate property accessor implementations |
| src/Common/ChartDrawing/Helper/TypedPropertiesAccessor.cs | Typed property accessor using compiled expressions |
| src/Common/ChartDrawing/Helper/DynamicPropertiesAccessor.cs | Dynamic property accessor for runtime type resolution |
| src/Common/ChartDrawing/Helper/IdentityAccessor.cs | Identity accessor for empty property strings |
| src/Common/ChartDrawing/Helper/IgnoreAccessor.cs | Null accessor for null property strings |
| src/Common/ChartDrawing/Helper/IPropertiesAccessor.cs | Common interface for property accessors |
| src/Common/ChartDrawing/Data/ValueHolder.cs | Tracks nested property changes via INotifyPropertyChanged |
| src/Common/ChartDrawing/Data/NotifiableDataPoint.cs | Wraps data items with change notifications for chart coordinates |
| src/Common/ChartDrawing/Chart/ScatterControlSlim.cs | Refactored to use new property accessor system with change tracking |
| src/Common/ChartDrawing/Chart/Annotator.cs | Updated to use property accessors and support reactive label updates |
| tests/Common/ChartDrawingTests/Helper/ExpressionHelperTests.cs | Comprehensive tests for expression helper methods |
| tests/Common/ChartDrawingTests/Data/ValueHolderTests.cs | Tests for ValueHolder property change tracking |
| tests/Common/ChartDrawingUiTest/Chart/ScatterControlSlimTest2.xaml | New UI test for scatter control with typed/typeless variants |
| tests/Common/ChartDrawingUiTest/Chart/ScatterControlSlimNotifyTest.xaml | UI test for property change notifications |
| tests/Common/ChartDrawingUiTest/Chart/AnnotatorNotifyTest.xaml | UI test for annotator change notifications |
| src/MSDIAL5/MsdialGuiApp/View/Search/PeakSpotNavigatorView.xaml | Fixed ComboBox binding to use ComboBoxItem with DataContext |
| src/MSDIAL5/MsdialGuiApp/Model/Gcms/GcmsAnalysisModel.cs | Updated to handle nullable annotation labels |
| tests/Common/ChartDrawingTests/ChartDrawingTests.csproj | Simplified project references format |
| tests/Common/ChartDrawingUiTest/ChartDrawingUiTest.csproj | Added UseWPF property and changed LangVersion to latest |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using System.Runtime.CompilerServices; | ||
|
|
There was a problem hiding this comment.
The System.Runtime.CompilerServices namespace is imported but not used in this file. This import should be removed to keep the code clean.
| using System.Runtime.CompilerServices; |
| <TargetFrameworkProfile /> | ||
| <GenerateAssemblyInfo>false</GenerateAssemblyInfo> | ||
| <LangVersion>12</LangVersion> | ||
| <LangVersion>latest</LangVersion> |
There was a problem hiding this comment.
Using 'latest' for LangVersion is not recommended for production code as it can lead to unpredictable behavior when the SDK is updated. Consider specifying an explicit version like '12' or use 'latestmajor' for more stability.
| <LangVersion>latest</LangVersion> | |
| <LangVersion>7.3</LangVersion> |
| return; | ||
| } | ||
| Value = Item is null ? null : Accessor.Apply(Item); | ||
| PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Value))); |
There was a problem hiding this comment.
The Value property is being set and then PropertyChanged is invoked manually on line 115, but the Value setter already invokes PropertyChanged when the value changes (lines 36-38). This results in duplicate property change notifications. Remove line 115 to avoid the redundant notification.
| PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Value))); |
Description
This update improves existing chart components so that graphs automatically reflect changes in the underlying data values.
Changes
NotifiableDataPoint,ValueHolder, and related helper classes to manage property change notifications.AnnotatorandScatterControlSlimto listen for data updates and re-render automatically.IPropertiesAccessorabstraction to replace expression-based property access.Impact
AnnotatorandScatterControlSlimcomponents inChartDrawing.Testing
Added new unit tests:
ValueHolderTestsverifies that property change notifications correctly update observed values, and that event detachment works properly.ExpressionHelperTestsensures property accessor expressions behave as expected and handle invalid input safely.Added new UI test pages:
ScatterControlSlimNotifyTestandAnnotatorNotifyTestconfirm that graphs visually update when underlying data changes.ScatterControlSlimTest2validates both typed and typeless data scenarios.