Add Column objects to Tree and Table public API

[corranwebster]

This is an initial cut at making Column objects part of the public API for Tree and Table objects.

This PR:

• adapts the Tree and Table APIs to use ColumnT protocol objects in the place of column headings and accessors.
• adds a read-only columns property with the current columns
• preserves backwards compatibility for the old API
• adds a show_headings argument gives more direct control over whether table headers are shown
• makes some changes to the internal Tree and Table implementation API, passing a column object rather than heading and accessor (and keeps backwards compatibility with any 3rd-party implementations)
• adds a basic Column ABC which is more appropriate for creating simple column subclasses than AccessorColumn
• adds equality tests and hashability for AccessorColumn.
• documentation for Columns
• an example showing different ways to use columns

Fixes #4091.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[corranwebster]

I think tests should pass now, so this should be ready for review.

[freakboy3742]

A partial review to get you unstuck.

I've picked up a couple of documentation issues, and I've pushed some minor updates to the table_columns example app.

The two big questions for me at this point are about compatibility with the older API.

First, should we retain the accessors argument? It needs to continue to exist for the short term for backwards compatibility purposes, but given that it is a very narrow interpretation of what can be done with a column, and there's a better way to express the idea using AccessorColumn... is there any reason to continue to allow accessors as an long term solution?

Second - I'm also wondering if missing_value should be moved into Column responsibility. It doesn't seem out of the question that a "missing" value might be column-dependent (e.g, "missing" might be best represented by a different icon, or "Unknown", or a sentinel date, depending on the data type and context - and all in the same table).

[freakboy3742]

Suggested change

	[tool.briefcase.app.table.macOS]
	[tool.briefcase.app.table_columns.macOS]

Rinse and repeat for other platform tables below.

[freakboy3742]

There's an implied "how" follow up question here. Longer term, a topic guide or howto might be useful. In the short term, pointing out that a custom column can be any ColumnT-satisfying class is probably sufficient.

[corranwebster]

A partial review to get you unstuck.

I've picked up a couple of documentation issues, and I've pushed some minor updates to the table_columns example app.

Thanks.

The two big questions for me at this point are about compatibility with the older API.

First, should we retain the accessors argument? It needs to continue to exist for the short term for backwards compatibility purposes, but given that it is a very narrow interpretation of what can be done with a column, and there's a better way to express the idea using AccessorColumn... is there any reason to continue to allow accessors as an long term solution?

It remains needed for the case where it gets passed through to the ListSource or TreeSource for handling rows which are sequences, particularly when you have more values in the row than columns, or you want to be able to add more columns later.

I'd be happy to get rid of it in the long-term, but we'd need some alternative to cover this case (like a "data source factory" argument).

Second - I'm also wondering if missing_value should be moved into Column responsibility. It doesn't seem out of the question that a "missing" value might be column-dependent (e.g, "missing" might be best represented by a different icon, or "Unknown", or a sentinel date, depending on the data type and context - and all in the same table).

It would make some sense, but a custom column has complete control over what to do if it can't determine a valid value - it's got the suggestion of a default passed in from the widget, but a custom column doesn't have to follow that suggestion if it's got a better idea.

We could add various default values as arguments to AccessorColumn that override the widget-supplied missing value, but it's also possibly worth waiting a bit to see how things evolve and are used before adding those.

[freakboy3742]

is there any reason to continue to allow accessors as an long term solution?

It remains needed for the case where it gets passed through to the ListSource or TreeSource for handling rows which are sequences, particularly when you have more values in the row than columns, or you want to be able to add more columns later.

🤦 Despite this being a subject of recent conversation, I completely missed this as the retained use case for accessors.

On that basis, I agree we can't remove it - at least, not without some other changes.

The thing that is jarring for me is that there are essentially two use cases for accessors - an initial mechanism for describing which data fills which column; and the ordering that is used to resolve list-based data into Rows in a ListSource.

The column mapping can be (and in the simple case, will be) described by the column headings; and there's a clear path to "deprecate accessors and encourage the use of AccessorColumn".

But the ordering that is used to resolve list-based data into Rows is:

1. Only used if the input data is an iterable that is non-mappable
2. Cannot be changed after original creation of the widget

Those two properties strike me as odd - there's something you can define, but can't change, even though there's an argument with the same name elsewhere in the API; but the property won't even be used in many cases.

I'd be happy to get rid of it in the long-term, but we'd need some alternative to cover this case (like a "data source factory" argument).

I can see what you're describing here - but I suspect it's going to end up being most complex for the very simple case that needs it - "A table display a list of tuples" is the simple case here; defining a "factory" for the simple case seems like overkill.

I wonder if maybe it is as simple as:

1. Deprecating accessors as an argument everywhere that it is used, in preference for user-defined AccessorColumn instances for the case where a simple heading mapping doesn't work
2. Introducing a data_order argument (open to suggestions on naming) that fills the role that _data_accessor_order currently has - except that it's settable as an end user. If undefined, it assumes the value it currently takes based on heading titles. data_order must be a list, and is only used to resolve values for data rows.

I think that retains all the existing functionality; but it retains separation between the two use cases for accessors. Does that make sense?

Second - I'm also wondering if missing_value should be moved into Column responsibility.
...
We could add various default values as arguments to AccessorColumn that override the widget-supplied missing value, but it's also possibly worth waiting a bit to see how things evolve and are used before adding those.

