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 (with ListTableSource) #2593

Closed

Conversation

Nutzzz
Copy link
Contributor

@Nutzzz Nutzzz commented May 2, 2023

Note this also duplicates PR #2591 .

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

…ned view

Also adds MinCellWidth to TableView
@tznind
Copy link
Collaborator

tznind commented May 3, 2023

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.

Thanks for taking the time to continue exploring this idea. You can pass a TableView into the constructor of your ListTableSource. I think it would be much cleaner code if you were able to use ITableSource directly and dispense with the DataTable entirely.

I added the ITableSource interface so we could avoid subclassing TableView or extending its codebase to the point it is hard to maintain.

Let me know what you think of this idea. I am happy to help with it further if you would like me to elaborate on this pattern.

We can make methods public on TableView or internal so if you need things like GetHeaderHeight we can change it from private to internal.

Something like:

public class ListTableSource : ITableSource {

		public IList List;

		private TableView tableView;

		/// <summary>
		/// Creates a new table instance based on the data in <paramref name="list"/>.
		/// </summary>
		/// <param name="list"></param>
		/// <param name="tableView"></param>
		public ListTableSource (TableView tableView, IList list)
		{
			this.tableView = tableView;
			this.List = list;

			// TODO: I think this is what FlowTable method currently does
			Columns = CalculateColumns();

			Rows = (int)Math.Ceiling (list.Count / (float)Columns);

		}

		/// <inheritdoc/>
		public object this [int row, int col]
		{
			get {
				var idx = (row * Columns) + col;
				if(idx >= List.Count) {
					return null;
				}
				return List [idx];
			}
		}

		/// <inheritdoc/>
		public int Count => List.Count;

		/// <inheritdoc/>
		public int Rows { get; private set; }

		/// <inheritdoc/>
		public int Columns { get; private set; }

		/// <inheritdoc/>
		public string [] ColumnNames => Enumerable.Range (0, Columns).Select (n => n.ToString ()).ToArray();

[...]
}

@Nutzzz
Copy link
Contributor Author

Nutzzz commented May 3, 2023

Thanks for taking the time to continue exploring this idea. You can pass a TableView into the constructor of your ListTableSource. I think it would be much cleaner code if you were able to use ITableSource directly and dispense with the DataTable entirely.

Thank you for your help. In my first try after #2576 and based on your comments, I tried to do away with the subclass and make as few changes to TableView as possible. Initially I was passing just the necessary aspects of the View (rather than the View itself) into ListTableSource's constructor. But I still needed to add the orientation styles (direction to populate values, and direction to scroll) to TableView, and more importantly there's the fact that some (but not all) changes to the table's style or other properties (e.g., the View bounds or MaxCellWidth) require a reflow.

@tznind
Copy link
Collaborator

tznind commented May 3, 2023

Initially I was passing just the necessary aspects of the View (rather than the View itself) into ListTableSource's constructor

I think passing the whole TableView in is fine and that is what I have done in #2589 (adding checkboxes). If it makes life easier and code more readable just do it!

[...] I still needed to add the orientation styles (direction to populate values, and direction to scroll) to TableView

ListColumnStyle could be a property on the ListTableSource couldn't it?

and more importantly there's the fact that some (but not all) changes to the table's style or other properties (e.g., the View bounds or MaxCellWidth) require a reflow.

I think a valid approach to this problem would be to register an event? what do you think? For example:

+ ListColumnStyle style;
private TableView tableView;

/// <summary>
/// Creates a new table instance based on the data in <paramref name="list"/>.
/// </summary>
/// <param name="list"></param>
/// <param name="tableView"></param>
- public ListTableSource (TableView tableView, IList list)
+ public ListTableSource (TableView tableView, IList list, ListColumnStyle style)
{
	this.tableView = tableView;
	this.List = list;
+	this.style = style;

	// TODO:
	Columns = CalculateColumns;
	Rows = (int)Math.Ceiling (list.Count / (float)Columns);

+	// TODO: Depending on exactly when this event is called 
+	// it may be 'too late' (view may have already been drawn).
+	// Not sure what the exact best event to tap for this is but
+	// this is the general idea.

+	// And if there isn't a good event for it we can add one!
+	tableView.DrawContent += TableView_DrawContent;

}

+ private Rect lastBounds;
+ private int lastMaxCellWidth;
+ private void TableView_DrawContent (object sender, DrawEventArgs e)
+ {
+ 	if(
+ 		(!tableView.Bounds.Equals(lastBounds)) ||
+ 		tableView.MaxCellWidth != lastMaxCellWidth /* ||
+ 		TODO add other conditions here */) {

+ 		Columns = CalculateColumns ();
+ 	}
+ 	lastBounds = tableView.Bounds;
+ 	lastMaxCellWidth = tableView.MaxCellWidth;
+ }

@Nutzzz
Copy link
Contributor Author

Nutzzz commented May 7, 2023

Closing this in favor of #2603 .

@Nutzzz Nutzzz closed this May 7, 2023
@Nutzzz Nutzzz deleted the tableview_listcolumns_newclass2 branch May 10, 2023 15:15
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.

2 participants