Revisit `View.Subviews` API

[tig]

The List<View> infrastructure in View (see ViewSubviews.cs) is inefficient, clumsy, and a source of weird bugs. We should think hard about how to refactor subview management to address.

Examples:

AddAt/RemoveAt - In Bar and other places there's a need to "add/remove at". We don't expose an API for that and code to do it is clumsy:

    // TODO: Move this to View
    /// <summary>Inserts a <see cref="Shortcut"/> in the specified index of <see cref="View.Subviews"/>.</summary>
    /// <param name="index">The zero-based index at which item should be inserted.</param>
    /// <param name="item">The item to insert.</param>
    public void AddShortcutAt (int index, Shortcut item)
    {
        List<View> savedSubViewList = Subviews.ToList ();
        int count = savedSubViewList.Count;
        RemoveAll ();

        for (var i = 0; i <= count; i++)
        {
            if (i == index)
            {
                Add (item);
            }

            if (i < count)
            {
                Add (savedSubViewList [i]);
            }
        }

        SetNeedsDisplay ();
    }

Thread safety

    internal bool _addingView;

Implementation details are too exposed

Subviews is public IList<View> Subviews => _subviews?.AsReadOnly () ?? _empty; which is a feeble attempt to prevent stupidity. A better solution may be to use IObservable or similar.

Background

(This used to be titled "Change SubViews to a be a Dictionary<ustring, View>. The old context is below:)

Aligned with https://github.com/migueldeicaza/gui.cs/issues/252 we should use a Dictionary<ustring, View> where the ustring was the (currently unused) Id of the View.

I keep finding cases where I have to allocate variables to deal with the creation of lots of controls. It would be far cleaner to be able to use the fact that I have to call Parent.Add for each control and leverage that collection.

This:

				var pulseButton = new Button ("Pulse") {
					X = Pos.Center (),
					Y = 3,
					Clicked = () => Pulse ()
				};

				var startButton = new Button ("Start Timer") {
					Y = Pos.Y (pulseButton),
					Clicked = () => Start ()
				};

				var stopbutton = new Button ("Stop Timer") {
					Y = Pos.Y (pulseButton),
					Clicked = () => Stop ()
				};
				Add (startButton);
				Add (pulseButton);
				Add (stopbutton);
				// Center three buttons with 5 spaces between them
				// TODO: Use Pos.Width instead of (Right-Left) when implemented (#502)
				startButton.X = Pos.Left (pulseButton) - (Pos.Right (startButton) - Pos.Left (startButton)) - 5;
				stopbutton.X = Pos.Right (pulseButton) + 5;

Becomes this:

				Add (new Button ("Pulse") {
					Id = "pulseButton",
					X = Pos.Center (),
					Y = 3,
					Clicked = () => Pulse ()
				});
				Add (new Button ("Start Timer") {
					Id = "startButton",
					X = Pos.Center (),
					Y = 3,
					Clicked = () => Pulse ()
				});
				Add (new Button ("Stop Timer") {
					Id = "stopbutton",
					X = Pos.Center (),
					Y = 3,
					Clicked = () => Pulse ()
				});

				// Center three buttons with 5 spaces between them
				// TODO: Use Pos.Width instead of (Right-Left) when implemented (#502)
				Children("startButton").X = ...

Doing this will also enable all kinds of LINQ goodness (e.g. I can find any children views of a type by querying Children.Values e.g. var buttons = Children.Values.OfType<Button>().).

[tig]

Hmmm....

I just noticed that View actually already has a n `IList<View> Subviews property that iterates over the same collection as GetEnumerator.

So I retract part of this suggestion and re-state it as:

• View.Subviews should be a Dictionary<string, View> instead of a list.

[migueldeicaza]

Well the list represents the order in which the views exist, and the dictionary would remove that.

What we could do is keep the two data structures present - whenever a view is added or removed, we update both places - one to track the order, one as a convenience for looking up views. And for scenarios that contain no Id at all, we do not need to create the dictionary at all, so it is a slight optimization.

[tznind]

There is already an Id field in View. Adding index access would be a 1 line change to View.

/// <summary>
/// Returns the first subview whose <see cref="View.Id"/> matches the <paramref name="index"/> or null
/// if none found.
/// </summary>
/// <param name="index"></param>
/// <returns></returns>
public View this [ustring index] => Subviews.FirstOrDefault(v=>ustring.Equals(v.Id,index));

Edit: Might need to check that ustring.Equals does string and not reference comparison

There are other questions (some in other issues referenced from this one):

• Should adding the same Id twice throw Exceptions?
• Should unamed views get automatically assigned an Id?

I think that we should be as 'light touch' as possible when it comes to Id and not do either of these things. As long as the comment on the indexer makes it clear about the behaviour in the case of collisions.