[IMP] Dashboard: Cache clickable cells #4090
Closed
Conversation
This file contains 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
rrahir
force-pushed
the
master-clickable-store-rar
branch
from
b9cda0c
to
489742f
Compare
rrahir
changed the title
rrahir
force-pushed
the
master-clickable-store-rar
branch
from
489742f
to
5a99f26
Compare
hokolomopo
reviewed
Apr 29, 2024
| if ( | ||
| invalidateEvaluationCommands.has(cmd.type) || | ||
| cmd.type === "EVALUATE_CELLS" || | ||
| (cmd.type === "UPDATE_CELL" && cmd.content) |
There was a problem hiding this comment.
also need to check cmd.format theoritically (=CELL formula)
There was a problem hiding this comment.
Could you explain why? Not sure how a change in the format could have an impact on the clickable cell features.
Now, assuming you are right, I guess we should factorize this kind of condition because I think it's not clear :D
There was a problem hiding this comment.
|
|
||
| private findClickableAction(position: CellPosition) { | ||
| const getters = this.getters; | ||
| for (const item of clickableCellRegistry.getAll().sort((a, b) => a.sequence - b.sequence)) { |
There was a problem hiding this comment.
Probably a non-factor because the registry isn't big, be we create a new array + sort it at each call of this method
There was a problem hiding this comment.
it could be better 👍
| if (getters.isInMerge(position)) { | ||
| zone = getters.getMerge(position)!; | ||
| } else { | ||
| zone = positionToZone({ col, row }); |
There was a problem hiding this comment.
Suggested change
| zone = positionToZone({ col, row }); | |
| zone = positionToZone(position); |
nitpick
There was a problem hiding this comment.
also nitpick: this could be a ternary
| const rect = getters.getVisibleRect(zone); | ||
| cells.push({ | ||
| coordinates: rect, | ||
| position: position, |
There was a problem hiding this comment.
still nitpicking
Suggested change
| position: position, | |
| position, |
Comment on lines
70
to
72
| let zone: Zone; | ||
| if (getters.isInMerge(position)) { | ||
| zone = getters.getMerge(position)!; |
There was a problem hiding this comment.
In the case of emrges, we will re-compute this.getClickableAction for each cell of the merge, and have a ClickableCell with the same zone for each cell right ? That doesn't seems right ... (we love merges)
There was a problem hiding this comment.
agreed that it's a waste of resource but not fundamentally wrong ^^
There was a problem hiding this comment.
it could be fixed in prior versions as well actually
rrahir
added a commit
to odoo-dev/odoo
that referenced
this pull request
May 6, 2024
Following odoo/o-spreadsheet#4090, Some helpers are adapted to suit their use with the clickable cell refactoring. task-3869752
The computation of the clickable cells visibility condition can sometimes be costy (per cell). At the moment, we recompute those conditions at each rendering request, which can become quickly costy if we simply scroll the viewport. This revision introduces a caching of this computations to smooth out the user experience when scrolling a dashboard. Task: 3869752
rrahir
force-pushed
the
master-clickable-store-rar
branch
from
5a99f26
to
8f5be05
Compare
robodoo
pushed a commit
that referenced
this pull request
May 10, 2024
The computation of the clickable cells visibility condition can sometimes be costy (per cell). At the moment, we recompute those conditions at each rendering request, which can become quickly costy if we simply scroll the viewport. This revision introduces a caching of this computations to smooth out the user experience when scrolling a dashboard. closes #4090 Task: 3869752 Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
pro-odoo
pushed a commit
to odoo-dev/odoo
that referenced
this pull request
May 13, 2024
Following odoo/o-spreadsheet#4090, Some helpers are adapted to suit their use with the clickable cell refactoring. task-3869752
pro-odoo
pushed a commit
to odoo-dev/odoo
that referenced
this pull request
May 14, 2024
Following odoo/o-spreadsheet#4090, Some helpers are adapted to suit their use with the clickable cell refactoring. task-3869752
pro-odoo
pushed a commit
to odoo-dev/odoo
that referenced
this pull request
May 14, 2024
Following odoo/o-spreadsheet#4090, Some helpers are adapted to suit their use with the clickable cell refactoring. task-3869752
pro-odoo
pushed a commit
to odoo-dev/odoo
that referenced
this pull request
May 15, 2024
Following odoo/o-spreadsheet#4090, Some helpers are adapted to suit their use with the clickable cell refactoring. task-3869752
pro-odoo
pushed a commit
to odoo-dev/odoo
that referenced
this pull request
May 15, 2024
Following odoo/o-spreadsheet#4090, Some helpers are adapted to suit their use with the clickable cell refactoring. task-3869752
pro-odoo
pushed a commit
to odoo-dev/odoo
that referenced
this pull request
May 15, 2024
Following odoo/o-spreadsheet#4090, Some helpers are adapted to suit their use with the clickable cell refactoring. task-3869752
pro-odoo
pushed a commit
to odoo-dev/odoo
that referenced
this pull request
May 15, 2024
Following odoo/o-spreadsheet#4090, Some helpers are adapted to suit their use with the clickable cell refactoring. task-3869752
pro-odoo
pushed a commit
to odoo-dev/odoo
that referenced
this pull request
May 16, 2024
Following odoo/o-spreadsheet#4090, Some helpers are adapted to suit their use with the clickable cell refactoring. task-3869752
robodoo
pushed a commit
to odoo/odoo
that referenced
this pull request
May 16, 2024
Following odoo/o-spreadsheet#4090, Some helpers are adapted to suit their use with the clickable cell refactoring. task-3869752 Part-of: #163358
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:
The computation of the clickable cells visibility condition can
sometimes be costy (per cell). At the moment, we recompute those
conditions at each rendering request, which can become quickly costy
if we simply scroll the viewport.
This revision introduces a caching of this computations to smooth out
the user experience when scrolling a dashboard.
Task: 3869752
Task: : 3869752
review checklist