Datagrid events

[martinRenou]

@sccolbert What is the reason for having the _repaint function in private?

We need to be able to schedule repaint of the datagrid. Whenever there is a change in our renderers (e.g. property change), we schedule a new repaint.

[sccolbert]

Renderers should really be readonly. If you absolutely need to swap a renderer, create a new one and add it to the renderer map. That will trigger an update. Otherwise, rely on metadata in the model to control dynamic renderer behavior.

[sccolbert]

I'm trying to avoid the need for library users to manually call repaint, by providing sufficient formal APIs to handle their use cases while repainting only when necessary.

[martinRenou]

Renderers should really be readonly

Why do you think renderers should be readonly?

As you know already, we are working with Bloomberg on a datagrid Jupyter Widget, and I made a TextRenderer Jupyter widget. The idea with Jupyter interactive widgets is that widgets are stateful objects that you can update dynamically from Python and the JavaScript counterpart will automatically update. Some of our use cases would be:

renderer = TextRenderer(background_color='red')

# Later on, dynamically updating the renderer. This needs a repaint of the datagrid, 
# or at least of the region affected by this renderer.
def conditional_color(cell):
    return 'red' if cell.value < 5 else 'green'
renderer.background_color = Expr(conditional_color)

So in our case updating renderers and repainting the grid is super useful.

I'm trying to avoid the need for library users to manually call repaint

I totally understand that. Although advanced users should be able to do so if they really think the grid needs a repaint.

[sccolbert]

So in our case updating renderers and repainting the grid is super useful.

Just create a brand new renderer, and set it on the grid. It will have the same effect as what you want:

https://github.com/phosphorjs/phosphor/blob/master/packages/datagrid/src/datagrid.ts#L280-L284 https://github.com/phosphorjs/phosphor/blob/master/packages/datagrid/src/datagrid.ts#L1584-L1589 https://github.com/phosphorjs/phosphor/blob/master/packages/datagrid/src/datagrid.ts#L303-L313

There's no need to have each individual renderer be dynamic.

[sccolbert]

Although advanced users should be able to do so if they really think the grid needs a repaint.

Not if I have control over everything that goes into painting the grid. By definition I would know everything that could cause a repaint. I know from experience that allow the user to schedule their own paints will be abused.

[martinRenou]

Just create a brand new renderer

We would lose what makes interactive widgets powerful... The ability to dynamically update attributes, and to link widgets attributes together is really important. For example, we would like the user to be able to link a slider value to the reference value in the conditional rendering logic:

We already have this logic, and it does a repaint when the reference value changes. And we are not losing anything in terms of performances.

[sccolbert]

A cell renderer is not a widget...

[sccolbert]

You've already got a handler which is setting a renderer value and calling repaint on the grid when your ipywidget attribute changes. It should be trivial for you to just create a new renderer and set it on the grid in the same handler.

[martinRenou]

A cell renderer is not a widget...

It is. Jupyter interactive widgets are not always meant to have a visual representation. Most of the times those widgets are only a means to keep in sync a Python model (here our TextRenderer Python class) with a JavaScript model.

Another usage example is with colorscales as rendering functions, it makes a lot of sense to me to be able to change the colormap without having to throw away a renderer.

You've already got a handler which is setting a renderer value and calling repaint on the grid when your ipywidget attribute changes. It should be trivial for you to just create a new renderer and set it on the grid in the same handler

True, but it is not ideal in terms of design. Recreating a new phosphor TextRenderer instance every single time there is a change in our Python TextRenderer model feels overkill.

Also, repaint was part of the public API. I would not be surprised if someone else is relying on this API already.

[sccolbert]

Datagrid is not 1.0 and is therefore not a stable API.

Renderers are cheap to create. If you notice every property on the builtin TextRenderer class is readonly. This is on purpose. If you want to make your ipywidget configurable at the attribute level, that's on you. I'm not going to compromise my principles for ipywidgets.

[jasongrout]

Recreating a new phosphor TextRenderer instance every single time there is a change in our Python TextRenderer model feels overkill.

and

Renderers are cheap to create.

seems to be the main point of disagreement here? @martinRenou - do you actually see performance problems by creating new phosphor text renderers each time there is a change?

[jasongrout]

I'll also point out the similarities to react - react handles dom manipulation as a rendering detail. People thought it would be horribly inefficient to create a new render tree every time there was a change, but it turns out that for many usecases, it was a big win. Likewise, I can see Chris's point about bringing repainting completely under the control of datagrid.

(Don't let the analogy derail the discussion. It's an imperfect analogy, but it does point out the value of making repainting/rerendering solely the responsibility of the library, and provide a higher-level interface with more semantic context for when that rerendering/repainting should be done.)

[martinRenou]

do you actually see performance problems by creating new phosphor text renderers each time there is a change?

I don't. And it sounds like a reasonable workaround to me. But in terms of design, I would rather prefer the TextRenderer widget to not recreate a new phosphor TextRenderer on every change.

@sccolbert Although I find it pretty handy, I totally understand that the repaint is better under control of phosphor. Would you consider the CellRenderer to be an eventful class that triggers a repaint on attribute change (hence it would not be read-only)? Or is it not feasible? EDIT: I can come up with a PR, if you think this is reasonable.

[sccolbert]

Would you consider the CellRenderer to be an eventful class that triggers a repaint on attribute change (hence it would not be read-only)

No, because a grid may have dozens of cell renderers, and it's not necessary to keep track of that many signal connections. The renderers are specifically designed to be cheap and to be replaced dynamically. That's why the cell map has a changed signal.

You can quickly get carried away with subscribing to every single attribute in an application. I've seen that blow up and tank application performance on more than one occasion.

[martinRenou]

I am convinced. I'll recreate renderers on the fly then.