Cleans up/Refactors View.Subviews

[tig]

There is a lot of cruft in how Subviews (Add/Remove etc...) work. In order to work on these, I need this cleaned up:

• Fixes #3691 - Adds ViewArrangement.Popover #3852
• Finish Adornments (master Issue)  #2994
• Get rid of Toplevel - Introduce Runnable and Overlapped instead #2491

Proposed Changes/Todos

• Make all core code use InternalSubviews vs. Subviews
• Reorg unit tests
• Code and API doc cleanup

Pull Request checklist:

• I've named my PR in the form of "Fixes #issue. Terse description."
• My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
• My code follows the Terminal.Gui library design guidelines
• I ran dotnet test before commit
• I have made corresponding changes to the API documentation (using /// style comments)
• My changes generate no new warnings
• I have checked my code and corrected any poor grammar or misspellings
• I conducted basic QA to assure all features are working

[tig]

Folks (@BDisp , @tznind , etc...)

It's ALWAYS driven me nuts that View.Subviews has a lowercase V (or that SuperView has an upper case V).

Can I fix this, please?

[BDisp]

Folks (@BDisp , @tznind , etc...)

It's ALWAYS driven me nuts that View.Subviews has a lowercase V (or that SuperView has an upper case V).

Can I fix this, please?

I like more SubViews and SuperView.

[tig]

Thanks.

Here's another topic I'd like your opinion on:

Currently the events around View.Add/Remove are:

    /// <summary>Event fired when this view is added to another.</summary>
    public event EventHandler<SuperViewChangedEventArgs>? Added;

    /// <summary>Method invoked when a subview is being added to this view.</summary>
    /// <param name="e">Event where <see cref="ViewEventArgs.View"/> is the subview being added.</param>
    public virtual void OnAdded (SuperViewChangedEventArgs e)
    {
        View view = e.SubView;
        view.IsAdded = true;
        view.Added?.Invoke (this, e);
    }

    /// <summary>Event fired when this view is removed from another.</summary>
    public event EventHandler<SuperViewChangedEventArgs>? Removed;

    /// <summary>Method invoked when a subview is being removed from this view.</summary>
    /// <param name="e">Event args describing the subview being removed.</param>
    public virtual void OnRemoved (SuperViewChangedEventArgs e)
    {
        View view = e.SubView;
        view.IsAdded = false;
        view.Removed?.Invoke (this, e);
    }

Note that these are super confusing and do not match standard event patterns.

• OnAdded is called when a SubView is being added to this
• Added is raised ON THE SubView when the SubView has been added to this

etc...

There are NO overrides of either virtual method in our codebase.

The uses of the events (Added and Removed) are few. And none of them use the eventargs passed; they only use the events as a signal. Eg. TextField:

    private void TextField_Added (object sender, SuperViewChangedEventArgs e)
    {
        if (Autocomplete.HostControl is null)
        {
            Autocomplete.HostControl = this;
            Autocomplete.PopupInsideContainer = false;
        }
    }

This can and should be simplified.

My proposal:

• Call OnAddedChanged/Raise IsAddedChanged when this.IsAdded changes. Make it EventArg<bool>.
• Call OnSubViewAdded/Raise SubViewAdded when this.Add is called. This can continue to use SuperViewChangedEventArgs.

(Same for Remove).

thots?

[BDisp]

I think that way you're duplicating the event raising twice because IsAddedChanged is the same as using the SubViewAdded . In my opinion the methods OnXXX should be private void because a derived class would not have any reason to override it and broken the event to be raised.

[BDisp]

I would use the private void OnSubViewAdded which raise SubViewAdded.

[tig]

I think that way you're duplicating the event raising twice because IsAddedChanged is the same as using the SubViewAdded . In my opinion the methods OnXXX should be private void because a derived class would not have any reason to override it and broken the event to be raised.

Please read https://gui-cs.github.io/Terminal.GuiV2Docs/docs/events.html

A subclass of View may want to know when it's been added to a superview.

With my proposal it can either:

a) override OnIsAddedChanged

Or

b) subscribe to IsAddedChanged

Take a look at the latest code i pushed here and let me know what you think.

[BDisp]

Since this is a derived class it could override the OnIsAddedChanged.

[BDisp]

Since this is a derived class it could override the OnIsAddedChanged.

[BDisp]

Since this is a derived class it could override the OnIsAddedChanged.

[BDisp]

Since this is a derived class it could override the OnIsAddedChanged.

[BDisp]

A subclass of View may want to know when it's been added to a superview.

With my proposal it can either:

a) override OnIsAddedChanged

Or

b) subscribe to IsAddedChanged

Take a look at the latest code i pushed here and let me know what you think.

Understood. So I see that protect virtual methods has precedence to events. Only for to be sure.

[tig]

A subclass of View may want to know when it's been added to a superview.
With my proposal it can either:
a) override OnIsAddedChanged
Or
b) subscribe to IsAddedChanged
Take a look at the latest code i pushed here and let me know what you think.

Understood. So I see that protect virtual methods has precedence to events. Only for to be sure.

This is precisely the debate here:

#3955 (comment)

This is the current model that is implemented consistently in all of the places where I've modernize the eventing. "OverrideFirst".

[BDisp]

A unit test that you maybe could add into the SubviewTests.cs:

    [Fact]
    public void IsAdded_IsAddedChanged_SubViewAdded_SubViewRemoved ()
    {
        var isAdded = false;

        View superView = new () { Id = "superView" };
        View subView = new () { Id = "subView" };

        superView.SubViewAdded += (s, e) =>
                                  {
                                      Assert.True (isAdded);
                                      Assert.True (subView.IsAdded);
                                      Assert.Equal (subView, e.SubView);
                                      Assert.Equal (superView, e.SuperView);
                                  };

        superView.SubViewRemoved += (s, e) =>
                                    {
                                        Assert.False (isAdded);
                                        Assert.False (subView.IsAdded);
                                        Assert.Equal (subView, e.SubView);
                                        Assert.Equal (superView, e.SuperView);
                                    };

        subView.IsAddedChanged += (s, e) => { isAdded = e.CurrentValue; };

        superView.Add (subView);
        Assert.True (isAdded);
        Assert.True (subView.IsAdded);
        Assert.Single (superView.Subviews);

        superView.Remove (subView);
        Assert.False (isAdded);
        Assert.False (subView.IsAdded);
        Assert.Empty (superView.Subviews);
    }

[tig]

@tznind it's important you review this and are ok with it. Esp:

• Changing the event model for View.Add/Remove
• Renaming Subview to SubView

Sorry so many other files got touched. I got motivated to clean up a bunch of warnings in unit tests and the Subview rename too.

Thanks

[tig]

Ok, look again.

• Nuked View.IsAdded.
• Added View.SuperViewChanged.