Refactor SnapLines to use IList directly instead of casting to ArrayList

[elachlan]

Kind of related: #8140

Mostly trying to remove ArrayList usage through the code base. This code is using a typesafe cast to ArrayList, where it can just use IList directly.

We cannot convert to List<T> because we have a public API that uses ArrayList ParentControlDesigner.AddPaddingSnapLines:

winforms/src/System.Windows.Forms.Design/src/System/Windows/Forms/Design/ParentControlDesigner.cs

Lines 295 to 319 in cc1a968

	// We need to allocation new ArrayList and pass it to the caller..
	// So its ok to Suppress this.
	protected void AddPaddingSnapLines(ref ArrayList snapLines)
	{
	snapLines ??= new ArrayList(4);
	//In order to add padding, we need to get the offset from the usable client area of our control
	//and the actual origin of our control. In other words: how big is the non-client area here?
	// Ex: we want to add padding on a form to the insides of the borders and below the titlebar.
	Point offset = GetOffsetToClientArea();
	//the display rectangle should be the client area combined with the padding value
	Rectangle displayRectangle = Control.DisplayRectangle;
	displayRectangle.X += offset.X; //offset for non-client area
	displayRectangle.Y += offset.Y; //offset for non-client area
	//add the four paddings of our control
	// Even if a control does not have padding, we still want to add Padding snaplines.
	// This is because we only try to match to matching snaplines. Makes the code a little easier...
	snapLines.Add(new SnapLine(SnapLineType.Vertical, displayRectangle.Left, SnapLine.PaddingLeft, SnapLinePriority.Always));
	snapLines.Add(new SnapLine(SnapLineType.Vertical, displayRectangle.Right, SnapLine.PaddingRight, SnapLinePriority.Always));
	snapLines.Add(new SnapLine(SnapLineType.Horizontal, displayRectangle.Top, SnapLine.PaddingTop, SnapLinePriority.Always));
	snapLines.Add(new SnapLine(SnapLineType.Horizontal, displayRectangle.Bottom, SnapLine.PaddingBottom, SnapLinePriority.Always));
	}

Ultimately it would be great if the public API could be changed to List<SnapLine> to be more explicit.

Microsoft Reviewers: Open in CodeFlow

[halgab]

Changes in this file seem only stylistic. I root for them, but don't look like they belong to this PR

[JeremyKuhne]

@elachlan As ControlDesigner.SnapLines is virtual there isn't a lot we can do with the public API. That said, there is no guarantee what the base type is for ControlDesigner.SnapLines so we can fiddle with the underlying collection.

One idea here is to create a helper that allows us to force type safety internally:

internal sealed class ListAdapter<T> : IList<T>
{
    private readonly IList _list;
    private ListAdapter(IList list) => _list = list.OrThrowIfNull();

    T IList<T>.this[int index]
    {
        get => (T?)_list[index] ?? throw new InvalidOperationException();
        set => _list[index] = value.OrThrowIfNull();
    }

    int ICollection<T>.Count => _list.Count;

    bool ICollection<T>.IsReadOnly => _list.IsReadOnly;

    void ICollection<T>.Add(T item) => _list.Add(item.OrThrowIfNull());
    void ICollection<T>.Clear() => _list.Clear();
    bool ICollection<T>.Contains(T item) => _list.Contains(item);
    void ICollection<T>.CopyTo(T[] array, int arrayIndex) => _list.CopyTo(array, arrayIndex);
    IEnumerator IEnumerable.GetEnumerator() => _list.GetEnumerator();
    int IList<T>.IndexOf(T item) => _list.IndexOf(item);
    void IList<T>.Insert(int index, T item) => _list.Insert(index, item.OrThrowIfNull());
    void IList<T>.RemoveAt(int index) => _list.RemoveAt(index);

    bool ICollection<T>.Remove(T item)
    {
        if (_list.IsReadOnly || !_list.Contains(item))
        {
            return false;
        }

        _list.Remove(item);
        return true;
    }

