Column renderers #1478

samschott · 2022-05-01T19:04:31Z

This PR is an initial set of changes to introduce a new Column widget which represents a Table or Tree column. Having a Column widget has multiple advantages:

It provides a single API to generate interactive columns. Currently supported are text, icons and checkboxes.
It breaks with the current one-to-one mapping between data source fields and columns:
- Multiple fields of a data source can be used in a column, for example for text and an icon.
- A field in a data source can be shown in multiple columns.
Changes to the data through the GUI can more easily be propagated to the data source.
It allows for column-specific styling such as width, color, font, etc. This is currently not implemented.

This PR lays the groundwork by introducing the new Column widget and using it in internally only. The public APIs of toga.Table and toga.Tree remain unchanged for now, apart from exposing the created columns in a new property. This means that current benefits mostly lie in cleaner implementations of some functionality.

Support in different backends for the new column features varies:

Cocoa and Gtk: Support text, icon and checkbox.
Winforms and Android: Supports text only. Other column properties will be ignored.

Next steps will be:

Allow passing Columns instead of headers and accessors when instantiating a new Table or Tree. Possibly move towards deprecating accessors and headers arguments?
Support column styling.
Support icons in the first table / tree column on Windows. AFAIK, winforms does not allow icons in other columns.

This PR supersedes #1085.

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

samschott · 2022-05-01T19:49:42Z

src/android/toga_android/widgets/tablecolumn.py

+
+
+class Column(Widget):
+ def create(self):


Note that this is still a no-op. I'll need to become more familiar with android APIs to do anything useful here

samschott · 2022-05-02T09:37:53Z

Converted to draft since handling of "missing values" is not yet clear. The current Column API can easily incorporate a placeholder for missing text but it the behaviour for missing icon or checkbox data needs to be better defined.

samschott · 2022-05-02T14:39:01Z

I've added hopefully reasonable fallbacks now when no data is available:

Text: Falls back to provided missing_value.
Icon: Falls back to toga icon.
CheckBox: Falls back to showing no checkbox at all.

* Don't test removed accessor attribute * Allow duplicate accessors

freakboy3742 · 2022-05-18T05:27:13Z

@samschott FYI - I've seen this, and I promise I'll take a look soon; I'm currently knee deep in M1, Android, and Py3.11 updates; once I surface from those, this is high on my list.

freakboy3742

I've finally cleared the decks of underlying platform support work, so I've had a chance to dig into this PR. My overall impression is that it's heading in the right direction, with some notable gaps and tweaks required (some of which you've flagged already). I've flagged a couple more inline; but some overall comments:

We definitely need something in the examples that shows off the checkbox and icon functionality.
I can see what you've done exposing the Column as a widget on the impl layer so it can be exposed by the factory. However, that means there's a loss of parity between the interface and impl layers. I wonder if it might make more sense to expose an interface on the Table/Tree widget impl that acts as a factory for Column Impls?
You've flagged the need to provide an API to configure the list of columns at time of construction. I can see how that could enable us to deprecate headings and accessors; my hesitation is the extent to which that complicates the "simple" case of "show a table based on this list of lists".
The only thing that feels slightly off in the overall design the interface between the Source and the widget. This manifests in 2 ways. The first is the get/set_data_for_node() API. While this definitely works, the use of a getter/setter doesn't feel very Pythonic; the obvious alternative would be an object following the descriptor protocol - but then we're getting very close to the API that Source provides. That leads into the second manifestation - the fact that when Source is constructed, it needs to have accessors... that are taken from the Column. I'm not sure I have a concrete counterproposal at this point - but it definitely feels like there's some friction in the way we're handling data access that could be avoided with a slightly different API design.

freakboy3742 · 2022-05-31T02:48:38Z

src/cocoa/toga_cocoa/widgets/internal/cells.py

+ send_super(__class__, self, "init")
+ self.imageView = NSImageView.alloc().init()
+ self.checkbox = NSButton.alloc().init()
+ self.textField = NSTextField.alloc().init()


How significant is the overhead of defining the image and checkbox widgets for every cell, and adding them to the layout, even if they're not used?

That's a good question. In my experience, the overhead has been small since in practice only the number of TogaTableCellView instances that are actually visible on screen will be created (maybe a few more than that) and then reused when scrolling.

We might gain a bit by lazily allocating images and checkboxes as needed while slightly complicating the logic.

freakboy3742 · 2022-05-31T02:50:31Z

examples/tree_source/tree_source/app.py

