[IMP] evaluatedCell: add origin #6531
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
fdamhaut
force-pushed
the
master-add-origin-to-evaluatedCell-flda
branch
5 times, most recently
from
970c7cc to
deb2f52
Compare
LucasLefevre
requested changes
Jun 17, 2025
src/helpers/cells/cell_evaluation.ts
Outdated
| @@ -62,7 +64,9 @@ export function createEvaluatedCell( | |||
| ): EvaluatedCell { | |||
| const link = detectLink(functionResult.value); | |||
| if (!link) { | |||
| return _createEvaluatedCell(functionResult, locale, cell); | |||
| const evaluateCell = _createEvaluatedCell(functionResult, locale, cell); | |||
| if (!evaluateCell.origin) evaluateCell.origin = functionResult.origin; | |||
There was a problem hiding this comment.
I think this if is useless. What comes out of _createEvaluatedCell never has an origin
Suggested change
| if (!evaluateCell.origin) evaluateCell.origin = functionResult.origin; | |
| evaluateCell.origin = functionResult.origin; |
src/helpers/cells/cell_evaluation.ts
Outdated
| @@ -148,6 +153,7 @@ const emptyCell = memoize(function emptyCell(format: string | undefined): EmptyC | |||
| type: CellValueType.empty, | |||
| isAutoSummable: true, | |||
| defaultAlign: "left", | |||
| origin: { sheetId: "", col: 0, row: 0 }, | |||
There was a problem hiding this comment.
remove this since origin is optional.
It's better to have a missing origin rather than a wrong value
Comment on lines
334
to
336
| // Todo discuss about correctness vs perf | ||
| return EMPTY_CELL; | ||
| // return { ...EMPTY_CELL, origin: position }; |
There was a problem hiding this comment.
Let's keep it without the origin for now
Comment on lines
512
to
513
| if (!evaluatedCell.origin) { | ||
| evaluatedCell.origin = position; |
There was a problem hiding this comment.
an alternative approach to all this would be to add the origin when accessing the reference value in refFn and range.
It means a cell with a formula which is not returning a reference wouldn't have a .origin (nor literal cells), but that makes (also/more?) sense.
I would only be 2 places to change instead of the 5 or more here.
fdamhaut
force-pushed
the
master-add-origin-to-evaluatedCell-flda
branch
from
deb2f52 to
af4f07c
Compare
src/helpers/cells/cell_evaluation.ts
Outdated
Comment on lines
67
to
68
| const evaluateCell = _createEvaluatedCell(functionResult, locale, cell); | ||
| return addOrigin(evaluateCell, functionResult.origin); |
There was a problem hiding this comment.
if addOrigin mutates the input, it's better if it doesn't return it. At least it's very clear that it mutates in place
Suggested change
| const evaluateCell = _createEvaluatedCell(functionResult, locale, cell); | |
| return addOrigin(evaluateCell, functionResult.origin); | |
| const evaluateCell = _createEvaluatedCell(functionResult, locale, cell); | |
| addOrigin(evaluateCell, functionResult.origin); | |
| return evaluateCell; |
tests/evaluation/evaluation.test.ts
Outdated
| @@ -1139,6 +1139,121 @@ describe("evaluateCells", () => { | |||
| toCellPosition(sheetId, "A2") | |||
| ); | |||
| }); | |||
|
|
|||
| describe("origin", () => { | |||
| let model, sheetId; | |||
There was a problem hiding this comment.
add types. It cannot be inferred and leads to any
tests/evaluation/evaluation.test.ts
Outdated
Comment on lines
1154
to
1155
| col: 0, | ||
| row: 2, |
There was a problem hiding this comment.
easier to read
Suggested change
| col: 0, | |
| row: 2, | |
| ...toCartesian("A3") |
tests/evaluation/evaluation.test.ts
Outdated
|
|
||
| test("on literal", () => { | ||
| setCellContent(model, "A3", "5"); | ||
| expect(getEvaluatedCell(model, "A3").origin).toMatchObject({ |
There was a problem hiding this comment.
Suggested change
| expect(getEvaluatedCell(model, "A3").origin).toMatchObject({ | |
| expect(getEvaluatedCell(model, "A3").origin).toEqual({ |
| @@ -369,7 +371,7 @@ export class Evaluator { | |||
|
|
|||
| if (!isMatrix(formulaReturn)) { | |||
| const evaluatedCell = createEvaluatedCell( | |||
| nullValueToZeroValue(formulaReturn), | |||
| addOrigin(nullValueToZeroValue(formulaReturn), formulaPosition), | |||
There was a problem hiding this comment.
add position to createEvaluatedCell ?
src/helpers/cells/cell_evaluation.ts
Outdated
| @@ -191,3 +197,13 @@ function errorCell(value: string, message?: string): ErrorCell { | |||
| defaultAlign: "center", | |||
| }; | |||
| } | |||
|
|
|||
| export function addOrigin<T extends FunctionResultObject | EvaluatedCell>( | |||
There was a problem hiding this comment.
not needed to be generic anymore ?
fdamhaut
force-pushed
the
master-add-origin-to-evaluatedCell-flda
branch
from
af4f07c to
3f73e7d
Compare
fdamhaut
force-pushed
the
master-add-origin-to-evaluatedCell-flda
branch
2 times, most recently
from
1c7104c to
7311a46
Compare
With this commit, evaluation result keeps track of the value origin through references. The goal it to reuse the origin value for the "See records" feature in odoo. if A1: =PIVOT.VALUE(...) A2: =A1 with this commit both A1 and A2 will have the "See records" because we can track down that the value in A2 is actually coming from A1 which contains a pivot formula. It also paves the ground if we want to implement dynamic ranges in formula `=IF(..., A1, A2) : VLOOKUP(...)` Task: 3810138
LucasLefevre
force-pushed
the
master-add-origin-to-evaluatedCell-flda
branch
from
7311a46 to
8684e3c
Compare
robodoo
pushed a commit
that referenced
this pull request
Jul 11, 2025
With this commit, evaluation result keeps track of the value origin through references. The goal it to reuse the origin value for the "See records" feature in odoo. if A1: =PIVOT.VALUE(...) A2: =A1 with this commit both A1 and A2 will have the "See records" because we can track down that the value in A2 is actually coming from A1 which contains a pivot formula. It also paves the ground if we want to implement dynamic ranges in formula `=IF(..., A1, A2) : VLOOKUP(...)` closes #6531 Task: 3810138 Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
description of this task, what is implemented and why it is implemented that way.
Task: 3810138
review checklist