Column renderers

[samschott]

This PR introduces the concept of Columns for toga.Table and toga.Tree at the interface layer, as proposed in #1066. The columns define how the data is displayed through properties such as min_width, max_width and a renderer. The renderer entirely defines how a table column should be populated from the data source, taking values from one or more accessors in the data source and using them to display text, icons, checkboxes, etc.

Basic implementations of some renderers for the Cocoa and Gtk backends are also provided. Any editing functionality is missing.

The resulting usage is as follows:

>>> data = [{'head_1': 'value 1', 'head_2': 'value 2', 'head_3': 'value3'}),
>>>         {'head_1': 'value 1', 'head_2': 'value 2', 'head_3': 'value3'}]
>>> table_source = ListSource(data)

Columns can be provided in several forms. As a list of column titles which will be matched against accessors in the data:

>>> columns = ['Head 1', 'Head 2', 'Head 3']

A list of tuples with column titles and accessors:

>>> columns = [('Head 1', 'head_1'), ('Head 2', 'head_2'), ('Head 3', 'head_3')]

As Column instances with a renderer. The renderer determines how a column is displayed and how the data is mapped to column properties such as text and icon:

>>> columns = [
>>>     Column(title='Head 1', renderer=RendererIconText(text='head_1', icon='head_2')),
>>>     Column(title='Head 2', renderer=RendererText(text='head_1')),
>>> ]

Now we can create our Table:

>>> table = Table(columns=columns, data=table_source)

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]

Also, I have not yet changed any of the tests, etc. This PR is just to get some initial feedback about the API and to get an idea of what is required to adapt the backends.

[samschott]

Ok, so I have adapted the winforms backend as well now (only supporting text renderers) and updated the tests. The DetailedList tests on Gtk are still failing because it uses the same SourceTreeModel as Tree and Table and I haven't adapted their code. This is something to be done for later.

@freakboy3742, could you already have a look at this and check if you like concept behind the changes?

[freakboy3742]

Apologies for the delay in reviewing this - I've had a lot going on in Real Life at the moment.

Broadly - I like what I see here. I've nitpicked a couple of small things that stood out; I'm also not 100% sure about the top-level organization of toga.renderers - it feels to me like they should be a submodule of toga.widget; but the bones of what you've laid out here look really good to me. I'm sure I'll find more when it gets time for a final review.

This is obviously backwards incompatible (changing headings to columns) - and while the new name makes sense, I think there's enough code in the wild using the old API that we should provide a simple backwards compatibility shim (since, AFAICT, it should be a fairly simple mapping in this case).

The only thing I'm not sure about is how this maps onto "inputs" in the data source. Is the plan to add Renderer classes to handle input elements? And if so - how do those input widgets tie back to the renderer, and then the data source?

[freakboy3742]

This example was deliberate; it's using a native Python list rather than a ListSource to highlight that you can use simple datatypes as a source, and they'll be converted automatically to a Data Source.

[freakboy3742]

As with the table demo, the choice to not use an explicit Source here was deliberate.

[samschott]

No problem about the late review, changes across so many modules are a lot of work. Regarding the issues you raised:

Module:

I have thought about adding renderers as a submodule of toga.widgets. My reasoning for not doing so was that they are not technically widgets since they don't have an implementation layer. However, I am very flexible here.

Backwards compatibility:

That should be relatively easy to implement.

Input elements:

I am planning to add it to the renderers but I'm still not sure about the best API. At the moment, the renderers already support "input widgets": text in a table cell has always been editable in toga(-cocoa) and I have already added support for rendering checkboxes in the cocoa and gtk backends. However, a way to propagate changes in the table view back to the data source is still missing.

The issue with updating the source is as follows: at the moment, a renderer attribute can accept a number of column data types. For example, the checked_state accepts bools and 0 or 1, the text attribute can display any data type with a __str__ method, etc. I can therefore see three options:

1. The user needs to provide a callback supplied to the renderer such as on_text_changed which will be used to update the data source. The callback could take the row, the accessor name and the new value as arguments. This provides maximum flexibility. Only items with such a callback can actually be edited in the table. This is the most versatile API but its also more complex. Simple mappings may become unnecessarily complicated to code.
2. Renderer attributes can be marked as editable (e.g., the checkbox or the text) and will automatically update the data source. The renderer tries to intelligently preserve the data type: if it finds an integer in the data source, the checked state will be saved as an integer instead of a bool. This is a simpler API but may fail or cause unexpected behaviour in some cases.
3. We restrict the allowed data source types for a renderer: e.g., a checkbox can only be populated from boolean data. This makes any back-conversion trivial but couples the data source more strongly to its representation.

Any opinions?

[samschott]

After some delays, I've finally found some time to come back to this and decided to make a few changes.