That's fair. Having a widget-level default that is overridden by a Column-level default that is overridden by an actual value makes sense; and it also strikes me as something that can be added in the future if it turns out it might be useful.

[corranwebster]

I wonder if maybe it is as simple as:

1. Deprecating accessors as an argument everywhere that it is used, in preference for user-defined AccessorColumn instances for the case where a simple heading mapping doesn't work
2. Introducing a data_order argument (open to suggestions on naming) that fills the role that _data_accessor_order currently has - except that it's settable as an end user. If undefined, it assumes the value it currently takes based on heading titles. data_order must be a list, and is only used to resolve values for data rows.

I think that retains all the existing functionality; but it retains separation between the two use cases for accessors. Does that make sense?

I think that, or some variation on it, may work.

But stepping back a bit, I think the general guide is that after all of this, we still want users to be able to do something like

table = toga.Table(
    columns=["Name", "Age"],
    data=[
        ("Arthur Dent", 42),
        ("Ford Prefect", 37),
        ("Tricia McMillan", 38),
    ]
)

and it Just Works. But when things get a bit more complex there's a clear path to resolve the complexity. The complexity we're running into here is essentially what happens when we have:

data=[
    ("Arthur Dent", "Earth", 42),
    ("Ford Prefect", "Betelgeuse Five", 37),
    ("Tricia McMillan", "Earth", 38),
]

and the user still wants name and age columns. The current solution is:

table = toga.Table(
    columns=["Name", "Age"],
    accessors=["name", "planet", "age"],
    data=[
        ("Arthur Dent", "Earth", 42),
        ("Ford Prefect", "Betelgeuse Five", 37),
        ("Tricia McMillan", "Earth", 38),
    ]
)

(Edit: actually we also need columns=[AccessorColumn("Name", "name), AccessorColumn("Age", "age")] as well)
but this discussion is making me wonder if the "right" solution when you get this complexity is actually:

table = toga.Table(
    columns=["Name", "Age"],
    data=ListSource(
        accessors=["name", "planet", "age"],
        data=[
            ("Arthur Dent", "Earth", 42),
            ("Ford Prefect", "Betelgeuse Five", 37),
            ("Tricia McMillan", "Earth", 38),
        ],
    ),
)

ie. "the data became more complex, so the data source definition has be more complex" is better than "the data became more complex, so the table widget has be more complex"

This approach would mean:

• deprecate accessors completely from the table and tree widgets: they are a data concern not a widget concern, a matter for sources and columns to deal with.
• when someone does something like table.data = [... list of tuples ...] we create a ListSource/TreeSource and try:
  • copying the accessors from the previous source
  • failing that regenerate the accessors from the current columns (this should only happen if a user has a custom data source which doesn't use accessors, and then switches to an ad-hoc ListSource, which is already a fairly niche edge case; it might be OK to simply raise an exception in this situation)

In other words, we rely on the data sources to track the order of the accessors, rather than trying to do it in the widget. And if that doesn't do what the user wants, the fix is for the user to explicitly use a ListSource/TreeSource with the exact accessors they need.

This is sort of an alternative fix for #4071 and is a slight behaviour change from the current state of the main branch (behaviour should be the same for the case where the user never sets data explicitly to a Source), but I think it would be OK from a backwards compatibility point of view since we haven't yet had a release which fixes #4071.

[corranwebster]

I think this is ready for a re-review: I've addressed all the comments, but there is one issue remaining which may not need to be addressed in this PR, but should be thought about: currently ListSource and TreeSource objects will raise if they get passed an empty list of accessors, but as long as the rows are provided to them in dictionary form, there is no need for the accessors so in some cases they may be superfluous.

In any case, what this means is that if a Table is created without any columns or data initially, this will cause an exception. It may also occur if all the columns are custom columns without an accessor attribute. There are work-arounds involving creating an empty data source manually with some dummy accessors, but it seems to me that we might be able to drop the requirement and only error if the user doesn't supply accessors and then tries to use them.

[freakboy3742]

In other words, we rely on the data sources to track the order of the accessors, rather than trying to do it in the widget. And if that doesn't do what the user wants, the fix is for the user to explicitly use a ListSource/TreeSource with the exact accessors they need.

This is sort of an alternative fix for #4071 and is a slight behaviour change from the current state of the main branch (behaviour should be the same for the case where the user never sets data explicitly to a Source), but I think it would be OK from a backwards compatibility point of view since we haven't yet had a release which fixes #4071.

+1 to this. It gives good conceptual separation of data concerns and display concerns; and we don't need to worry about backwards compatibility with code we haven't formally released.

I think this is ready for a re-review: I've addressed all the comments, but there is one issue remaining which may not need to be addressed in this PR, but should be thought about: currently ListSource and TreeSource objects will raise if they get passed an empty list of accessors, but as long as the rows are provided to them in dictionary form, there is no need for the accessors so in some cases they may be superfluous.

Yeah - I think we can handle that separately. Allowing a ListSource/TreeSource to be created with no accessors, and then raising an exception if you try to use data that requires accessors would make sense to me.

[freakboy3742]

I've pushed some minor updates - mostly stylistic things in docs. The only thing of any substance is catching the case of using accessors in insert_column() and append_column() calls and raising a DeprecationWarning; but otherwise, this looks like a really solid improvement to the Table/Tree API. Thanks for all your work on this!