feat: implement Presenter classes for weak references #1454

yuto-trd · 2026-01-19T10:25:09Z

Description

Implement Presenter classes (DrawablePresenter, FilterEffectPresenter, BrushPresenter, TransformPresenter) that enable weak object referencing via Reference<T>. These Presenters allow the same object to be "presented" in multiple places without violating the single-parent constraint.

Breaking changes

None

Fixed issues

Enables object reuse in multiple locations through delegation pattern

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f51cfda815

ℹ️ 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".

chatgpt-codex-connector · 2026-01-19T10:27:13Z

src/Beutl.Engine/Graphics/DrawablePresenter.cs

+            if (_target != newTarget)
+            {
+                _target = newTarget;
+                _targetResource?.Dispose();
+                _targetResource = (Drawable.Resource?)newTarget.Value?.ToResource(context);


Refresh target when reference resolves to same Id

When Target is first an unresolved Reference (only Id) and later resolved via Reference.Resolved(...), the Id stays the same but Value becomes non-null. Reference<T> equality compares only Id, so _target != newTarget stays false and _targetResource is never created. That leaves the presenter permanently no-op for deserialized or lazily-resolved references. Consider also checking newTarget.Value (or whether _targetResource is null) to trigger resource creation even when the Id is unchanged.

Useful? React with 👍 / 👎.

Copilot

Pull request overview

This pull request introduces a delegation pattern through four new Presenter classes (DrawablePresenter, FilterEffectPresenter, BrushPresenter, TransformPresenter) that enable weak object referencing via Reference<T>. These classes allow the same graphical object to be "presented" in multiple locations without violating the single-parent constraint inherent in the object hierarchy.

Changes:

Added four new Presenter classes that delegate operations to target objects through weak references
Updated BrushConstructor to handle BrushPresenter resources by unwrapping and delegating to the target brush
Each Presenter maintains its own Resource class that manages the lifecycle of the target resource

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
src/Beutl.Engine/Media/Brushes/BrushPresenter.cs	New class that delegates brush operations to a referenced Brush object with resource management
src/Beutl.Engine/Graphics/Transformation/TransformPresenter.cs	New lightweight presenter for Transform objects that delegates matrix creation
src/Beutl.Engine/Graphics/FilterEffects/FilterEffectPresenter.cs	New class that delegates filter effect operations with resource lifecycle management
src/Beutl.Engine/Graphics/DrawablePresenter.cs	New class that delegates drawable rendering and measurement operations with resource management
src/Beutl.Engine/Graphics/BrushConstructor.cs	Added logic to unwrap BrushPresenter resources and delegate to their target brushes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-19T10:33:17Z

src/Beutl.Engine/Graphics/BrushConstructor.cs

+        // Handle BrushPresenter by delegating to the target brush
+        if (Brush is BrushPresenter.Resource presenter && presenter.TargetResource != null)
+        {
+            return new BrushConstructor(Bounds, presenter.TargetResource as Brush.Resource, BlendMode).CreateShader();


The cast presenter.TargetResource as Brush.Resource is redundant. According to the class definition, TargetResource is already of type Brush.Resource?. The as operator will always succeed (or return null if TargetResource is null, but that's already checked). Consider simplifying to just pass presenter.TargetResource directly.

Copilot · 2026-01-19T10:33:17Z

src/Beutl.Engine/Graphics/Transformation/TransformPresenter.cs

+
+    /// <summary>
+    /// Creates the transformation matrix by delegating to the target Transform.
+    /// Returns Identity matrix if the target is null.


Consider documenting the behavior when the target reference is null or unresolved. While the code handles this gracefully by returning Identity, it would be helpful for API consumers to explicitly state this behavior in the documentation.

Suggested change

/// Returns Identity matrix if the target is null.

/// Returns the identity matrix if the target is null or the reference cannot be resolved.

Copilot · 2026-01-19T10:33:18Z

src/Beutl.Engine/Media/Brushes/BrushPresenter.cs

+using Beutl.Engine;
+using Beutl.Graphics.Rendering;
+
+namespace Beutl.Media;
+
+/// <summary>
+/// A presenter that delegates brush operations to a referenced Brush object.
+/// This allows the same Brush to be "presented" in multiple places without violating
+/// the single-parent constraint.
+/// </summary>
+[SuppressResourceClassGeneration]
+public sealed class BrushPresenter : Brush
+{
+    public BrushPresenter()
+    {
+        ScanProperties<BrushPresenter>();
+    }
+
+    /// <summary>
+    /// Gets the property that holds a reference to the target Brush object.
+    /// </summary>
+    public IProperty<Reference<Brush>> Target { get; } = Property.Create<Reference<Brush>>();
+
+    public override Brush.Resource ToResource(RenderContext context)
+    {
+        var resource = new Resource();
+        bool updateOnly = true;
+        resource.Update(this, context, ref updateOnly);
+        return resource;
+    }
+
+    public new sealed class Resource : Brush.Resource
+    {
+        private Reference<Brush> _target;
+        private Brush.Resource? _targetResource;
+
+        public Reference<Brush> Target
+        {
+            get => _target;
+            set => _target = value;
+        }
+
+        public Brush.Resource? TargetResource
+        {
+            get => _targetResource;
+            set => _targetResource = value;
+        }
+
+        public override void Update(EngineObject obj, RenderContext context, ref bool updateOnly)
+        {
+            base.Update(obj, context, ref updateOnly);
+            var presenter = (BrushPresenter)obj;
+            var newTarget = context.Get(presenter.Target);
+
+            if (_target != newTarget)
+            {
+                _target = newTarget;
+                _targetResource?.Dispose();
+                _targetResource = (Brush.Resource?)newTarget.Value?.ToResource(context);
+                if (!updateOnly)
+                {
+                    Version++;
+                    updateOnly = true;
+                }
+            }
+            else if (_targetResource != null && newTarget.Value != null)
+            {
+                var oldVersion = _targetResource.Version;
+                bool _ = false;
+                _targetResource.Update(newTarget.Value, context, ref _);
+                if (!updateOnly && oldVersion != _targetResource.Version)
+                {
+                    Version++;
+                    updateOnly = true;
+                }
+            }
+        }
+
+        protected override void Dispose(bool disposing)
+        {
+            if (disposing)
+            {
+                _targetResource?.Dispose();
+            }
+            base.Dispose(disposing);
+        }
+    }
+}


The new BrushPresenter class lacks test coverage. Given that the repository has comprehensive testing for other drawable and brush-related functionality (as seen in ShapeTests.cs), consider adding tests to verify: (1) proper delegation to the target brush, (2) correct resource lifecycle management, and (3) behavior when the target reference is null or changes.

Copilot · 2026-01-19T10:33:18Z

src/Beutl.Engine/Graphics/DrawablePresenter.cs

+using Beutl.Engine;
+using Beutl.Graphics.Rendering;
+
+namespace Beutl.Graphics;
+
+/// <summary>
+/// A presenter that delegates drawable operations to a referenced Drawable object.
+/// This allows the same Drawable to be "presented" in multiple places without violating
+/// the single-parent constraint.
+/// </summary>
+[SuppressResourceClassGeneration]
+public sealed class DrawablePresenter : Drawable
+{
+    public DrawablePresenter()
+    {
+        ScanProperties<DrawablePresenter>();
+    }
+
+    /// <summary>
+    /// Gets the property that holds a reference to the target Drawable object.
+    /// </summary>
+    public IProperty<Reference<Drawable>> Target { get; } = Property.Create<Reference<Drawable>>();
+
+    /// <summary>
+    /// Renders the drawable by delegating to the target Drawable.
+    /// Does nothing if the target is null.
+    /// </summary>
+    public override void Render(GraphicsContext2D context, Drawable.Resource resource)
+    {
+        var r = (Resource)resource;
+        if (r.TargetResource != null)
+        {
+            ((Drawable)r.TargetResource.GetOriginal()).Render(context, r.TargetResource);
+        }
+    }
+
+    /// <summary>
+    /// This method is not called because Render() delegates to the target directly.
+    /// </summary>
+    protected override void OnDraw(GraphicsContext2D context, Drawable.Resource resource)
+    {
+        // Not called because Render() delegates to the target directly
+    }
+
+    /// <summary>
+    /// Measures the drawable by delegating to the target Drawable.
+    /// Returns Size.Empty if the target is null.
+    /// </summary>
+    protected override Size MeasureCore(Size availableSize, Drawable.Resource resource)
+    {
+        var r = (Resource)resource;
+        if (r.TargetResource == null)
+        {
+            return Size.Empty;
+        }
+        return ((Drawable)r.TargetResource.GetOriginal()).MeasureInternal(availableSize, r.TargetResource);
+    }
+
+    public override Drawable.Resource ToResource(RenderContext context)
+    {
+        var resource = new Resource();
+        bool updateOnly = true;
+        resource.Update(this, context, ref updateOnly);
+        return resource;
+    }
+
+    public new sealed class Resource : Drawable.Resource
+    {
+        private Reference<Drawable> _target;
+        private Drawable.Resource? _targetResource;
+
+        public Reference<Drawable> Target
+        {
+            get => _target;
+            set => _target = value;
+        }
+
+        public Drawable.Resource? TargetResource
+        {
+            get => _targetResource;
+            set => _targetResource = value;
+        }
+
+        public override void Update(EngineObject obj, RenderContext context, ref bool updateOnly)
+        {
+            base.Update(obj, context, ref updateOnly);
+            var presenter = (DrawablePresenter)obj;
+            var newTarget = context.Get(presenter.Target);
+
+            if (_target != newTarget)
+            {
+                _target = newTarget;
+                _targetResource?.Dispose();
+                _targetResource = (Drawable.Resource?)newTarget.Value?.ToResource(context);
+                if (!updateOnly)
+                {
+                    Version++;
+                    updateOnly = true;
+                }
+            }
+            else if (_targetResource != null && newTarget.Value != null)
+            {
+                var oldVersion = _targetResource.Version;
+                bool _ = false;
+                _targetResource.Update(newTarget.Value, context, ref _);
+                if (!updateOnly && oldVersion != _targetResource.Version)
+                {
+                    Version++;
+                    updateOnly = true;
+                }
+            }
+        }
+
+        protected override void Dispose(bool disposing)
+        {
+            if (disposing)
+            {
+                _targetResource?.Dispose();
+            }
+            base.Dispose(disposing);
+        }
+    }
+}


The new DrawablePresenter class lacks test coverage. Given that the repository has comprehensive testing for other drawable functionality (as seen in ShapeTests.cs), consider adding tests to verify: (1) proper delegation to the target drawable for rendering and measuring, (2) correct resource lifecycle management, and (3) behavior when the target reference is null or changes.

Copilot · 2026-01-19T10:33:18Z

src/Beutl.Engine/Media/Brushes/BrushPresenter.cs

+        public override void Update(EngineObject obj, RenderContext context, ref bool updateOnly)
+        {
+            base.Update(obj, context, ref updateOnly);
+            var presenter = (BrushPresenter)obj;
+            var newTarget = context.Get(presenter.Target);
+
+            if (_target != newTarget)
+            {
+                _target = newTarget;
+                _targetResource?.Dispose();
+                _targetResource = (Brush.Resource?)newTarget.Value?.ToResource(context);
+                if (!updateOnly)
+                {
+                    Version++;
+                    updateOnly = true;
+                }
+            }
+            else if (_targetResource != null && newTarget.Value != null)
+            {
+                var oldVersion = _targetResource.Version;
+                bool _ = false;
+                _targetResource.Update(newTarget.Value, context, ref _);
+                if (!updateOnly && oldVersion != _targetResource.Version)
+                {
+                    Version++;
+                    updateOnly = true;
+                }
+            }
+        }


The Update method contains duplicated logic that appears across all Presenter classes (BrushPresenter, DrawablePresenter, FilterEffectPresenter). Consider extracting this common pattern into a base class or helper method to reduce code duplication and improve maintainability. The pattern includes: (1) comparing target references, (2) disposing and recreating resources on change, (3) updating existing resources when unchanged, and (4) managing version increments.

Copilot · 2026-01-19T10:33:19Z

src/Beutl.Engine/Graphics/DrawablePresenter.cs

+        public override void Update(EngineObject obj, RenderContext context, ref bool updateOnly)
+        {
+            base.Update(obj, context, ref updateOnly);
+            var presenter = (DrawablePresenter)obj;
+            var newTarget = context.Get(presenter.Target);
+
+            if (_target != newTarget)
+            {
+                _target = newTarget;
+                _targetResource?.Dispose();
+                _targetResource = (Drawable.Resource?)newTarget.Value?.ToResource(context);
+                if (!updateOnly)
+                {
+                    Version++;
+                    updateOnly = true;
+                }
+            }
+            else if (_targetResource != null && newTarget.Value != null)
+            {
+                var oldVersion = _targetResource.Version;
+                bool _ = false;
+                _targetResource.Update(newTarget.Value, context, ref _);
+                if (!updateOnly && oldVersion != _targetResource.Version)
+                {
+                    Version++;
+                    updateOnly = true;
+                }
+            }
+        }


There's a potential issue when a target reference's Value becomes null while keeping the same Id. If _target == newTarget (same Id) but newTarget.Value is null, the else-if condition is skipped, leaving _targetResource pointing to a stale resource. Consider adding logic to dispose _targetResource when newTarget.Value is null, even if the reference Id hasn't changed.

Copilot · 2026-01-19T10:33:20Z

src/Beutl.Engine/Graphics/FilterEffects/FilterEffectPresenter.cs

+            else if (_targetResource != null && newTarget.Value != null)
+            {
+                var oldVersion = _targetResource.Version;
+                bool _ = false;
+                _targetResource.Update(newTarget.Value, context, ref _);
+                if (!updateOnly && oldVersion != _targetResource.Version)
+                {
+                    Version++;
+                    updateOnly = true;
+                }


There's a potential issue when a target reference's Value becomes null while keeping the same Id. If _target == newTarget (same Id) but newTarget.Value is null, the else-if condition is skipped, leaving _targetResource pointing to a stale resource. Consider adding logic to dispose _targetResource when newTarget.Value is null, even if the reference Id hasn't changed.

Suggested change

else if (_targetResource != null && newTarget.Value != null)

{

var oldVersion = _targetResource.Version;

bool _ = false;

_targetResource.Update(newTarget.Value, context, ref _);

if (!updateOnly && oldVersion != _targetResource.Version)

{

Version++;

updateOnly = true;

}

else

{

if (newTarget.Value == null)

{

if (_targetResource != null)

{

_targetResource.Dispose();

_targetResource = null;

if (!updateOnly)

{

Version++;

updateOnly = true;

}

}

}

else if (_targetResource != null)

{

var oldVersion = _targetResource.Version;

bool _ = false;

_targetResource.Update(newTarget.Value, context, ref _);

if (!updateOnly && oldVersion != _targetResource.Version)

{

Version++;

updateOnly = true;

}

}

Copilot · 2026-01-19T10:33:20Z

src/Beutl.Engine/Media/Brushes/BrushPresenter.cs

+        var resource = new Resource();
+        bool updateOnly = true;
+        resource.Update(this, context, ref updateOnly);
+        return resource;
+    }
+
+    public new sealed class Resource : Brush.Resource


Class has the same name as its base class.

Suggested change

var resource = new Resource();

bool updateOnly = true;

resource.Update(this, context, ref updateOnly);

return resource;

}

public new sealed class Resource : Brush.Resource

var resource = new BrushPresenterResource();

bool updateOnly = true;

resource.Update(this, context, ref updateOnly);

return resource;

}

public sealed class BrushPresenterResource : Brush.Resource

Copilot · 2026-01-19T10:33:20Z

src/Beutl.Engine/Graphics/DrawablePresenter.cs

+        return resource;
+    }
+
+    public new sealed class Resource : Drawable.Resource


Class has the same name as its base class.

Copilot · 2026-01-19T10:33:20Z

src/Beutl.Engine/Graphics/FilterEffects/FilterEffectPresenter.cs

+        return resource;
+    }
+
+    public new sealed class Resource : FilterEffect.Resource


Class has the same name as its base class.

Add DrawablePresenter, FilterEffectPresenter, BrushPresenter, and TransformPresenter to enable weak object referencing via Reference<T>. This allows the same object to be "presented" in multiple places without violating single-parent constraints. - All Presenters delegate method calls to target objects - Custom Resource classes manage target lifecycle - BrushConstructor updated to support BrushPresenter

Add DropDownButton-based UI for selecting target objects when property value is a Presenter class (FilterEffectPresenter, TransformPresenter, BrushPresenter). Uses ObjectSearcher to enumerate available targets within the same Scene. - Add TargetObjectInfo record for holding target information - Add TargetPickerFlyout and TargetPickerFlyoutViewModel - Update FilterEffectEditor, TransformEditor, BrushEditor with Presenter mode layout (property name | DropDownButton | menu) - Update FilterEffectListItemEditor, TransformListItemEditor with Presenter mode layout (reorderHandle | DropDownButton | buttons)

…or dynamic updates

…t, and Transform presenters

…Effect, and Transform

github-actions · 2026-01-20T18:52:55Z

No TODO comments were found.

github-actions · 2026-01-20T18:55:11Z

Package	Line Rate	Branch Rate	Complexity	Health
Beutl.Operators	3%	0%	87	➖
Beutl.Editor	98%	94%	594	✔
Beutl.Core	40%	36%	2939	➖
Beutl.Engine	27%	23%	10585	➖
Beutl.Threading	100%	94%	118	✔
Beutl.Utilities	73%	67%	358	➖
Beutl.Configuration	39%	19%	288	➖
Beutl.Extensibility	14%	15%	105	➖
Beutl.ProjectSystem	19%	11%	1879	➖
Beutl.Language	0%	0%	792	➖
Summary	30% (12709 / 42252)	28% (3456 / 12134)	17745	➖

Minimum allowed line rate is 0%

Copilot AI review requested due to automatic review settings January 19, 2026 10:25

Copilot started reviewing on behalf of yuto-trd January 19, 2026 10:25 View session

chatgpt-codex-connector bot reviewed Jan 19, 2026

View reviewed changes

Copilot AI reviewed Jan 19, 2026

View reviewed changes

yuto-trd force-pushed the yuto-trd/object-presenter branch from 70a0561 to d7a25d9 Compare January 20, 2026 18:18

yuto-trd added 4 commits January 21, 2026 03:51

feat: refactor CurrentTargetName to use SubscribeCurrentValueChange f…

3c54837

…or dynamic updates

feat: implement IPresenter interface for Brush, Drawable, FilterEffec…

1e401a7

…t, and Transform presenters

feat: update presenters to use IPresenter interface for Brush, Filter…

81e2745

…Effect, and Transform

yuto-trd force-pushed the yuto-trd/object-presenter branch from d7a25d9 to 81e2745 Compare January 20, 2026 18:52

	/// Returns Identity matrix if the target is null.
	/// Returns the identity matrix if the target is null or the reference cannot be resolved.

Uh oh!

feat: implement Presenter classes for weak references #1454

Are you sure you want to change the base?

feat: implement Presenter classes for weak references #1454

Conversation

yuto-trd commented Jan 19, 2026

Description

Breaking changes

Fixed issues

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 20, 2026

Uh oh!

github-actions bot commented Jan 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants