search

phosphorjs / phosphor

The PhosphorJS Library
BSD 3-Clause "New" or "Revised" License
1.04k stars 169 forks source link

implement per-cell metadata. simplify the renderer map. #430

Closed sccolbert closed 5 years ago

sccolbert commented 5 years ago

cc @blink1073 @afshin @jasongrout @ellisonbg @SylvainCorlay

This updates the DataModel to allow per-cell metadata. I profiled this with the datagrid example app, and found negligible difference in rendering times. This change will make it easier to build a spreadsheet model, where the easiest way to store per-cell type/styling/formatting info is with per-cell metadata.

I also updated the logic of the RendererMap. My initial implementation of matching based on metadata string values was too much magic (IMO) and was unnecessarily restrictive. The new version is much simpler and has much more flexibility. It allows for arbitrary resolution logic by the end user.

The prepare method was removed from CellRenderer because it was called once per column. This matched the mental model that metadata was per-column rather than per-cell. The idea was that a renderer could pre-set GC properties which are common for a column. This turned out to not be necessary because the GC is caching property changes automatically anyway. Getting rid of that method simplifies the mental model.

There are a few other unrelated updates to copyright dates and changing interface to type where it makes sense.

nmichaud commented 5 years ago

@sccolbert The prepare method is still useful for computing per-column attributes that affect the rendering (for example, the location of axis/grid lines across all cells in a column for a barcharts based on ideal labeling, etc). Those are still a bit expensive to do per cell, and they don't make sense to calculate in the data model since they depend on the rendering space

sccolbert commented 5 years ago

@nmichaud Thanks. I can add it back and make it a no-op in the base class.

sccolbert commented 5 years ago

@nmichaud Wait, I can't add it back, because we have per-cell renderers. So you'll need to cache those computations based on the width/index of the column.

nmichaud commented 5 years ago

Yeah I was afraid of that. Will see how that works out.