@@ -75,7 +75,7 @@ def can_have_children(self):
 # Property that returns the first column value as (icon, label)
 @property
 def name(self):
- return self._icon, self.path.name


Comment is now out of date; plus, the demo has now lost the icons.

Indeed. It will regain the icons once the API for that is set.

freakboy3742 · 2022-05-31T03:22:14Z

src/core/toga/widgets/internal/tablecolumn.py

+ self.text = text
+ self.icon = icon
+ self.checked_state = checked_state
+ self.editable = editable


+1 to making these attributes customisable... but are we actually using this capability anywhere? AFAICT, the implementation currently hard-codes all the references to row/node attributes.

We are not, really. New data will be fetched from the source when refreshing the table or when new rows scroll into view. At this point, changes to these accessors will be visible in the widget. We'd probably want setters here which force a reload.

freakboy3742 · 2022-05-31T03:22:44Z

src/core/toga/widgets/internal/tablecolumn.py

+ Args:
+ node (``Node`` or ``Row``): A Node in a TreeSource or a Row in a ListSource
+ that holds the data for this row.
+ role (str): The type of data to query. Can be "text", "icon" or "checked_state".


We need to differentiate between the specific role, and the attribute on the data source that may be providing the data for that role. We should introduce constants or an enum for these defined roles.

Good point.

freakboy3742 · 2022-05-31T03:24:45Z

src/core/toga/widgets/internal/tablecolumn.py

+from toga.widgets.base import Widget
+
+
+class Column(Widget):


There's an inconsistency between the file name and class name here

freakboy3742 · 2022-05-31T05:17:31Z

src/cocoa/toga_cocoa/widgets/tree.py


- return max(heights)
+ # @objc_method


We should either preserving the comment around why this is commented out... remove the block entirely

This is indeed no longer needed since this PR removes support for showing arbitrary widgets in a column.

samschott · 2022-05-31T21:08:08Z

Thanks for having a look at this!

Regarding the constructor API, I'm still searching for a solution that keeps the simplicity while enabling customisation. All while maintaining backward compatibility. Once I have that, I'll update the examples to make use of it. Regarding the Columns in the Impl layer, I like the idea of exposing an interface on the Table/Tree widget to act as a factory.

Now to the lager last point. The main goals of what a Column is supposed to do:

Hold the style definition of a column and possible other properties (resizable, sortable, etc). This is still to do.
Provide a mapping between fields in the data source and what is visible in a table column. This mapping also associates a column-specific role with each field, e.g., use the field value as icon path, text, etc. This info should live with the Column instead of the data source (as was the case with the icon, text tuple hack), because the same data might be displayed in different ways by different tables. 3.

In this sense, the Column tries to do something similar to the source: It provides access to data. However, while the data sources are essentially a "list" or a "tree" of data classes with a notification mechanism, the Column does not provide a data structure at all but only a mapping from fields of a data class to "something to display" (including styles, at some point...). With that in mind, I'm not so concerned about the manifestations which you describe:

The first is the get/set_data_for_node() API.

I agree that there are more elegant and Pythonic ways of solving this. Let me see what I can do here.

That leads into the second manifestation - the fact that when Source is constructed, it needs to have accessors... that are taken from the Column.

The data source already required accessors in the constructor previously. This is just a necessity when "creating" attributes of a Row / Node dynamically. However, why do you say that the accessors are taken from the Column? The opposite is happening: accessors are defined for data source. The Column is then told which accessor it should use to fetch the data from the source for each type of widget that it supports.

In the current implementation, this admittedly takes a bit of a roundabout way: Because the constructor only supports headings and accessors, we cannot differentiate between text and other ways to display data. Therefore, the accessors are always interpreted as text accessors in the constructor. When the data source is actually initialised from whatever data type was provided as input, I just read back the accessors from the Column instead of storing them in a separate variable in Table.__init__().

freakboy3742 · 2022-06-01T00:41:40Z

Thanks for having a look at this!

Regarding the constructor API, I'm still searching for a solution that keeps the simplicity while enabling customisation. All while maintaining backward compatibility. Once I have that, I'll update the examples to make use of it. Regarding the Columns in the Impl layer, I like the idea of exposing an interface on the Table/Tree widget to act as a factory.

FWIW: Backwards compatibility is highly desirable, but negotiable at this point. I've been holding off formally cutting v0.3 specifically so we have the flexibility to break compatibility if we need to. A clean forward path is always preferable, but if a good API design exists that isn't backwards compatible, I wouldn't rule it out.

