New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IMP] tables: introduce dynamic tables #3624
Conversation
1bce42f
to
c3df937
Compare
c3df937
to
dbea100
Compare
7ac2d90
to
891baa3
Compare
ebeac35
to
6f04814
Compare
e97c439
to
7d12eeb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments :)
f6b299d
to
4e3b7a4
Compare
THe first commit definitely deserves to be backported |
src/plugins/core/tables.ts
Outdated
for (const table of this.getTables(cmd.sheetId)) { | ||
if (cmd.target.every((zone) => !intersection(zone, table.range.zone))) { | ||
const tables: Record<UID, InternalTable> = {}; | ||
for (const table of this.getInternalTables(cmd.sheetId)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was already present before but this command can technically delete several tables at a time. should the command have been targeting the exact table zone definition all along?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably could have been a good way to do it, but at the time it would have required a bit more logic in the menu items for no real reason.
Now I'm stuck with it, I don't see how I would migrate the revisions correctly in python.
bb40f87
to
9030d22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few comments/suggestions :)
6f94bb1
to
68f43b1
Compare
7952c80
to
52e8b03
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few more comments. I think we're good after :)
52e8b03
to
ba972c9
Compare
Before this the getter `getSpreadPositionsOf` only returned the position of the cells an array formula was spread to, minus the cell the array formula was in. This made it impossible to differentiate between a cell with a normal formula, and a cell with an array formula returning a 1x1 matrix. Now the getter returns the root position of the array formula as well. The getter `getArrayFormulaSpreadingOn` was also modified to work when calling it on the root position of an array formula. There was also a bug where the spread relation weren't properly cleaned. Task: 3707037
ba972c9
to
1e9a5d1
Compare
This commit introduces the concept of dynamic table. A dynamic table is a table that is bound to a array formula, instead of a static range. The table will grow and shrink along the results of the array formula. This means that the table zones are now an UI concept. The core plugin now manages `CoreTable` objects, that can be either static or dynamic (in which case they don't have a zone, but a single cell position). The UI plugin `DynamicTablesPlugin` transform those `CoreTable` into `Table` objects that every other plugin and component can easily use. As the core plugin doesn't have access to the evaluation results, and can't know if a cell contains an array formula, it's the responsibility of the components that create a table to determine if it's dynamic or not. It is now not possible to keep the same filtered values in a copied table, since an UPDATE_CELL will invalidate the tables of the `DynamicTablesPlugin` until the next finalize, and thus `UPDATE_FILTER` won't work in the clipboard. Task: 3707037
1e9a5d1
to
891e892
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
robodoo r+ rebase-ff
:)
Merge method set to rebase and fast-forward. |
robodoo r+ rebase-ff |
Merge method set to rebase and fast-forward. |
I'm sorry, @VincentSchippefilt: this PR is already reviewed, reviewing it again is useless. |
Before this the getter `getSpreadPositionsOf` only returned the position of the cells an array formula was spread to, minus the cell the array formula was in. This made it impossible to differentiate between a cell with a normal formula, and a cell with an array formula returning a 1x1 matrix. Now the getter returns the root position of the array formula as well. The getter `getArrayFormulaSpreadingOn` was also modified to work when calling it on the root position of an array formula. There was also a bug where the spread relation weren't properly cleaned. Task: 3707037 Part-of: #3624
This commit introduces the concept of dynamic table. A dynamic table is a table that is bound to a array formula, instead of a static range. The table will grow and shrink along the results of the array formula. This means that the table zones are now an UI concept. The core plugin now manages `CoreTable` objects, that can be either static or dynamic (in which case they don't have a zone, but a single cell position). The UI plugin `DynamicTablesPlugin` transform those `CoreTable` into `Table` objects that every other plugin and component can easily use. As the core plugin doesn't have access to the evaluation results, and can't know if a cell contains an array formula, it's the responsibility of the components that create a table to determine if it's dynamic or not. It is now not possible to keep the same filtered values in a copied table, since an UPDATE_CELL will invalidate the tables of the `DynamicTablesPlugin` until the next finalize, and thus `UPDATE_FILTER` won't work in the clipboard. closes #3624 Task: 3707037 Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
[IMP] tables: introduce dynamic tables
This commit introduces the concept of dynamic table. A dynamic table
is a table that is bound to a array formula, instead of a static range.
The table will grow and shrink along the results of the array formula.
The notion is transparent to the user: if the users defines a table on
a single cell that contains an array formula, or all the cells of the
array formula, the table will be dynamic.
This means that the table zones are now an UI concept. The core plugin
now manages
InternalTable
objects, that can be either static ordynamic (in which case they don't have a zone, but a single cell position).
The UI plugin
DynamicTablesPlugin
transform thoseInternalTable
intoTable
objects that every other plugin and component can easily use.As the core plugin doesn't have access to the evaluation results, and
can't know if a cell contains an array formula, it's the responsability
of the components that create a table to determine if it's dynamic or not.
[IMP] array formula:
getSpreadPositionsOf
now returns root positionBefore this the getter
getSpreadPositionsOf
only returned theposition of the cells an array formula was spread to, minus the
cell the array formula was in. This made it impossible to differentiate
between a cell with a normal formula, and a cell with an array formula
returning a 1x1 matrix.
Now the getter returns the root position of the array formula as well.
There was also a bug where the spread relation weren't properly cleaned.
Task: 3707037
review checklist