Revisit Pen API

[antonfirsov]

Constructor bloat

Currently Pens are immutable. As a consequence, all properties are initialized via constructor, which leads to terrible constructor bloat, especially after adding JointStyle and EndCapStyle to Pen in #185.

Default arguments are also off the table because of extensibility issues and non-constant parameters.

My suggestion is to drop immutability of Pen together with all it's constructors.

public class Pen
{
    IBrush StrokeFill { get; set; } // defaults to black solid brush
    float StrokeWidth { get; set; }

    // Can be also ReadOnlyMemory<float> but not many users know it's assignable to float[]
    // Con: the factory methods in `Pens` will allocate an extra array to guarantee immutability of shared constants.
    float[] StrokePattern { get; set; }

    public JointStyle JointStyle { get; set; }
    public EndCapStyle EndCap { get; set; }
}

An alternative for the above is to introduce a PenBuilder class, but that feels like overcomplication of a simple problem.

IPen

I also propose removing the IPen base interface. I don't see the value for such an abstraction, since a pen is really just a tuple of a few simple properties, there is no logic behind.

- public interface IPen { }

[JimBobSquarePants]

Yeah. Agree here, the constructor bloat is an issue and I don't think we should create some sort of options struct toe workaround.

I think we should also seal the type.

[antonfirsov]

Actually, I haven't considered an options struct, we should explore that too. It could help avoiding the Pen <-> Brush mutability asymmetry, which I don't like much, just proposed as a lesser evil compared to constructor bloat.

Alternative proposal:

// Can be a class if we don't like null values representing defaults
public struct PenOptions
{
    IBrush? StrokeFill { get; set; } // defaults to black solid brush when undefined
    float? StrokeWidth { get; set; }
    float[]? StrokePattern { get; set; }
    public JointStyle JointStyle { get; set; }
    public EndCapStyle EndCap { get; set; }
}

public class Pen
{
    public Pen(IBrush strokeFill) { }
    public Pen(IBrush strokeFill, float strokeWidth) { }
    public Pen(PenOptions options) { }

    IBrush StrokeFill { get; }
    float StrokeWidth { get; }
    Span<float> StrokePattern { get; }
    public JointStyle JointStyle { get; }
    public EndCapStyle EndCap { get; }
}

[jtjackson]

Doesn't make sense to me to have that many constructors for 'Pen' here. Can we just go with single one like:

public class Pen
{
    public Pen(PenOptions options) { }

    IBrush StrokeFill { get; }
    float StrokeWidth { get; }
    Span<float> StrokePattern { get; }
    public JointStyle JointStyle { get; }
    public EndCapStyle EndCap { get; }
}

However, the first proposed approach (with no constructors) may be a better approach.

[antonfirsov]

@tocsoft since you are active these days: any thoughts on this? Any alternative ideas to avoid the constructor bloat?

[dlemstra]

It looks like we are now mixing mutablity and immutability in that class. I think it would be better to make it muttable and remove the IPen interface. Not sure if Pen is the best name for this? Maybe renaming Pen to PenOptions and making it muttable would be another option?

[tocsoft]

I like the options struct pattern, overloads for the most common options, but anything past the basics use the options to define.

Basiclly I think this pattern would work well:

// Can be a class if we don't like null values representing defaults
public struct PenOptions
{
    IBrush? StrokeFill { get; set; } // defaults to black solid brush when undefined
    float? StrokeWidth { get; set; }
    IEnumerable<float>? StrokePattern { get; set; }
    public JointStyle? JointStyle { get; set; }
    public EndCapStyle? EndCap { get; set; }
}

public sealed class Pen
{
    public Pen(IBrush strokeFill) { }
    public Pen(IBrush strokeFill, float strokeWidth) { }
    public Pen(PenOptions options) { }

    public IBrush StrokeFill { get; }
    public float StrokeWidth { get; }
    public ReadonlySpan<float> StrokePattern { get; }
    public JointStyle JointStyle { get; }
    public EndCapStyle EndCap { get; }
}

• StrokePattern should be ReadonlySpan<> rather than Span<> on the pen and and probably IEnumerable<> on the options

---

I'm not a fan of making Pen mutable as it mean the class would no longer be safe to be shared across threads, ideally a user would/could define the pens they want statically/early and use them throughout their code. (Fonts already require/encourages having a shared central location for font access its not much different).

Also as an aside: (but somewhat related due to the dependency) While we are already breaking IPen would it make sense to fix up/remove the IBrush interface too and instead just move it over to be an abstract base class instead.

[antonfirsov]

I also think now that the PenOptions direction would be better since it preserves immutability. +1 for turning IBrush into an abstract class for consistency.

[JimBobSquarePants]

Fixed with #257 and #211