Now to the lager last point. The main goals of what a Column is supposed to do:

Hold the style definition of a column and possible other properties (resizable, sortable, etc). This is still to do.
+1

Provide a mapping between fields in the data source and what is visible in a table column. This mapping also associates a column-specific role with each field, e.g., use the field value as icon path, text, etc. This info should live with the Column instead of the data source (as was the case with the icon, text tuple hack), because the same data might be displayed in different ways by different tables.

Agreed.

In this sense, the Column tries to do something similar to the source: It provides access to data. However, while the data sources are essentially a "list" or a "tree" of data classes with a notification mechanism, the Column does not provide a data structure at all but only a mapping from fields of a data class to "something to display" (including styles, at some point...).

I think we're on the same page here - but to restate to make sure:

The Source is a collection of raw data. It's structured in the sense that there is order in the items (rows/nodes) being stored, and each item has attributes.
The Column describes a mapping between the attributes of each item, and the content (and mode of editing, if appropriate) of cells in that column.

With that in mind, I'm not so concerned about the manifestations which you describe:

The first is the get/set_data_for_node() API.

I agree that there are more elegant and Pythonic ways of solving this. Let me see what I can do here.

That leads into the second manifestation - the fact that when Source is constructed, it needs to have accessors... that are taken from the Column.

The data source already required accessors in the constructor previously. This is just a necessity when "creating" attributes of a Row / Node dynamically. However, why do you say that the accessors are taken from the Column? The opposite is happening: accessors are defined for data source. The Column is then told which accessor it should use to fetch the data from the source for each type of widget that it supports.

In the current implementation, this admittedly takes a bit of a roundabout way: Because the constructor only supports headings and accessors, we cannot differentiate between text and other ways to display data. Therefore, the accessors are always interpreted as text accessors in the constructor. When the data source is actually initialised from whatever data type was provided as input, I just read back the accessors from the Column instead of storing them in a separate variable in Table.__init__().

Yeah - it's the "roundabout way" part that wasn't sitting right with me. The creation of the column itself (final API notwithstanding) makes sense - Table(columns=[Column('name'), Column('age'), Column('available', attr='is_available')]) is broadly an API that maps to what the end user is building, and with the concept of mapping columns to attributes of each item in the data. It's the process of building the additional mapping when using a "simple" data source that doesn't sit right.

Thinking about it some more, I suspect the root of the problem may actually lie in Source, and the way _create_row is being used. The only reason the list source needs to know about the accessors at all is to convert data that is in a simple list form into a dictionary that is compatible with Row.

There are essentially 4 use cases that Table/Tree needs to support for passing in data:

List of tuples/lists: data = [('a', 1), ('b', 2)]
List of dicts: data = [{'name': 'a', 'size': 1}, {'name': 'b':, 'size': 2}]
List of custom objects: data = [MyObj(name='a', size=1), MyObj(name='b', size=2)]
Custom data source: data = MySource(...)

Of those, it's only case (1) that needs to know about the columns - and that's only so the source can convert form (1) into something closer to form (2) so that the column can use attribute lookup. It might make sense to do that conversion on the Table/Tree side, rather than pushing it down to the Source.

samschott · 2022-06-04T17:11:13Z

Ok, in that case I think we are agreed :)

Regarding the types of data that you want to accept as input, the third case of a list of objects does not currently seem to be supported, though I agree that it should be. Passing arbitrary objects to a ListSource currently assigns the entire object to the first accessor. However, it would probably be more useful if the objects would replace the Rows in a List source.

In case of a TreeSource, it's a bit more difficult because we need to support the concept of child Nodes, which objects that are passed in might not have.

Edit: Maybe replacing Rows with custom objects is not a good idea after all, the current mechanism of notifying the table of changes to the data is baked in to the predefined toga.sources.Row.

freakboy3742 · 2022-06-07T04:43:55Z

Edit: Maybe replacing Rows with custom objects is not a good idea after all, the current mechanism of notifying the table of changes to the data is baked in to the predefined toga.sources.Row.

We may not be able to entirely replace Row with custom objects - but it should be possible for Row to use a custom object as its data object (instead of copying the data into a dictionary).

samschott · 2022-06-07T21:38:04Z

We may not be able to entirely replace Row with custom objects - but it should be possible for Row to use a custom object as its data object (instead of copying the data into a dictionary).