    IEnumerator<T> IEnumerable<T>.GetEnumerator() => new Enumerator(_list.GetEnumerator());

    private sealed class Enumerator(IEnumerator _enumerator) : IEnumerator<T>
    {
        T IEnumerator<T>.Current => (T)_enumerator.Current;
        object IEnumerator.Current => _enumerator.Current;

        void IDisposable.Dispose() { }
        bool IEnumerator.MoveNext() => _enumerator.MoveNext();
        void IEnumerator.Reset() => _enumerator.Reset();
    }

    public static IList<T> AsList(IList list) => list as IList<T> ?? new ListAdapter<T>(list);
}

Then ControlDesigner could have something like this:

// Maybe we could make this public in the future? Naming would be quite a challenge I think.
internal IList<T> SnapLinesInternal => ListAdapter<SnapLines>.AsList(SnapLines);

// This is the _current_ SnapLinesInternal, which should be renamed.
internal IList<SnapLine> EdgeAndMarginSnapLines() => EdgeAndMarginSnapLines(Control.Margin);

[ghost]

This submission has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 14 days.

It will be closed if no further activity occurs within 7 days of this comment.

[elachlan]

I'll set aside some time and get the last changes sorted.

[elachlan]

@JeremyKuhne I am not really following on how the ListAdapter helps here? IList<T> can't be implicitly converted to IList.

[halgab]

I'm not sure annotating FlowPanelDesigner for NRT should be part of this PR.

[elachlan]

Are you sure? Its a very small change.

[halgab]

I though the parent class wasn't annotated, but apparently it is, so in that light it's a small change (though the parent's parent isn't, which exposes us to incorrect assumptions)

[JeremyKuhne]

I am not really following on how the ListAdapter helps here? IList<T> can't be implicitly converted to IList.

@elachlan ListAdapter is for our own "casting" to IList<T>. We know we want only T, not T?, and not object?. If the data structure is IList<T> it just casts to it, otherwise it creates an adapter for the IList from the property. That way we can write consuming code as if it is always IList<T>.

[elachlan]

@JeremyKuhne I think it clicked. Let me know if this makes sense.

[JeremyKuhne]

Added some comments. Sorry about the delay, a bit busy currently.

[JeremyKuhne]

Needs a blank line and comments for the class

[JeremyKuhne]

This won't work if the adapter is in play. One could add IList to the adapter, but then everything would get the wrapper at that point. If they try to do the same thing again.

You can just not expose the internal and do the ListAdapter call in each usage and return the original IList. Another option is to create some internal interface that we could use as a general pattern to handle this sort of wrapping. Something like:

internal interface IWrapper<T>
{
    T Unwrap();
}

internal ListAdapter<T> : IList<T>, IWrapper<IList>
{
    IList IWrapper<IList>.Unwrap() => _list;
    // ...
}

internal static AdapterHelpers
{
    internal IList Unwrap(this IList list) => list is IWrapper<IList> wrapper ? wrapper.Unwrap() : (IList)list;
    internal IList<T> Adapt(this IList list) => list is IList<T> ilist ? ilist : new ListWrapper(list);
}

// Example usage:

IList<SnapLine> snapLines = base.SnapLines.Adapt<SnapLine>();
// ...
return snapLines.Unwrap();

[elachlan]

I think this results in having to use List<SnapLine> and not IList<SnapLine> so as to avoid ((IList)snapLines).Unwrap();

// Example usage:

List<SnapLine> snapLines = base.SnapLines.Adapt<SnapLine>();
// ...
return snapLines.Unwrap();

Or should I just do the explicit cast?

[JeremyKuhne]

@elachlan this looks good outside of adding comments to the new types and making sure they are in individual files per guidelines.

[JeremyKuhne]

Looks good, just get comments on the new types and move them into their own files.

[elachlan]

@JeremyKuhne how is that?

[JeremyKuhne]

Just one more nit

[elachlan]

Weird test failure I think is unrelated.