Column data styles for Table and Tree widgets

[corranwebster]

This PR adds support for data-driven styling of Table and Tree cells to allow support for things like value-based background colors for numerical cells (heatmaps/colormaps), highlighting of outliers or errors, right-aligning of numeric values and so on. This PR implements the functionality for Cocoa, Qt and Android backends.

Currently this supports text alignment, foreground and background colors, and fonts.

This PR specifically doesn't include:

• Travertino style support for columns (ie. non-data driven styling). However this PR implements most of the backend support needed for Travertino styles on columns.
• Related, the font support is aware of the Table's font style properties, but there is nothing to trigger a refresh in the columns if the style changes. I think this works on Qt and Android, but not currently on Cocoa.
• extending the AccessorColumn to support styling from the data, eg. allowing (icon, text, style) values (design is needed) See Style Information in AccessorColumns #4261.
• supporting DetailedList (this would make sense, but would need DetailedList to use Column objects or something similar).

For the other backends:

• GTK support is straightforward, but may make the data model unwieldy/inefficient (we need to add many more values to the table model to support styling)
• Winforms should probably wait on Winforms Tree widget #4235 to be merged; and additionally I do not have a machine to test on.
• iOS, Textual and Web backends don't have a table widget.

Known issues:

• the Android table doesn't have the text fields fill the width of the column, so the background color is only behind the actual text, and although the text alignment is set correctly (it is verified in tests), it doesn't show because the text field is only the width of the text.

Ref #1066 and #3760.

After this is merged it might be that we can consider #1066 finished.

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

[johnzhou721]

Mainly looks good. A couple of typos and nits, and 2 places where I feel we may be able to deduplicate the code from a design perspective.

[johnzhou721]