Fair point. I'd rather defer this to another PR though, it's a bit off-topic for this one and has some subtleties involved. For example: Do we want to proxy all attribute access for those objects? How do we handle type checking when creating the ListSource? In principle, even an integer is an object with attributes such as real and imag which could be accessors for data to show in a table. Where do we draw the line?

require dicts to be passed instead

codecov · 2022-06-07T22:00:08Z

Codecov Report

Merging #1478 (62566a7) into main (400b693) will increase coverage by 0.06%.
The diff coverage is 96.77%.

❗ Current head 62566a7 differs from pull request most recent head 6690292. Consider uploading reports for the commit 6690292 to get more accurate results

Impacted Files	Coverage Δ
src/core/toga/widgets/internal/tablecolumn.py	`94.11% <94.11%> (ø)`
src/core/toga/widgets/table.py	`97.67% <100.00%> (+0.20%)`	⬆️
src/core/toga/widgets/tree.py	`94.00% <100.00%> (+0.97%)`	⬆️

samschott force-pushed the column-renderers-2 branch 4 times, most recently from 157040d to 74a7e66 Compare May 1, 2022 19:44

samschott mentioned this pull request May 1, 2022

Column renderers #1085

Closed

4 tasks

samschott commented May 1, 2022

View reviewed changes

samschott force-pushed the column-renderers-2 branch 5 times, most recently from 62566a7 to 3542f62 Compare May 1, 2022 21:29

samschott marked this pull request as ready for review May 1, 2022 21:36

samschott marked this pull request as draft May 2, 2022 09:36

samschott force-pushed the column-renderers-2 branch 2 times, most recently from e5af018 to d3a2c42 Compare May 2, 2022 14:37

samschott marked this pull request as ready for review May 2, 2022 14:39

samschott added 11 commits May 7, 2022 15:00

new constants for tree check boxes

38ad5e9

make Node and Row public

e794460

add missing __init__.py

13ccfc4

define table column widget

1b2f086

table column implementations

f90a44b

switch Table and Tree to column-based implementations

99ace69

[gtk sourcetreemodel] provide a single column only

9a0673c

remove Column from docs for now

3b52674

adapt tests

ce0115a

* Don't test removed accessor attribute * Allow duplicate accessors

update gtk backend tests

a79ae1f

add column test

45ebace

samschott force-pushed the column-renderers-2 branch from d3a2c42 to 45ebace Compare May 7, 2022 13:00

freakboy3742 requested changes May 31, 2022

View reviewed changes

samschott added 6 commits June 7, 2022 23:58

rename tablecolumn -> column

e231cf5

don't allow indeterminate checkboxes in table cell initially

95e5f1f

cleaner column API

67846e0

remove accessors from source APIs

34c9bdf

require dicts to be passed instead

allow passing columns to table and tree constructors

708ae75

update example apps

6690292

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Column renderers #1478

Column renderers #1478

samschott commented May 1, 2022 •

edited

Loading

samschott May 1, 2022

samschott commented May 2, 2022

samschott commented May 2, 2022

freakboy3742 commented May 18, 2022

freakboy3742 left a comment

freakboy3742 May 31, 2022

samschott Jun 4, 2022

freakboy3742 May 31, 2022

samschott Jun 4, 2022

freakboy3742 May 31, 2022

samschott Jun 7, 2022

freakboy3742 May 31, 2022

samschott Jun 4, 2022

freakboy3742 May 31, 2022

samschott Jun 7, 2022

freakboy3742 May 31, 2022

samschott Jun 4, 2022

samschott commented May 31, 2022

freakboy3742 commented Jun 1, 2022

samschott commented Jun 4, 2022 •

edited

Loading

freakboy3742 commented Jun 7, 2022

samschott commented Jun 7, 2022

codecov bot commented Jun 7, 2022

Column renderers #1478

Are you sure you want to change the base?

Column renderers #1478

Conversation

samschott commented May 1, 2022 • edited Loading

PR Checklist:

Choose a reason for hiding this comment

samschott commented May 2, 2022

samschott commented May 2, 2022

freakboy3742 commented May 18, 2022

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samschott commented May 31, 2022

freakboy3742 commented Jun 1, 2022

samschott commented Jun 4, 2022 • edited Loading

freakboy3742 commented Jun 7, 2022

samschott commented Jun 7, 2022

codecov bot commented Jun 7, 2022

Codecov Report

samschott commented May 1, 2022 •

edited

Loading

samschott commented Jun 4, 2022 •

edited

Loading