Optimize property tracking with CorePropertyOperationObserver #1447
Conversation
…ionObserver for improved property tracking
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var operation = new UpdatePropertyValueOperation<Easing?>( | ||
| source, childPath, args.NewValue, args.OldValue) | ||
| { | ||
| _splineEasingPublisher?.Dispose(); | ||
| _splineEasingPublisher = null; | ||
|
|
||
| if (args.NewValue is SplineEasing newSplineEasing) | ||
| { | ||
| _splineEasingPublisher = new SplineEasingOperationObserver( | ||
| _operations, | ||
| newSplineEasing, | ||
| _sequenceNumberGenerator, | ||
| keyFrame, | ||
| childPath, | ||
| _propertyPathsToTrack); | ||
| } | ||
| } | ||
|
|
||
| var operationType = typeof(UpdatePropertyValueOperation<>).MakeGenericType(args.Property.PropertyType); | ||
| var operation = (ChangeOperation)Activator.CreateInstance( | ||
| operationType, | ||
| source, childPath, args.NewValue, args.OldValue)!; | ||
| operation.SequenceNumber = _sequenceNumberGenerator.GetNext(); | ||
| SequenceNumber = _sequenceNumberGenerator.GetNext() | ||
| }; |
There was a problem hiding this comment.
The KeyFrame-specific handler still emits an UpdatePropertyValueOperation for the Easing property (this method builds and pushes the operation on the subject), but InitializeChildPublishers now constructs a CorePropertyOperationObserver for every core property—including KeyFrame.Easing—and each of those observers subscribes to PropertyChanged and publishes the same update operation. As a result, changing KeyFrame.Easing now produces two identical operations with different sequence numbers, bloating undo/redo or remote sync streams for that property. Consider letting the per-property observer handle the publish or skipping the extra Easing handler to avoid duplicates.
Useful? React with 👍 / 👎.
Removed unused property update operation logic.
|
No TODO comments were found. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new CorePropertyOperationObserver<T> class to improve property tracking architecture and refactors CoreObjectOperationObserver to delegate per-property tracking to these specialized observers. The changes also optimize several places by removing reflection-based object instantiation in favor of direct generic type instantiation.
Key Changes:
- Added new
CorePropertyOperationObserver<T>class that handles tracking for individual properties, including their child objects and collections - Refactored
CoreObjectOperationObserverto createCorePropertyOperationObserver<T>instances for each tracked property instead of directly managing child publishers - Replaced reflection-based
Activator.CreateInstancecalls with direct generic instantiation inEnginePropertyOperationObserver
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Beutl.Editor/Observers/CorePropertyOperationObserver.cs | New class that encapsulates property-level tracking logic, creating child/collection observers as needed and managing property value change operations |
| src/Beutl.Editor/Observers/CoreObjectOperationObserver.cs | Refactored to delegate property tracking to CorePropertyOperationObserver<T> instances; simplified property change handler to only manage KeyFrame.Easing special case for SplineEasing publisher |
| src/Beutl.Editor/Observers/EnginePropertyOperationObserver.cs | Performance optimization replacing reflection-based instantiation with direct generic instantiation in three methods |
Comments suppressed due to low confidence (1)
src/Beutl.Editor/Observers/CoreObjectOperationObserver.cs:208
- The OnPropertyChanged method will create duplicate operations for KeyFrame.Easing property changes. When the Easing property changes on a KeyFrame:
- The CorePropertyOperationObserver instance (created in InitializeChildPublishers for the Easing property) will handle the property change and emit an UpdatePropertyValueOperation
- This OnPropertyChanged handler will also handle the same property change and emit another UpdatePropertyValueOperation
This results in two operations being published for a single property change. Consider either:
- Removing the operation publishing from this handler (lines 202-207) and only keeping the RecreateSplineEasingPublisher call, or
- Excluding the Easing property from being tracked in InitializeChildPublishers when the object is a KeyFrame
private void OnPropertyChanged(object? sender, PropertyChangedEventArgs e)
{
if (PublishingSuppression.IsSuppressed) return;
if (e is not CorePropertyChangedEventArgs<Easing> args || sender is not CoreObject source)
return;
if (args.Property.Id != KeyFrame.EasingProperty.Id)
return;
if (_propertiesToTrack != null &&
!_propertiesToTrack.Contains(args.Property.Name))
return;
RecreateSplineEasingPublisher();
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using System.Collections; | ||
| using System.ComponentModel; | ||
| using System.Reactive.Subjects; | ||
| using Beutl.Editor.Operations; | ||
| using Beutl.Serialization; | ||
|
|
||
| namespace Beutl.Editor.Observers; | ||
|
|
||
| public sealed class CorePropertyOperationObserver<T> : IOperationObserver | ||
| { | ||
| private CoreObjectOperationObserver? _childPublisher; | ||
| private IOperationObserver? _collectionPublisher; | ||
| private readonly CoreProperty<T> _property; | ||
| private readonly ICoreObject _object; | ||
| private readonly Subject<ChangeOperation> _operations = new(); | ||
| private readonly HashSet<string>? _propertyPathsToTrack; | ||
| private readonly string _propertyPath; | ||
| private readonly OperationSequenceGenerator _sequenceNumberGenerator; | ||
| private readonly IDisposable? _subscription; | ||
|
|
||
| public CorePropertyOperationObserver( | ||
| IObserver<ChangeOperation>? observer, | ||
| ICoreObject obj, | ||
| CoreProperty<T> property, | ||
| OperationSequenceGenerator sequenceNumberGenerator, | ||
| string propertyPath, | ||
| HashSet<string>? propertyPathsToTrack = null) | ||
| { | ||
| _object = obj; | ||
| _property = property; | ||
| _propertyPath = propertyPath; | ||
| _sequenceNumberGenerator = sequenceNumberGenerator; | ||
| _propertyPathsToTrack = propertyPathsToTrack; | ||
|
|
||
| if (observer != null) | ||
| { | ||
| _subscription = _operations.Subscribe(observer); | ||
| } | ||
|
|
||
| RecreateChildPublisher(); | ||
| obj.PropertyChanged += OnPropertyChanged; | ||
| } | ||
|
|
||
| public IObservable<ChangeOperation> Operations => _operations; | ||
|
|
||
| public void Dispose() | ||
| { | ||
| _object.PropertyChanged -= OnPropertyChanged; | ||
|
|
||
| _childPublisher?.Dispose(); | ||
| _childPublisher = null; | ||
|
|
||
| _collectionPublisher?.Dispose(); | ||
| _collectionPublisher = null; | ||
|
|
||
| _subscription?.Dispose(); | ||
| _operations.OnCompleted(); | ||
| } | ||
|
|
||
| private void RecreateChildPublisher() | ||
| { | ||
| _childPublisher?.Dispose(); | ||
| _childPublisher = null; | ||
|
|
||
| _collectionPublisher?.Dispose(); | ||
| _collectionPublisher = null; | ||
|
|
||
| T value = _object.GetValue(_property); | ||
|
|
||
| switch (value) | ||
| { | ||
| case IList list: | ||
| var elementType = ArrayTypeHelpers.GetElementType(list.GetType()); | ||
| if (elementType == null) throw new InvalidOperationException("Could not determine the element type of the list."); | ||
| var observerType = typeof(CollectionOperationObserver<>).MakeGenericType(elementType); | ||
|
|
||
| _collectionPublisher = (IOperationObserver)Activator.CreateInstance(observerType, | ||
| _operations, list, _object, | ||
| _propertyPath, _sequenceNumberGenerator, _propertyPathsToTrack)!; | ||
| break; | ||
| case ICoreObject child: | ||
| _childPublisher = new CoreObjectOperationObserver( | ||
| _operations, | ||
| child, | ||
| _sequenceNumberGenerator, | ||
| _propertyPath, | ||
| _propertyPathsToTrack); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| public void OnPropertyChanged(object? sender, PropertyChangedEventArgs e) | ||
| { | ||
| if (PublishingSuppression.IsSuppressed) return; | ||
| if (e is not CorePropertyChangedEventArgs<T> args) return; | ||
| if (sender is not ICoreObject source) return; | ||
|
|
||
| // プロパティが同じか | ||
| if (args.Property.Id != _property.Id) return; | ||
|
|
||
| RecreateChildPublisher(); | ||
|
|
||
| var operation = new UpdatePropertyValueOperation<T?>( | ||
| (CoreObject)source, _propertyPath, args.NewValue, args.OldValue) | ||
| { | ||
| SequenceNumber = _sequenceNumberGenerator.GetNext() | ||
| }; | ||
| _operations.OnNext(operation); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new CorePropertyOperationObserver class lacks test coverage. There are comprehensive tests for CoreObjectOperationObserver, EnginePropertyOperationObserver, CollectionOperationObserver, and other observer classes in the tests/Beutl.UnitTests/Editor/ directory, but no tests exist for the newly introduced CorePropertyOperationObserver. Consider adding tests to cover:
- Property change tracking and operation emission
- Child publisher creation and disposal for ICoreObject values
- Collection publisher creation and disposal for IList values
- Proper disposal of resources
- Interaction with PropertyPathsToTrack filtering
Minimum allowed line rate is |
Description
Introduce
CorePropertyOperationObserverto enhance property tracking and optimizeCoreObjectOperationObserver. This change improves the handling of property updates and operations.Breaking changes
No breaking changes introduced.
Fixed issues
No specific issues fixed.