Datagrid: Introduce AsyncCellRenderer and ImageRenderer

[martinRenou]

This is a step towards fixing jupyter-widgets/ipydatagrid#253

Introducing AsyncCellRenderer

Prior to this change, the Lumino datagrid would not allow rendering cells asynchronously. This means that rendering images was not really possible (unless the cell renderer had a reference to the datagrid and requested a redraw once the asynchronous task was done).

This PR introduces the AsyncCellRenderer abstract class, which the datagrid uses as following (more or less pseudo-code):

if (renderer instanceof AsyncCellRenderer) {
  if (!renderer.isReady(config)) {
    // Paint a placeholder, waiting for all asynchronous tasks to be performed
    renderer.paintPlaceholder(gc, config);

    // Perform all asynchronous tasks (without waiting) then redraw the region of the cell
    renderer.load(config).then(() => {
      this.repaintRegion(cell.position);
    }); 
  } else {
    // The async renderer is ready, we can paint synchronously
    renderer.paint(gc, config);
  }
} else {
  // Classic synchronous painting
  renderer.paint(gc, config);
}

This allows supporting asynchronous cell rendering without slowing down the synchronous way.

Introducing the ImageRenderer

This implements the AsyncCellRenderer and allows to load images in the datagrid:

[martinRenou]

Ready for review!! CI failure seems unrelated, I don't have the right to restart it

[martinRenou]

All green! Thanks to the person who restarted it :D

[krassowski]

Looks great! I kicked the failing test. I left two questions on the API design.

Also, we now have a minimal scaffold for datagrid tests here, it would be greatly appreciated if the critical subset of the new API was unit-tested.

[krassowski]

I wonder if it would be better to go for more flexible width: 100% and height: 100%. This way we can implement other sizing options (e.g. based on pixels) in the future, and already use width: 50%. Unless it does not make sense here.

[martinRenou]

That works with me. Note that we cannot apply CSS rules though, so we'll need to parse the percentage and compute it manually. But it's not impossible :D

[krassowski]

Yes, parsing only % values for now should be managable. I would encourage doing this in this PR. In the future we might swap the implementation for CSSStyleValue.parse once Firefox supports it.

[krassowski]

Do elements in this cache need to be disposed some way to avoid memory leaks?

[martinRenou]

As it's a static property, it will live as long as the page is running, is that right? So I guess there will be no memory leak happening?

[martinRenou]

I thought we may want to clean the cache periodically, could be done in a separate PR if needed.

[krassowski]

In context of outputs in a notebook, if user opens and then closes 100 notebooks with different in images in data grids, will the memory stay constant or increase with each notebook? If this was tied to a renderer instance, I imagine we would not need to worry as the reference would not linger, but then we would need to fetch the resource for each output rendering which is not ideal either.

[krassowski]

Culling the cache periodically might be a solution indeed, but we would want to do this selectively (not touch any currently rendered data grids). So we may want to change the cache implementation in the future to do some reference counting or time-based expiration. To make it easier on future ourselves, what if that was a non-exported global variable so that it is not a part of public API? Any downside to this idea?

[martinRenou]

Memory will increase if those images are always different.

To make it easier on future ourselves, what if that was a non-exported global variable so that it is not a part of public API

Right!! We should definitely make that cache property private.

I'm fine with the idea to periodically clean the cache. The browser will have cached the image somewhere anyway, so next time we reload the same image it should not take much time to download again.

[martinRenou]

Would that work for you if I make the cache property private for now, and we can come up with cache cleaning logic in a separate PR (if/when we see issues with it)?

[krassowski]

Yes, absolutely!

[martinRenou]

@krassowski thanks a lot for the useful review!

I just pushed another commit:

• making the cache attribute private
• adding width and height properties which supports sizes in percentage or pixels. If one property is set to an empty string, it will be computed depending on the other and respecting the original image size ratio. If both are set to an empty string, the original image size will be used.
  I figured width='' and height=100% could be a good default (respect the image size ratio and take the whole height available), tell me what you think.

[martinRenou]

Also, we now have a minimal scaffold for datagrid tests here, it would be greatly appreciated if the critical subset of the new API was unit-tested

Oops forgot to tackle this, will do

[martinRenou]

@krassowski I think it's good for another round of review if you have time!

[krassowski]

Thank you @martinRenou!