Gui.cs: StatusBar fix exposes big issue...Lack of Dispose

[tig]

In https://github.com/migueldeicaza/gui.cs/pull/685, I caused a bug on exiting scenarios.

The problem is, within the StatusBar constructor we connect up to Application.Resized

Application.Resized += (e) => {

This event subscription never gets removed, so even after the status bar has been destroyed this code gets called.

In order to fix this correctly, the View class heirarchy needs to support Disposable, I think,

I just built a POC to illustrate it, but it feels like a much deeper change than I'm comfy throughing together on a saturday morning. For now I'm going to hack around it to ensure that when the event handler is called nothing bad happens.

[BDisp]

I already realize that the cap and num does not display its status at beginning.

[tig]

I already realize that the cap and num does not display its status at beginning.

That's actually a different problem, caused by the fact that UI Catalog never calls into the system to ask for the state of those keys, and just waits for a key message.

[BDisp]

May be related with this #683?

[tig]

May be related with this #683?

I don't think so.

[tig]

@migueldeicaza , while we have your attention:

I have convinced myself the Responder class hierarchy needs formal IDisposable support.

I have written a POC that adds IDisposable to Responder and then modified Application and a few other key places to call Dispose on subviews appropriately.

e.g. here's View.Dispose:

protected override void Dispose (bool disposing)
{
    foreach (var subview in InternalSubviews) {
        subview.Dispose ();
    }

    base.Dispose (disposing);
}

I added this to Responder:

#if DEBUG
        /// <summary>
        /// For debug purposes to verify objects are being disposed properly
        /// </summary>
        public bool WasDisposed = false;
        public static List<Responder> Instances = new List<Responder> ();
        public Responder ()
        {
            Instances.Add (this);
        }
#endif 

and used this to show it was working:

[Fact]
public void Run_All_Sceanrios ()
{
    List<Type> scenarioClasses = Scenario.GetDerivedClasses<Scenario> ();
    Assert.NotEmpty (scenarioClasses);

    foreach (var scenarioClass in scenarioClasses) {
        // Setup some fake kepresses 
        // Passing empty string will cause just a ctrl-q to be fired
        Console.MockKeyPresses.Clear ();
        int stackSize = CreateInput ("");
        int iterations = 0;
        Application.Iteration = () => {
            iterations++;
            // Stop if we run out of control...
            if (iterations > 10) {
                Application.RequestStop ();
            }
        };
        Application.Init (new FakeDriver (), new NetMainLoop (() => FakeConsole.ReadKey (true)));

        var ms = 1000;
        var abortCount = 0;
        Func<MainLoop, bool> abortCallback = (MainLoop loop) => {
            abortCount++;
            Application.RequestStop ();
            return false;
        };
        var token = Application.MainLoop.AddTimeout (TimeSpan.FromMilliseconds (ms), abortCallback);

        var scenario = (Scenario)Activator.CreateInstance (scenarioClass);
        scenario.Init (Application.Top, Colors.Base);
        scenario.Setup ();
        scenario.Run ();

        Application.Shutdown ();

        Assert.Equal (0, abortCount);
        // # of key up events should match # of iterations
        Assert.Equal (1, iterations);
        Assert.Equal (stackSize, iterations);
    }

    foreach (var inst in Responder.Instances) {
        Assert.True (inst.WasDisposed);
    }
}

What do you think of adding this? I think we need it.

[migueldeicaza]

Adding Disposable is fine, but I think that this surfaces one of the things that I hate most about the event programming in C#, which is that these things can leak and generally consume too much memory.

I wonder if we should instead encourage folks to either subclass Application and catch a proper virtual method for "Resize", or using a delegate pattern for the event.

[tig]

Fixed in #693