1. Keep backward compatible API to Table and Tree with headers and accessors and emit deprecation warnings.
2. Remove the Renderer concept. Is was too complicated for what I was trying to achieve. Instead, data source accessors are directly defined in the Column.
3. Use style to apply column styling such as width, etc. This is still work in progress.

TODOs:

• Actually apply the style in the backends.
• Implement callbacks when a Table or Tree cell is edited. The handler can already be registered but won't do anything.
• Improve tests.

[samschott]

@freakboy3742, I know it's been a while, but could you have another look at this? The changes are outlined in the previous post. The major change has been the removal of the Renderer class. Accessing and using data source values is now done by the Column itself, thus removing one level of abstraction.

Also new is the capability of editing text and checkbox values and seeing those edits reflected in the data source. A good example to showcase this is examples/table where editing the left Table will also update the values in the right Table which is using the same source.

I haven't implemented any width settings yet since I'd like your input on that. Ideally settings such as width, flex, or even min_width, max_width should be set by the style since they are CSS-like properties. However, doing so for table columns would likely mean bypassing toga's style applicator and layout engine entirely. Or would it maybe be possible to set the parent Tree / Table as a viewport and implement set_bounds only for the width? The alternative would be to handle column width as a separate argument, independent of the style, as proposed in #1122.

[freakboy3742]

Broadly, I'm really happy with the direction this is going. I need to wrap my head around some of the internal changes to rendering - they clearly work, but I want to make sure I understand them fully.

From a high level, there are two big things that stand out for me:

The first is the the use of widget.Column/toga.Column as a public API. The implication is that Column is something that can be used standalone, which clearly won't ever be true. I'm wondering if this might a time where an inner class might make sense, making the external API toga.Table.Column and toga.Tree.Column. Since Column is identical for Table and Tree, it might also make sense for there to be a single "internal" implementation of Column that is then namespace as toga.Table.Column and toga.Tree.Column.

The second is the public handler interface around columns. There's obviously a need to handle events at the level of Column to handle editability etc, but I'm not sure it follows that we need to have a public handler interface on Column to handle on_click/on_change events. In my mind, a Tree/Table is a visualization of a data source. A GUI change (such as editing text or checking a box) should be modifying the data source, and it should be the data source telling you that there's been a change.

This isn't a distinction that we currently make with "single datum" widgets (like TextInput) - but there's 2 reasons for that:

1. While ValueSource exists in the toga API, we're not currently providing support for it anywhere. We should be, though - that's been on my todo list for a long time

2. When a widget controls a single value, it's a lot easier to think of the widget as the value, especially when the value isn't connected to anything more abstract.

In the long term, it would be easy to think of a simplified API where a TextInput has a ValueInput by default, and setting an on_change handler on the widget is a proxy for the underlying data source; however, I'm not sure it makes sense for the same to be true for Tree/Table, as those widgets wrap a list of rows, not a single value.

Do those ideas make any sense to you?

[samschott]

I'm wondering if this might a time where an inner class might make sense, making the external API toga.Table.Column and toga.Tree.Column

I completely agree with you and was already thinking along the same lines.

and it should be the data source telling you that there's been a change.

I did not think of that but do agree with your logic here. It is indeed a bit awkward to have the callbacks attached to the column when it is really the data which has changed. This could be implemented for instance through the setters of the data source.

My only concern is distinguishing between programmatic changes of the data vs user interface changes. At the moment, single value widgets in toga only call the handler when the value is changes through the GUI. When setting the handlers on the data store, it would make sense for them to be called also when the data changes programmatically. Qt for instance emits different signals for programmatic vs GUI changes to a value.

[samschott]

Handling init this way is cleaner but comes with a significant performance impact. Though I prefer having a clean implementation here and working on rubicon-objc to improve the performance.

[freakboy3742]

My only concern is distinguishing between programmatic changes of the data vs user interface changes.

Completely agreed. There's a definite risk of a "GUI change occurs, issues signal to data source, data source changes, issues signal to GUI..." cycle that we need to be careful to prevent.

Qt for instance emits different signals for programmatic vs GUI changes to a value.

That would be one option; another might be a flag on data-based signals that indicates whether the change was initiated by a data change or as a result of a UI change. Whatever approach we take, this particular detail needs a lot of attention to make sure we get the details right.

[samschott]

Thinking about this, toga.sources.Source already has the "listeners" API which we might leverage to register and call handlers.

Regarding cycles: there already is a bit of a cycle at the moment: Modifying a row in the GUI will cause the "change" listener to be called, which will update all views that are attached to the data source, including the view that caused the change. However, this programmatic update of the GUI does not trigger anything else, breaking the cycle at this point.