Suggested change

	return Color.parse("#ff0000)
	return Color.parse("#ff0000")

[johnzhou721]

A nit for this to read more smoothly:

Suggested change

	The `Table` and `Tree` widgets on Cocoa and Qt, and the `Table` widget on Android, can now specify colors, fonts and text alignment to use for individual cells based on the data contained in the cell by writing an appropriate `Column` subclass.
	The `Table` and `Tree` widgets on Cocoa and Qt and the `Table` widget on Android can now specify colors, fonts, or text alignment for individual cells based on their data by providing an appropriate `Column` subclass.

[johnzhou721]

This piece of code gets thrown around a lot. Is it possible to have a set_textfield_attributes function at the top of this file that accepts all the stylistic attributes, and reuse this in tree.py?

[johnzhou721]

Outdated commenet.

[johnzhou721]

Outdated comment.

[johnzhou721]

I suggest having an abstraction for this whole block, like a function that returns correct value for a Qt role and a Toga column object, so we can reuse it in both table and tree files.

[johnzhou721]

Stray debug

[freakboy3742]

Pragmatically, this looks like a good implementation of style-modifying behaviours - but I'm not sure how I feel about the core idea that the API is proposing.

We've almost completely avoided having public APIs for style modifying features. I can't think of any outside of Canvas, and in that context it's exposing very specific drawing instructions that aren't really "style" per se.

While direct "change font" etc APIs are a pragmatic solution, this would be a fairly radical change to Toga's API. While a lot of these API implementations makes sense (and would likely be required in any table styling API) on the backend, I'm not sure they makes sense as a public interface.

I'd much rather spend some time getting a design for #4261 #4238 sorted out, rather than implement a short-term solution that we then need to deprecate because we find a clean style-based API.

(EDIT: Corrected the ticket reference)

[corranwebster]

Pragmatically, this looks like a good implementation of style-modifying behaviours - but I'm not sure how I feel about the core idea that the API is proposing.

We've almost completely avoided having public APIs for style modifying features. I can't think of any outside of Canvas, and in that context it's exposing very specific drawing instructions that aren't really "style" per se.

So the intent of this really isn't "styles" per-se either, which is what I was trying to get at with the distinction of "data-driven styling" - this is for handling the situation where things like color come from the data itself, rather than from the styling of the application.

The most direct example of this would be a column where the value for each row is a Color, and we want to display that color as the background of the cell. So the color, which would normally be part of the "style" is actually the data that we are trying to display.

Similarly, a list of fonts, where sample text is displayed in the font.

But the motivating examples for me are from data visualisation as noted (colormaps, highlighting outliers, etc.).

EDIT: the distinction here is "if the data changes, does the 'style' change" (so updates come via the usual data source mechanisms).

While direct "change font" etc APIs are a pragmatic solution, this would be a fairly radical change to Toga's API. While a lot of these API implementations makes sense (and would likely be required in any table styling API) on the backend, I'm not sure they makes sense as a public interface.

There's definitely the possibility that this is the wrong API: for example it might be that rather than the individual bits of a "style" there should be just one "row_style" method which returns back style data in some usable format for the backend. That's more difficult to use for someone who only wants to change one aspect of the style, but it may be more in-line with the existing APIs.

Or in a future with a more complete CSS implementation, there's just a css method.

Or the use-cases mentioned gain separate explicit support (eg. you want to colormap values, then the column has a "colormap" attribute which takes appropriate arguments and then applies them to the row values internally to get the colors to use.

This API is more-or-less the dumbest thing that could work and be implemented incrementally.

I'd much rather spend some time getting a design for #4261 #4238 sorted out, rather than implement a short-term solution that we then need to deprecate because we find a clean style-based API.

I implemented this, because I don't see it as a stop-gap or a short-term solution that would be deprecated later. Rather column styling (as discussed tangentially in #4238) would get slotted in behind this API as a default behaviour, something like (for background color, say):

• the Column object has a style attribute
• the default background_color implementation changes to instead of returning None, it looks up the Column style's background_colour and returns that

So columns which want to do fancy data-based colouring can override it completely, but most columns would just get the style background, and the opportunity is there to do something which takes into account the default and modify it (eg. by highlighting outliers with a darker shade of the style's background color).

I agree that we'd like to avoid a situation where people start using this to do things which should be done via styling, but I think it's a fairly small lift from this PR to adding styling in the sorts of ways discussed in #4238 - it's more or less just adding the default behaviour plus a way of notifying the table implementation to re-draw when the column style changes.

[freakboy3742]

So the intent of this really isn't "styles" per-se either, which is what I was trying to get at with the distinction of "data-driven styling" - this is for handling the situation where things like color come from the data itself, rather than from the styling of the application.

The use case of "data informs style" definitely makes sense.

However, I'm not sure it follows that the API should expose options for that directly. The separation of content and form is something fairly fundamental to most presentation mechanisms from the last 30 years:

• MVC emphasises that the way something looks is independent of the data itself
• markup languages like LaTeX are fundamentally about separating form from content
• HTML added, and then actively discouraged the use of font properties in the content in favour of CSS.

While direct "change font" etc APIs are a pragmatic solution, this would be a fairly radical change to Toga's API. While a lot of these API implementations makes sense (and would likely be required in any table styling API) on the backend, I'm not sure they makes sense as a public interface.

There's definitely the possibility that this is the wrong API:

Honestly, I think it's mostly the fact that there are explicit font(), text_align() etc methods on the class that is grating against me - because it's such an obvious change from the rest of Toga's API.

This API is more-or-less the dumbest thing that could work and be implemented incrementally.

So... you say that... but is it actually that from a full fix for #4238?

Here's my thinking: the API you've implemented here essentially relies on the fact that a change in a value will trigger a redraw. If the thing that is grating for me is the existence of a font(), text_align() etc methods... do we have to use that interface? A style object has a representation for all those properties (in most cases, literally) - so if ColumnT defines style property accessor, that would allow a subclass to define an implementation that returns a Pack object that overrides the font, color, text-align etc properties that are of interest, cascading back to table-level style definitions.

For me, that's a lot closer to what I'd anticipate as the final form of #4238-esque style descriptions - adding a style, **kwargs attribute to Column (or at least to AccessorColumn). The only real sticking point on that API is that there's no way for a change in style to notify the tables using the column of the style change. I'd almost be OK with flagging that as a known limitation of a v1 API: that style for a column is set and copied at time of construction (or, alternatively, that style changes only take effect when rows are redrawn), and leave the longer term problem of adapting to dynamic style changes as a future problem.

However, even if we only do the first half (i.e., add a style accessor), I think we'd get all the features that you've described here, but with an interface that has much closer alignment with a future style-base state.

[corranwebster]

There's definitely the possibility that this is the wrong API:

Honestly, I think it's mostly the fact that there are explicit font(), text_align() etc methods on the class that is grating against me - because it's such an obvious change from the rest of Toga's API.

But it's also not, because of the kwargs on widgets. I can say:

text_field = TextInput("my_text", text_align=RIGHT, font=...)

rather than

text_field = TextInput("my_text", style=Style(text_align=RIGHT, font=...))

it's just that to access those properties you need to go through the style object.

This API is more-or-less the dumbest thing that could work and be implemented incrementally.

So... you say that... but is it actually that from a full fix for #4238?

Nope! This doesn't fix #4238, and isn't intended to, this is solving a different problem (where the color, font, etc. is the data). But I could fold in a fix for #4238 into this, particularly if we leave the problem of updating styles for later.

Here's my thinking: the API you've implemented here essentially relies on the fact that a change in a value will trigger a redraw. If the thing that is grating for me is the existence of a font(), text_align() etc methods... do we have to use that interface? A style object has a representation for all those properties (in most cases, literally) - so if ColumnT defines style property accessor, that would allow a subclass to define an implementation that returns a Pack object that overrides the font, color, text-align etc properties that are of interest, cascading back to table-level style definitions.

For me, that's a lot closer to what I'd anticipate as the final form of #4238-esque style descriptions - adding a style, **kwargs attribute to Column (or at least to AccessorColumn). The only real sticking point on that API is that there's no way for a change in style to notify the tables using the column of the style change. I'd almost be OK with flagging that as a known limitation of a v1 API: that style for a column is set and copied at time of construction (or, alternatively, that style changes only take effect when rows are redrawn), and leave the longer term problem of adapting to dynamic style changes as a future problem.

However, even if we only do the first half (i.e., add a style accessor), I think we'd get all the features that you've described here, but with an interface that has much closer alignment with a future style-base state.

So if I understand, I think you'd prefer a final API (which partially solves #4238) more like:

class Column:
    def __init__(self, heading: str, style: StyleT, **kwargs): ...
    def cell_style(self, row) -> StyleT: ...

Which is definitely do-able and not a huge change from what I have here if we don't worry about style updates (ie. the default style is set at column creation time and not changed). I think the API should call it cell_style rather than style because:

• there are style properties like width and flex which really only make sense for the column as a whole, so we may want a column-level style property.
• there's also an issue of dealing with styling the column headers that needs to be thought through

My gut feeling is that this may be a bit more cumbersome to work with than fine-grained methods (ie. I just want to set the background color of this cell to highlight an outlier, but now I need to worry about dealing a whole style object), and Pack objects are fairly heavyweight, dynamic objects rather than just being a bundle of values, but if it is a problem I guess it's easy to add the more fine-grained API in later and have the row_style method accumulate things into a StyleT object.

I'll play around with this and see what it looks like.

[freakboy3742]

Honestly, I think it's mostly the fact that there are explicit font(), text_align() etc methods on the class that is grating against me - because it's such an obvious change from the rest of Toga's API.

But it's also not, because of the kwargs on widgets.

Sure - that's more of a "convenience of construction" thing, though. As you note, they're not reflected for access - it's purely a **kwargs-> Pack(**kwargs) converter on construction.

So if I understand, I think you'd prefer a final API (which partially solves #4238) more like:

Yes - that's the idea.

Which is definitely do-able and not a huge change from what I have here if we don't worry about style updates (ie. the default style is set at column creation time and not changed). I think the API should call it cell_style rather than style because:

• there are style properties like width and flex which really only make sense for the column as a whole, so we may want a column-level style property.

I hadn't thought of that aspect - but it's a fair point.

• there's also an issue of dealing with styling the column headers that needs to be thought through

Hrm... I wonder if that means we ultimately end up with a header_style(), style() and cell_style()?

My gut feeling is that this may be a bit more cumbersome to work with than fine-grained methods (ie. I just want to set the background color of this cell to highlight an outlier, but now I need to worry about dealing a whole style object), and Pack objects are fairly heavyweight, dynamic objects rather than just being a bundle of values, but if it is a problem I guess it's easy to add the more fine-grained API in later and have the row_style method accumulate things into a StyleT object.

Fair - but they can also be shared - so you could have your 3 "color" styles for red, green, and blue cells, and return those instances based on the value of the row.

[corranwebster]

So if I understand, I think you'd prefer a final API (which partially solves #4238) more like:

Yes - that's the idea.

I spent some time with this and I'm running into a mismatch between the way styles work and the way that we need to use them to get cell styles.

If I understand the abstraction, you're never supposed to read the attributes off of the style objects directly (because the actual attributes used aren't part of the base API), rather the styles are designed to be set, and the process of setting calls the applicator object which then pushes the changes onto the widget via the widget API's set_* methods, and in this process things like the actual Font object to use, or the bounds of the widget, are computed.

The style information used in cell styling is much more ephemeral: in most cases we'd be creating styles on the fly with no applicator, and what we really want to do is read off the style attributes that we care about. To follow the intended usage, I think I'd need to create a little "dummy" applicator class like:

class DummyApplicator:
     def set_text_align(self, text_align):
         setattr(self, "text_align", text_align)
     def set_font(self, font):
         ...
     # etc

and then read off the values, something like:

style = column.cell_style(row)
applicator = DummyApplicator()
style.applicator = applicator
# now applicator.values has what we want, eg. applicator.text_align

For some of the backends we could possibly even write custom ephemeral applicators that also include the logic for setting the appropriate style attributes (or have some sort of ephemeral Cell implementation object as a target) which would work well for Android say, but for efficient item models (such as Qt, or if we wrote a data model for Gtk, I think) we will end up computing the whole style and applying the applicator over and over again for all the values each time it wants to get just one style component for a cell because of the way that it queries data for cells (although, to be fair, this is I think going to be the case for Qt no matter what: Qt item models work by Qt asking for a particular data "role" for the cell (eg. text, color, alignment, etc.) one-by-one so any API which is just providing a "style" object will have the style called multiple times for every visible cell in the table).

So I can make it work with Travertino styles, but it seems... complex.

Also, if we have styles for the whole column, then we're possibly going to need an applicator that sets those style values somewhere, and the logical place would be the column object, so the column objects might start growing set_* methods for each of the style attributes. Or maybe there's a "column implementation" object needed that's the target of all of this style info.

I'll think a bit more about this, but any thoughts (or pointing out where my mental model is wrong) would be useful.

[freakboy3742]

If I understand the abstraction, you're never supposed to read the attributes off of the style objects directly (because the actual attributes used aren't part of the base API), rather the styles are designed to be set, and the process of setting calls the applicator object which then pushes the changes onto the widget via the widget API's set_* methods, and in this process things like the actual Font object to use, or the bounds of the widget, are computed.

That's the general idea, yes. The underlying concept is that way you describe style relationships doesn't have to be a 1-1 mapping to the way those styles are manifested. This is most important when it comes to size-based information, and less obvious for things like font faces.

I think I'd need to create a little "dummy" applicator class like:

It doesn't necessarily have to be a standalone applicator class (although that's definitely an option); there just has to be a class that

So I can make it work with Travertino styles, but it seems... complex.

I'll definitely concede it's a lot more convoluted than "just set a font". In

Also, if we have styles for the whole column, then we're possibly going to need an applicator that sets those style values somewhere, and the logical place would be the column object, so the column objects might start growing set_* methods for each of the style attributes. Or maybe there's a "column implementation" object needed that's the target of all of this style info.

My read on this would be that this where we need to start considering how cascading works. Although we might be defining table-level, column-level and cell-level style definitions, a specific cell has a single style that is the cascaded set of all of them. The cell renderer would need to compute that cascade, and then apply the style as part of the rendering process. And yes - that means that somewhere in the chain there will be "set_*" methods, as that's the interface that the applicator requires.

I'll think a bit more about this, but any thoughts (or pointing out where my mental model is wrong) would be useful.

It sounds like your mental model is correct; how to make that model fit is definitely the complicated question here.