Discussion: should we throw ANEs or do nothing in `UITypeEditor.PaintValue subclasses`

[hughbe]

E.g. in ColorEditor:

        /// <summary>
        /// Paints a representative value of the given object to the provided canvas.
        /// Painting should be done within the boundaries of the provided rectangle.
        /// </summary>
        public override void PaintValue(PaintValueEventArgs e)
        {
            if (e.Value is Color color)
            {
                SolidBrush b = new SolidBrush(color);
                e.Graphics.FillRectangle(b, e.Bounds);
                b.Dispose();
            }
        }

Currently this is a NRE for Paint(null). However, do we want to throw an ANE or nop if the event args are null.

Looking at the construction of this code, I'm inclined to be a nop, as we're checking explicitly whether the event args that were passed to us were invalid

Let me know - I'd like to send in a PR

Current Cases

• ColorEditor: throws NRE
• FontNameEditor: throws NRE
• ImageEditor: nop
• ImageListEditor: throws NRE
• UITypeEditor (and other subclasses that do nothing in PaintValue): nop

[merriemcgaw]

We would like these to be nop in these cases. Thanks!

[JeremyKuhne]

We should start throwing for null arguments in version 6.0 (next major release). Right now we're plagued by inconsistent behavior with these depending on what current state is in seemingly random places.

[elachlan]

So any time we do a null check on an arg and then perform an operation, we replace it with a ArgumentNullException.ThrowIfNull() and remove the if block?

[elachlan]

We should start throwing for null arguments in version 6.0 (next major release). Right now we're plagued by inconsistent behavior with these depending on what current state is in seemingly random places.

@JeremyKuhne would you like me to put together a PR with ArgumentNullException.ThrowIfNull() on the event args parameter for the below overrides?

[JeremyKuhne]

So any time we do a null check on an arg and then perform an operation, we replace it with a ArgumentNullException.ThrowIfNull() and remove the if block?

To be clear, I don't want to add throws where there are none already. We should change NullReferenceExceptions here to ArgumentNullExceptions as we have context.

[elachlan]

I think they have all been refactored to make the null argument a no-op.