Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #16 - Add columned lists to TableView #2573

Closed

Conversation

Nutzzz
Copy link
Contributor

@Nutzzz Nutzzz commented Apr 25, 2023

Add ListColumnView (child of TableView), displays an IList in a columned view

As I mentioned in #2514 , I wanted to make it easier to flexibly create multi-column lists. This is what I put together. I still need to add Unit Tests and re-add sorting to the Scenario, but I wanted to see if this solution makes sense to you guys.

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

@Nutzzz
Copy link
Contributor Author

Nutzzz commented Apr 26, 2023

Though ListColumnView uses a lot from TableView, such that it would be highly duplicative to parent directly to View, there's a fair bit that doesn't apply as well, such that I imagine it would be confusing to use in practice. Should I perhaps create a new parent GridView that they can both share?

@tznind
Copy link
Collaborator

tznind commented Apr 26, 2023

I love the idea of supporting IList as a data provider for TableView.

I can see that your approach is viable where you are subclassing and converting to System.Data.DataTable. But I think that a better approach might be to implement this by refactoring TableView to use a new interface ITableDataSource instead of System.Data.DataTable. That way there would be only one class TableView and two data source implementations:

  • SystemDataTableSource
  • ListTableSource

The user would be free to write their own too e.g. TransposedListTableSource (as you have in your PR). Interface would be something like

interface ITableDataSource
{
    int Rows { get; }
    int Columns { get; }
    string[] ColumnNames { get; }
    object this[int row, int col]
    {
        get;
    }
}

This will be breaking but we are working on v2 I think we should consider it as its a nice feature and it is quite a common use case (to use arbitrary objects as the table data items).

That is the approach taken in TermKit and looks like it will make TableView much more flexible:

/// Protocol that describes a matrix made up of columns and rows, where the columns can be labeled
public protocol DataSource {

https://github.com/migueldeicaza/TermKit/blob/main/Sources/TermKit/Views/DataTable.swift

The Goal

The more I look at it, the more I'm excited about the idea.

Check this out! How cool would it be for an API user to be able to do something like:

var s = new EnumerableTableDataSource<Process>(Process.GetProcesses(),
    new()
    {
        { "Name",(p)=>p.ProcessName},
        { "Id",(p)=>p.Id},
        { "HasExited",(p)=>p.HasExited},
    });

myTableView.Table = s;

The EnumerableTableDataSource<T> class definition would be something like:

class EnumerableTableDataSource<T> : ITableDataSource
{
    private T[] data;
    private string[] cols;
    private Dictionary<string, Func<T, object>> lamdas;

    public EnumerableTableDataSource(IEnumerable<T> data, Dictionary<string, Func<T,object>> columnDefinitions)
    {
        this.data = data.ToArray();
        this.cols = columnDefinitions.Keys.ToArray();
        this.lamdas = columnDefinitions;
    }

    public object this[int row, int col]
    {
        get => this.lamdas[ColumnNames[col]](this.data[row]);
    }

    public int Rows => data.Length;

    public int Columns => cols.Length;

    public string[] ColumnNames => cols;
}

@tig
Copy link
Collaborator

tig commented Apr 26, 2023

Love!

@Nutzzz
Copy link
Contributor Author

Nutzzz commented Apr 26, 2023

I do have another branch where I first used TableView directly (though in my quick solution Table and ListData could be separately assigned to--your idea makes better sense). However, there are public methods and fields that make sense in only one or the other use case. From the point of view of a TableView, the few that I added certainly, but from the point of view of a ListColumnView, it no longer makes sense to have headers or FullRowSelect or TableSelection or otherwise treat any particular row or column differently from another. Maybe that's OK, but it bears some thought.

@tznind
Copy link
Collaborator

tznind commented Apr 26, 2023

I do have another branch where I first used TableView directly (though in my quick solution Table and ListData could be separately assigned to--your idea makes better sense). However, there are public methods and fields that make sense in only one or the other use case. From the point of view of a TableView, the few that I added certainly

Ok I'll pull my refactoring idea above out into a separate issue.

but from the point of view of a ListColumnView, it no longer makes sense to have headers or FullRowSelect or TableSelection or otherwise treat any particular row or column differently from another. Maybe that's OK, but it bears some thought.

Ah yes I see. Your view is almost like a Layout. You could imagine that a View layout grid could be used to do something like this?

Currently we don't have 'flow' , 'stack' or 'grid' style layouts.

TableView is designed very carefully for scaling to hundreds of columns and/or millions of rows. So there are things that it does at render from a given row/column offset and querying/styling only until the console runs out of space.

So for example it would be very difficult to modify TableView to support multi row cells.

I've not really got a point here just adding some background I guess.

@Nutzzz
Copy link
Contributor Author

Nutzzz commented Apr 26, 2023

Ah yes I see. Your view is almost like a Layout.

That's right, though leveraging TableView's scalability is still important to the use case I'm attempting here. I'm imagining this could be used, for instance, to make the FileDialog have an alternate "list view" layout (rather than the "detail view" used currently), using limited screen real estate more effectively when you're only concerned with looking through a large number of filenames.

Currently we don't have 'flow' , 'stack' or 'grid' style layouts.

My idea wasn't to add a GridView in the sense of hosting Views within given cells, though that would certainly be cool.

So for example it would be very difficult to modify TableView to support multi row cells.

Yes, an ability to "word wrap" would be interesting... however, you'd have to add horizontal cell lines, which cuts the opposite way in terms of best using screen real estate. On the other hand, I suppose you could use underlines and/or something like the throughline header I added to #2574 ... or alternating colors? In any case, that's also beyond what I was attempting here.

@Nutzzz
Copy link
Contributor Author

Nutzzz commented Apr 26, 2023

Ok I'll pull my refactoring idea above out into a separate issue.

I think that still makes sense for this PR, I'm just questioning what should reside within TableView or a new parent.

@tig
Copy link
Collaborator

tig commented Apr 26, 2023

It is shockingly eerie that just yesterday I was thinking about a "wrap" layout mode.

I was looking at the "ColorPocker" code and thinking I wanted it to support arbitrary rows/cols. "this could all be just a list of radio buttons with no text; it'd be nice of we had a 'word wrap these Views' mode".

@tig
Copy link
Collaborator

tig commented May 1, 2023

Closing as fixed by @tznind in #2576

@tig tig closed this May 1, 2023
@Nutzzz
Copy link
Contributor Author

Nutzzz commented May 2, 2023

@tig : This is not fixed. This was about generating an automatic layout based on the viewport size rather than just being able to use a list to create a one-dimensional table.

Obviously it needed to change as a result of #2576 , but since an implementation of ITableSource doesn't have any information about the view, I still have to either add methods to TableView or create a child class in addition to adding a ListTableSource. FWIW, I'll open a new PR [ #2593 ] that incorporates @tznind 's changes, and you can decide what to do with it.

@tig
Copy link
Collaborator

tig commented May 4, 2023

Got it. I was confused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants