Page MenuHome
Differential D10901

Spreadsheet: Persistent column storage and data source.
ClosedPublic
Actions

Authored by Jacques Lucke (JacquesLucke) on Apr 6 2021, 4:05 PM.

Details

Reviewers
Hans Goudey (HooglyBoogly)
Maniphest Tasks
T87048: Hide / reveal columns for spreadsheet editor
Commits
rB75491fe10021: Spreadsheet: persistent column storage and data source
Summary

A DataSource provides columns for the spreadsheet to display. Every column has a SpreadsheetColumnID as identifier. Columns are not generated eagerly anymore, instead the main spreadsheet code can request a column from a data source with an column identifier. The column identifiers can be stored in DNA and allow us to store persistent data per column.

On the user level the only thing that changes is that columns are not shown in alphabetical order anymore. Instead, new columns are always added on the left. The behavior can be changed, however I'd prefer not to automate this too much currently. I think we should just add operators to hide/reorder/resize columns soonish.

Diff Detail

Repository
rB Blender

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Apr 6 2021, 4:05 PM
Jacques Lucke (JacquesLucke) created this revision.
Harbormaster completed remote builds in B13890: Diff 35901.Apr 6 2021, 4:05 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 35903.Apr 6 2021, 4:23 PM
  • fixes
Harbormaster completed remote builds in B13892: Diff 35903.Apr 6 2021, 4:23 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 35905.Apr 6 2021, 4:29 PM
  • cleanup
Harbormaster completed remote builds in B13894: Diff 35905.Apr 6 2021, 4:29 PM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Apr 6 2021, 4:37 PM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)
Jacques Lucke (JacquesLucke) added a reviewer: Hans Goudey (HooglyBoogly).
Hans Goudey (HooglyBoogly) added a comment.Apr 7 2021, 6:27 AM

I didn't finish looking through this, here are some initial comments though. Most are about naming, or other things I didn't understand immediately when looking through this.

At first it all seems like a lot of abstraction for what's currently a relatively simple set of behaviors. I know this is mean to support future expansion, etc.
Generally though, including some comments about the big ideas of each abstraction would go a long way.

source/blender/editors/space_spreadsheet/space_spreadsheet.cc
199–200

A duplicate column would be an error on the part of the data source code? Or is it expected in some case?
It might be nice to have a quick bit of info about that here.

source/blender/editors/space_spreadsheet/spreadsheet_column_values.hh
55

I wouldn't call these factors. UI_UNIT_X is the factor, this is the width data, so I would just call it default_width.

source/blender/editors/space_spreadsheet/spreadsheet_data_source.cc
21–23

It usually seems cleaner to define these default "blank" functions right in the header. Is that possible here or did you put them here for another reason?

It's just one less search hit to look through when you want to know more about the implementations of these methods, which IMO helps a lot.

source/blender/editors/space_spreadsheet/spreadsheet_data_source.hh
29–32

It would be nice to add some more background for each of these functions, especially since it's meant to be easy to add more data sources in the future.

I think here would be the place?

source/blender/editors/space_spreadsheet/spreadsheet_data_source_geometry.cc
40–41

What about calling this foreach_available_column_id?

"default" makes the set of ids it calls the function on sound more permanent than it actually is, since it's really just whatever attributes are available at that moment.

source/blender/editors/space_spreadsheet/spreadsheet_layout.hh
21–22

To me ColumnLayout sounds like more than one column. Maybe Simply Column? Or if that's too vague, SpreadsheetColumn might be an okay alternative?

source/blender/makesdna/DNA_space_types.h
1864–1865

Picky: I get the temptation to put these on two lines, IMO the consistency of using one line is nice though, and it helps to allow ignoring it as "the ListBase part"

1866

Here I'd like to see a brief comment describing why id is allocated separately. i.e. why not just put it directly in the SpreadsheetColumn?

Jacques Lucke (JacquesLucke) planned changes to this revision.Apr 7 2021, 11:29 AM

Thanks for the feedback. Hope the comments below clear up some things. I'll incorporate the feedback a bit later.

source/blender/editors/space_spreadsheet/space_spreadsheet.cc
199–200

I could also just support duplicate columns here, shouldn't be an issue.

source/blender/editors/space_spreadsheet/spreadsheet_column_values.hh
55

Thing is, I noticed that UI_UNIT_X might not be good enough as a factor.
We might also have to take UI_style_get_dpi()->widget.points * U.pixelsize into account..

Besides that, calling it default_width seems fine as well.

source/blender/editors/space_spreadsheet/spreadsheet_data_source.cc
21–23

I tried to get into the habit of putting less stuff into headers when it is not necessary, but for these functions it seems fine if it improves readability.

source/blender/editors/space_spreadsheet/spreadsheet_data_source.hh
29–32

True.

source/blender/editors/space_spreadsheet/spreadsheet_data_source_geometry.cc
40–41

The reason I did not call it foreach_available_column_id is that there might be a huge number of possible columns (currently it is still quite limited, but for objects and if we support expressions, that can easily blow up). The idea was that the callback is called for the ids that should be displayed by default, if the user did not add/remove any columns manually (I really should have mentioned that in a comment).

source/blender/editors/space_spreadsheet/spreadsheet_layout.hh
21–22

Yeah, I ran a bit into naming conflicts here. I wanted to call the thing in DNA SpreadsheetColumn.
My idea here was that ColumnLayout contains layout info about a single column, while SpreadsheetLayout contains layout info about the entire spreadsheet.
Just Column might be a bit too vague, given that we work with columns at multiple different levels of abstraction (data source ColumnValues, DNA SpreadsheetColumn, data display ColumnLayout).

source/blender/makesdna/DNA_space_types.h
1864–1865

I honestly didn't even know there was a convention for this (looks like I don't add these listbase structures often ^^). Following the convention is fine.

1866

The reason originally was that we might want to "subclass" SpreadsheetColumnID if different data sources need different kinds of id. We could just put all id information in the SpreadsheetColumnID struct and then some fields would not be used in some cases.
At some point I even added this subclass behavior (by adding a type enum to the struct), but I removed it from this patch to avoid making it overly complicated.

Jacques Lucke (JacquesLucke) added a task: T87048: Hide / reveal columns for spreadsheet editor.Apr 7 2021, 12:23 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 35942.Apr 7 2021, 4:20 PM
  • cleanup
Harbormaster completed remote builds in B13920: Diff 35942.Apr 7 2021, 4:20 PM
Hans Goudey (HooglyBoogly) accepted this revision.Apr 8 2021, 6:18 PM

Overall this makes sense to me. It's a big change so it's a bit hard to keep track of it all conceptually though. I'm sure as things settle down the abstractions will start to become a bit clearer to me.

One thing I was wondering about is how it would cache the sort order so it didn't have to sort the data on every redraw in the future. I think more would need to be added to SpaceSpreadsheet_Runtime.
Which makes me think-- we can probably cache more there in the future, like the mesh from edit mode, the default lists maybe? That's a separate improvement anyway, it just got me thinking.

This revision is now accepted and ready to land.Apr 8 2021, 6:18 PM
Closed by commit rB75491fe10021: Spreadsheet: persistent column storage and data source (authored by Jacques Lucke (JacquesLucke)). · Explain WhyApr 9 2021, 10:21 AM
This revision was automatically updated to reflect the committed changes.
Jacques Lucke (JacquesLucke) added a commit: rB75491fe10021: Spreadsheet: persistent column storage and data source.

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
intern/
13 lines
editors/
space_spreadsheet/
14 lines
133 lines
51 lines
spreadsheet_column.hh
spreadsheet_from_geometry.hh
30 lines
73 lines
84 lines
65 lines
spreadsheet_data_source.cc
spreadsheet_from_geometry.hh
16 lines
94 lines
447 lines
spreadsheet_layout.hh
spreadsheet_from_geometry.hh
28 lines
spreadsheet_layout.cc
spreadsheet_column_layout.cc
55 lines
makesdna/
23 lines

Diff 36015

View Options

source/blender/blenkernel/intern/screen.c

Loading...
View Options

source/blender/editors/space_spreadsheet/CMakeLists.txt

Loading...
View Options

source/blender/editors/space_spreadsheet/space_spreadsheet.cc

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_cell_value.hh

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_column.hh

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_column.cc

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_column_layout.hh

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_column_layout.cc

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_column_values.hh

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_data_source.hh

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_data_source.cc

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_data_source_geometry.hh

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_data_source_geometry.cc

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_from_geometry.hh

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_from_geometry.cc

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_layout.hh

Loading...
View Options

source/blender/editors/space_spreadsheet/spreadsheet_layout.cc

Loading...
View Options

source/blender/makesdna/DNA_space_types.h

Loading...