I was more concerned about programmatic vs GUI changes to the data source as a convenience to toga users. They might want or expect handlers to be only invoked due to GUI changes, as is the case throughout all of toga at the moment.

[samschott]

I have improved the documentation for TreeSource and TableSource, including instructions on how to register listeners. For now, this API should be sufficient for users to register their own handlers through "delegate" objects.

Of course it would be nicer in the long term to provide an API for data source changes which is more inline with toga's regular "on_change" handler API. However, I am reluctant to add two different APIs for handlers to the data source interface and I am equally reluctant to redesign the existing listeners / notifications API at the moment.

[samschott]

I've added code to set the column width from the column's style. This is done during during init instead of through toga's layout system via set_bounds so that the width is only applied once and can be changed later through the GUI by dragging the separators between column headers. Other styling (font, color, etc) won't be part of this PR but can of course be added later.

@freakboy3742, this should be ready for a proper review now. I still need to improve test coverage but would ideally like to do that after making any requested changes.

[codecov]

Codecov Report

Merging #1085 (9c94488) into master (69b4e73) will decrease coverage by 1.39%.
The diff coverage is 75.00%.

Impacted Files	Coverage Δ
src/core/toga/widgets/internal/tablecolumn.py	50.00% <50.00%> (ø)
src/core/toga/widgets/table.py	83.33% <67.44%> (-14.14%)	⬇️
src/core/toga/widgets/tree.py	81.53% <68.96%> (-11.49%)	⬇️
src/core/toga/sources/tree_source.py	95.83% <95.23%> (-0.91%)	⬇️
src/core/toga/sources/base.py	100.00% <100.00%> (ø)
src/core/toga/sources/list_source.py	100.00% <100.00%> (ø)

[samschott]

@freakboy3742, I was looking at the column style again today and realised that I am happy with the current solution. Only the column width has any meaning and traditional layout mechanisms by Pack are not relevant because we don't have any constraints in total table width (the table just becomes horizontally scrollable).

What is more, the current style attributes width and height are only ever applied during init anyways, or when resizing the window, but not when changing the attribute value programatically (see Pack.apply() implementation). I would also argue that changing the column width on window resizing should not take place, since the total available width of the table does not change. Therefore, in effect, applying the column width once during init represents the expected behaviour even though it does in practice bypasses Pack's layout mechanism - which is not well suited for table columns in the first place.

What do you think? Would you be happy to leave things as is?

[freakboy3742]

So - there's a lot of good stuff here; there's still some things that need to be addressed, though:

1. Regarding your question about style: I agree that properties like width should only be applied at construction. That said, properties like flex do need to be kept in consideration during a resize (or, at least, they should be). The good news is that the layout inside the table is almost independent of the Pack algorithm; and as a first pass, if we didn't handle flex, I'd be OK with that.
2. This has obviously drifted from master; that's my fault for not responding to this sooner, so apologies for that. I took a quick pass at merging, but the resulting code threw segfaults in some internal cell rendering code on Cocoa; I'm not sure if that's because of my merge, or some other conflict that has been introduced.
3. That said - part of the reason for the drift is that there's a lot of changes here that are really good, but completely unrelated to core of the change - things like docstring and type annotation updates. It may assist the merge process if this could be broken into smaller chunks - one PR to get the groundwork straight, and then one to update the widgets. The good news is that I can commit to much more rapid code reviews now, so it's a lot less likely that PRs will go stale due to lack of attention.
4. My biggest concern at this point is backwards compatibility. I've flagged at least one attribute that is currently in the wild that will break the current API if it is used (missing_value); there's also a whole pattern of usage (providing a list of data that is transparently turned into a data source) that has been effectively deprecated by this change. There's also a lot of requirement for explicit attributes where they were once implied. On the one hand, there's the Zen of Python argument that explicit > implicit; but on the other hand, being able to just throw data at a table and have it render, or just throw some columns at a table and have it infer the obvious accessor was a nifty trick. If we're going to deprecate these uses, I'd like to make sure we're being explicit about it.

[freakboy3742]

We'll need to provide a backwards compatibility path for the missing_value argument.

On the subject of which... what is the replacement for this fallback behaviour?

[samschott]

I agree that it probably makes sense to split / redo this PR instead of trying to resolve merge conflicts. Your other concerns, especially regarding backward compatibility, are also valid, though the current API is a little bit too implicit for my taste...

Regarding the segfaults, I suspect this may have something to do with merging #1328 and / or recent changes to memory management in rubicon-objc. The row views should be retained and reused on the Cocoa side, this is what maintains good performance even for a large number of rows. However, we may be releasing the views somewhere unintentionally.

I might find some time to look at this is in more detail next week, though these days I am having trouble keeping up with your pace!

[samschott]

Closing in favour of #1478.