TableView doesn't dispose properly

[dodexahedron]

TableView is IDisposable by virtue of inheriting it from its ancestors.

However, it does not override Dispose, and it needs to.

TableView can and most likely nearly always will own a DataTable, which is IDisposable, so it needs to be sure that's cleaned up and then should delegate to base.Dispose() before finishing its Dispose method.

Related but optional:

Could potentially subscribe to the Disposed event of any DataTable given to a TableView, to help track things.

[dodexahedron]

This can be tricky and should likely not be done unconditionally, because something else TG isn't aware of may still care about the DataTable after TableView.Dispose is called.

It may be wise to, instead of unconditionally Disposing the DataTable, provide an overload of the constructor with a boolean as well as a public settable property for that boolean indicating whether the TableView owns the DataTable or not (as is standard - see any of the Stream classes for example), and therefore is supposed to dispose it or not.

An open question is whether the setter for TableView.Table should also respect that boolean, if it is, for example, passed null. I think the answer to that question is "yes," so long as TableView.Table is a property. But I think the implications of the IDisposable interface suggest that the Table property might be better off not having a setter, and instead having a method for setting it, which has the same optional boolean parameter for DataTable ownership that the constructor has, and which also sets that boolean, accordingly.

[dodexahedron]

Actually....

This is more about DataTableSource, I suppose, since it's the type that explicitly MUST own a DataTable.

But the inheritance and ownership tree would make it a LOT easier for Terminal.Gui if the ITableSource interface required IDisposable.

Otherwise, TableView.Dispose needs to perform a type check on its table field against IDisposable (which should succeed so long as a public instance Dispose method exists), if ownership has been declared by the user, because TableView technically only has to own an ITableSource, which doesn't explicitly implement IDisposable, but which is compatible with DataTable, along with the fact that DataTableSource is there and owns a DataTable.

Another couple of thoughts that occur to me based on how these property setters on various View types work, which I think are valuable for consideration on future work and which I might file an issue for at some point:

Any of the View types that are inherently collection containers, such as ListView, TableView, TreeView, etc, would benefit greatly from implementing INotifyCollectionChanged, and all Views should probably implement INotifyPropertyChanged (along with those interfaces' relevant implicit expectations).

That would also provide a common/consistent execution flow for things like calling various layout/update methods, and allow consumers (both internal and external) who subscribe to those events to inject code they need right before those things happen, in a controlled location (from Terminal.Gui's perspective), so that things can be more consistent across the entire library and provide consumers a relatively high degree of predictability for relatively "low effort" here (those air quotes may be doing some heavy lifting).

[tznind]

TableView is not responsible for disposing DataTable. Neither is DataTableSouce.

In both cases the collection is 'provided to' the class.

I would say that if you don't 'new' an object then you shouldn't dispose it.

Client code gets the table (e.g. from database or constructs it) so they should themselves dispose the table.

You could for example use the same System.DataTable in 2 views Or want to process the table data after the user has modified it in a modal UI.

I don't think adding a dispose bool helps much. If rather just document that it will never dispose and leave that to consumer

On Wed, 10 Jan 2024, 12:42 dodexahedron, @.***> wrote:

Actually....

This is more about DataTableSource, I suppose, since it's the type that explicitly MUST own a DataTable.

But the inheritance and ownership tree would make it a LOT easier for Terminal.Gui if the ITableSource interface required IDisposable.

Otherwise, TableView.Dispose needs to perform a type check on its table field against IDisposable (which should succeed so long as a public instance Dispose method exists), if ownership has been declared by the user, because TableView technically only has to own an ITableSource, which doesn't explicitly implement IDisposable, but which is compatible with DataTable, along with the fact that DataTableSource is there and owns a DataTable.

Another couple of thoughts that occur to me based on how these property setters on various View types work, which I think are valuable for consideration on future work and which I might file an issue for at some point:

Any of the View types that are inherently collection containers, such as ListView, TableView, TreeView, etc, would benefit greatly from implementing INotifyCollectionChanged, and all Views should probably implement INotifyPropertyChanged (along with those interfaces' relevant implicit expectations)

— Reply to this email directly, view it on GitHub https://github.com/gui-cs/Terminal.Gui/issues/3152#issuecomment-1884779490, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHO3C5AY3AX3XPZXUG3PFH3YN2EB3AVCNFSM6AAAAABBUWBFM2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBUG43TSNBZGA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[dodexahedron]

I agree in the general case about not Disposing what you don't create, as that's generally just proper practice.

That's why I proposed maybe implementing the pattern that streams do, where you tell your object at creation time if it should consider itself the owner of its dependency, since that's already an established and familiar pattern. And that would be optional, with default false, of course, which makes it a non-chsnge for existing uses but opens up much simpler life cycles for when one does specify ownership, especially since a user is quite likely to be treating things as essentially a databinding, and not wholesale replacing a datatable every time they change things.

Which then leads to the other part, though the two points aren't directly related. I think the property change notifications are actually more valuable here, and both providing the requisite events and the facilities to react to them would be a MAJOR win and also fit well into existing patterns consumers may be familiar with from WPF and WinUI. Helps to standardize various operations and avoid boilerplate.

Oh and back on disposal, yes, documentation of the behavior is also a perfectly acceptable solution, and certainly the simplest, on the TG side. I'd suggest both remarks in xmldoc as well as appropriate verbiage in whatever more formal docs exist or are created for it.

[tznind]

I don't think table view should dispose datatable.

I have read the suggestion but disagree as it is adding complexity where it isn't needed.

TableView is a ui component. It has a source. The source can be a DataTable wrapper. That's already 2 levels of indirection from the DataTable. I think it should never dispose that table.

[tznind]

As discussed, I think we agree there is not clear motive to do this so closing.