A review for the reference layer branch

[Equbuxu]

A better way to implement changes

At the moment the change that creates the reference layer also has transforming logic. But you should be able to transform the layer without recreating it too, so I think you should make 3 changes:

• Regular change: create reference layer (it shouldn't do any transforming)
• Regular change: delete reference layer (it should also just delete it)
• Updateable change: transform reference layer

This way:

• you'll have no duplicated code,
• it'll be easier to implement undo,
• you won't need nullables in actions.
• you won't need to call RaisePropertyChange on ReferenceBitmap all the time (more on that below)
  You'll still be able to start the transform change right after the reference layer is created.

Surface and thread safety

There are problems with putting a Surface into IReadOnlyReferenceLayer. Sorry for not bringing this up before, it's a bit complicated so I just kinda haven't though of this stuff back when you were implementing it.

In general, the things exposed through the read only interfaces must be one of these:

• A value type, e.g. an into or a struct
• An immutable type, e.g. a string or an ImmutableList
• A mutable type that is designed with thread safety in mind, e.g. ChunkyImage (worst option)

In all cases there should be no way to modify the value/object through the read only interfaces:

• For value types you can just omit the setter since they are always copied
• Immutable types are already immutable, so omitting a setter is enough too
• A mutable class must be wrapped in a read only interface that hides any methods that modify it's state (Note that just copying the mutable type won't work, since the UI thread will do the copying, and the ChangeableDocument can modify the object in the process)

Exposing a Surface in IReadOnlyReferenceLayer has two problems. Firstly, Surface as it is is not immutable, so anyone can draw on it from UI, which must not be possible (for thread safety and architecture consistency). Of course it's unlikely that anyone would wanna draw on the reference layer, but it's better to enforce those things, not everyone will know better. This can be solved by wrapping it in a read-only interface, but there is one more issue. Surface and SKSurface are not thread safe. It's entirely possible for changeable document to modify it in some way (draw on it, dispose it, etc.) while, say, the UI thread is copying it into a WriteableBitmap. Again, it probably won't happen or won't cause issues, but thread safety is complicated, and I'm not entirely sure how SKSurface works, so it's better to just use something else. Btw, passing the Surface as a reference from the UI is a problem too, cause UI keeps a reference and can use to write into the surface. An ImmutableArray with the bitmap's bits is probably the easiest replacement option here, potentially with some extra variables like bitmap size.

Other things that I've fixed:

Viewport shouldn't need properties for the reference layer because it already has access to DocumentViewModel. You can just bind to it's properties directly in Viewport.xaml, e.g. {Binding Document.ReferenceShape}. Also, you don't need to manually create bindings in the constructor. While fixing this I've noticed that moving the reference layer around is now laggy, which is caused by unnecessary calls to doc.RaisePropertyChanged(nameof(doc.ReferenceBitmap)). As I said above, this can be avoided by removing transformation logic from the CreateReferenceLayer_UpdateableChange (and making it a regular change).

Minor tips/nitpicks:

• IReadOnlyDocument.GetReferenceLayer() should be a property (done)
• DocumentUpdater.ApplyChangeFromChangeInfo(..) -> case CreateReferenceLayer_ChangeInfo separate the body into a function to keep the switch readable (done)
• Here is the code used to calculate the reference image size on creation:

if (referenceImage.Size.X > referenceImage.Size.Y)
{
    shapeWidth = Width;
    shapeHeight = Width / referenceImage.Size.X * referenceImage.Size.Y;
}
else
{
    shapeWidth = Height / referenceImage.Size.Y * referenceImage.Size.X;
    shapeHeight = Height;
}
ShapeCorners corners = new ShapeCorners(new RectD(VecD.Zero, new(shapeWidth, shapeHeight)));

Almost the same can be written as:

RectD referenceImageRect = new RectD(VecD.Zero, SizeBindable).AspectFit(new RectD(VecD.Zero, referenceImage.Size));
ShapeCorners corners = new ShapeCorners(referenceImageRect);

AspectFit is a very nice function, it takes a rectangle and fits it into another rectangle while keeping it's aspect ratio and keeping it centered. I'm saying "almost" only because the original code has a bug somewhere, so it doesn't work properly

• Here is the code that is supposed to make the bitmap smooth

RenderOptions.BitmapScalingMode="{Binding Zoombox.Scale, Converter={StaticResource ScaleToBitmapScalingModeConverter}}">

It doesn't work properly because the scale of the viewport isn't the same as the scale of the reference image. (fixed)

• It's marginally better to check for null via obj is null instead of obj == null, because obj == null might call the overloaded operator which can behave in any way, while is null checks for null directly and won't be affected by operator overloading. This barely has any importance though, I'm just saying.