Skip to content
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

[FIX] conditional format: huge revisions on copy/paste cf #5059

Closed
wants to merge 1 commit into from

Conversation

hokolomopo
Copy link
Contributor

@hokolomopo hokolomopo commented Oct 8, 2024

When copy/pasting a conditional format, the revision had one ADD_CONDITIONAL_FORMAT command per copied cell. That lead to huge revisions if, for example, a whole column with a CF was copied.

Now the revision only has a single ADD_CONDITIONAL_FORMAT command per modified CF.

The exact same work was done for data validation rules.

Benchmark:

Copy/paste a column of 100 cells with a CF:
Before: 46.6KB revision
After: 526B revision (divided by 88!)

Task: 4240668

review checklist

  • feature is organized in plugin, or UI components
  • support of duplicate sheet (deep copy)
  • in model/core: ranges are Range object, and can be adapted (adaptRanges)
  • in model/UI: ranges are strings (to show the user)
  • undo-able commands (uses this.history.update)
  • multiuser-able commands (has inverse commands and transformations where needed)
  • new/updated/removed commands are documented
  • exportable in excel
  • translations (_t("qmsdf %s", abc))
  • unit tested
  • clean commented code
  • track breaking changes
  • doc is rebuild (npm run doc)
  • status is correct in Odoo

@robodoo
Copy link
Collaborator

robodoo commented Oct 8, 2024

Pull request status dashboard

@hokolomopo hokolomopo force-pushed the 18.0-copy-cf-revision-adrm branch from 1a9c89e to 96e8ea6 Compare October 8, 2024 07:52
@rrahir
Copy link
Collaborator

rrahir commented Oct 8, 2024

The commit title should refer to clipboard and not just CF since you also fix data validations.

Also this seems to be the root cause why we have a weird order of clipboard handlers https://github.com/odoo/o-spreadsheet/blob/18.0/src/clipboard_handlers/index.ts#L23-L24. So might be worth backport this to saas-17.2 and change the handler order at the same time?

Copy link
Collaborator

@rrahir rrahir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

top top 👍

src/clipboard_handlers/conditional_format_clipboard.ts Outdated Show resolved Hide resolved
src/clipboard_handlers/data_validation_clipboard.ts Outdated Show resolved Hide resolved
@hokolomopo hokolomopo force-pushed the 18.0-copy-cf-revision-adrm branch from 96e8ea6 to 8c69f49 Compare October 8, 2024 11:43
When copy/pasting a conditional format, the revision had one
ADD_CONDITIONAL_FORMAT command per copied cell. That lead to huge
revisions if, for example, a whole column with a CF was copied.

Now the revision only has a single ADD_CONDITIONAL_FORMAT command per
modified CF.

The exact same work was done for data validation rules.

Benchmark:
-------------------
Copy/paste a column of 100 cells with a CF:
Before: 46.6KB revision
After: 526B revision (divided by 88!)

Task: 4240668
@hokolomopo hokolomopo force-pushed the 18.0-copy-cf-revision-adrm branch from 8c69f49 to f28d234 Compare October 8, 2024 13:55
Copy link
Collaborator

@LucasLefevre LucasLefevre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

robodoo r+

@robodoo robodoo closed this in 70fe23b Nov 22, 2024
@fw-bot fw-bot deleted the 18.0-copy-cf-revision-adrm branch December 6, 2024 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants