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

[IMP] clipboard: preserve cell style and format when copy/pasting cross spreadsheets #4053

Closed
wants to merge 2 commits into from

Conversation

Rachico
Copy link
Contributor

@Rachico Rachico commented Apr 11, 2024

[IMP] clipboard: preserve cell style and format when copy/pasting cross spreadsheets

Problem

Before this commit, copying and pasting content cross spreadsheets removes all cell style and format and only keeps cell values. This is because the model clipboard is invalidated from one instance to another.

Solution

This commit solves the issue by adding a new custom type in the os clipboard (application/o-spreadsheet) and using the content saved in this key to re-create the cell style and format in the new spreadsheet.

NOTE: After an assessment of the capabilities and limitations of most modern browsers, here's the bottom line till the date of this commit:

For Google Chrome (and all chromium browsers: Opera, Edge, Brave,...): read/write are supported for custom types; Therefore cross spreadsheet copy/paste works like a charm.

For Safari and Mozilla Firefox: saving custom types in the os clipboard is not supported; Therefore, cross spreadsheet copy/paste does not work.

Task: 3597039

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 Apr 11, 2024

src/plugins/ui_stateful/clipboard.ts Outdated Show resolved Hide resolved
src/types/commands.ts Outdated Show resolved Hide resolved
src/types/commands.ts Outdated Show resolved Hide resolved
src/types/commands.ts Outdated Show resolved Hide resolved
@Rachico Rachico force-pushed the master-copy-paste-to-other-spreadsheets-mera branch 5 times, most recently from 7a773c4 to 8f650d2 Compare April 15, 2024 09:43
@Rachico Rachico changed the title [IMP] Keep cell formatting when copy/pasting data from a spreadsheet to another [IMP] clipboard: keep cell formatting when copy/pasting data from a spreadsheet to another Apr 15, 2024
@Rachico Rachico changed the title [IMP] clipboard: keep cell formatting when copy/pasting data from a spreadsheet to another [IMP] clipboard: keep cell formatting when copy/pasting cells from a spreadsheet to another Apr 15, 2024
@Rachico Rachico force-pushed the master-copy-paste-to-other-spreadsheets-mera branch 2 times, most recently from 0c59953 to c5c78c4 Compare April 15, 2024 14:21
@Rachico Rachico marked this pull request as ready for review April 15, 2024 14:21
@Rachico Rachico changed the title [IMP] clipboard: keep cell formatting when copy/pasting cells from a spreadsheet to another [IMP] clipboard: keep cell formatting when copy/pasting cells from one spreadsheet to another Apr 15, 2024
@Rachico Rachico force-pushed the master-copy-paste-to-other-spreadsheets-mera branch 4 times, most recently from f2bee9d to 4c94ede Compare April 16, 2024 13:24
src/actions/menu_items_actions.ts Outdated Show resolved Hide resolved
tests/test_helpers/commands_helpers.ts Outdated Show resolved Hide resolved
src/plugins/ui_stateful/clipboard.ts Show resolved Hide resolved
src/plugins/ui_stateful/clipboard.ts Outdated Show resolved Hide resolved
src/plugins/ui_stateful/clipboard.ts Outdated Show resolved Hide resolved
tests/clipboard/clipboard_plugin.test.ts Outdated Show resolved Hide resolved
tests/clipboard/clipboard_plugin.test.ts Outdated Show resolved Hide resolved
tests/clipboard/clipboard_plugin.test.ts Outdated Show resolved Hide resolved
tests/clipboard/clipboard_plugin.test.ts Outdated Show resolved Hide resolved
tests/test_helpers/commands_helpers.ts Outdated Show resolved Hide resolved
@Rachico Rachico force-pushed the master-copy-paste-to-other-spreadsheets-mera branch 5 times, most recently from f9fcdec to b140631 Compare April 26, 2024 10:08
src/plugins/ui_stateful/clipboard.ts Outdated Show resolved Hide resolved
src/helpers/range.ts Outdated Show resolved Hide resolved
src/actions/menu_items_actions.ts Outdated Show resolved Hide resolved
src/actions/menu_items_actions.ts Outdated Show resolved Hide resolved
src/components/grid/grid.ts Outdated Show resolved Hide resolved
tests/clipboard/clipboard_plugin.test.ts Outdated Show resolved Hide resolved
tests/clipboard/clipboard_plugin.test.ts Outdated Show resolved Hide resolved
tests/clipboard/clipboard_plugin.test.ts Outdated Show resolved Hide resolved
tests/clipboard/clipboard_plugin.test.ts Outdated Show resolved Hide resolved
tests/clipboard/clipboard_plugin.test.ts Outdated Show resolved Hide resolved
@Rachico Rachico force-pushed the master-copy-paste-to-other-spreadsheets-mera branch 2 times, most recently from e32b699 to 069cb8d Compare May 6, 2024 12:01
@Rachico Rachico requested a review from LucasLefevre May 6, 2024 12:02
@Rachico Rachico force-pushed the master-copy-paste-to-other-spreadsheets-mera branch 2 times, most recently from d659d47 to 50546cf Compare May 8, 2024 08:34
@Rachico Rachico force-pushed the master-copy-paste-to-other-spreadsheets-mera branch 3 times, most recently from 68682e4 to 46b2256 Compare July 11, 2024 13:16
@Rachico Rachico requested a review from LucasLefevre July 11, 2024 13:27
@Rachico Rachico force-pushed the master-copy-paste-to-other-spreadsheets-mera branch from 46b2256 to 01bb5b3 Compare July 12, 2024 19:45
@Rachico Rachico force-pushed the master-copy-paste-to-other-spreadsheets-mera branch 4 times, most recently from 6230554 to 576d350 Compare July 30, 2024 09:56
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.

A few more suggestions, otherwise it LGTM 👍 and almost ready to merge :)

src/actions/menu_items_actions.ts Outdated Show resolved Hide resolved
src/clipboard_handlers/cell_clipboard.ts Outdated Show resolved Hide resolved
src/components/grid/grid.ts Outdated Show resolved Hide resolved
src/components/grid/grid.ts Outdated Show resolved Hide resolved
src/components/grid/grid.ts Outdated Show resolved Hide resolved
src/actions/menu_items_actions.ts Outdated Show resolved Hide resolved
src/helpers/range.ts Outdated Show resolved Hide resolved
src/helpers/ui/paste_interactive.ts Show resolved Hide resolved
src/plugins/ui_stateful/clipboard.ts Outdated Show resolved Hide resolved
tests/spreadsheet/spreadsheet_component.test.ts Outdated Show resolved Hide resolved
@Rachico Rachico force-pushed the master-copy-paste-to-other-spreadsheets-mera branch 2 times, most recently from 6843d08 to 78fff77 Compare July 30, 2024 16:31
@Rachico Rachico requested a review from LucasLefevre July 30, 2024 16:31
@Rachico Rachico force-pushed the master-copy-paste-to-other-spreadsheets-mera branch from 78fff77 to 8560e26 Compare July 31, 2024 08:34
@LucasLefevre LucasLefevre force-pushed the master-copy-paste-to-other-spreadsheets-mera branch from 8560e26 to e501270 Compare July 31, 2024 11:03
@Rachico Rachico force-pushed the master-copy-paste-to-other-spreadsheets-mera branch 3 times, most recently from 5353598 to e260667 Compare August 1, 2024 11:18
@Rachico Rachico force-pushed the master-copy-paste-to-other-spreadsheets-mera branch from e260667 to 1a1fcb7 Compare August 1, 2024 11:23
Before this commit, copying and pasting content cross spreadsheets removes
all cell style and format and only keeps cell values. This is because the
model clipboard is different from one instance to another.

This commit solves the issue by adding a new custom type in the os clipboard
(`application/o-spreadsheet`) and using the content saved in this key to
re-create the cell style and format in the new spreadsheet.

NOTE: After an assessment of the capabilities and limitations of most modern
browsers, here's the bottom line till the date of this commit:

For Google Chrome (and all chromium browsers: Opera, Edge, Brave,...):
read/write are supported for custom types; Therefore cross spreadsheet
copy/paste works like a charm.

For Safari and Mozilla Firefox: saving custom types in the os clipboard is not
supported; Therefore, cross spreadsheet copy/paste does not work.

Task: 3597039
@LucasLefevre LucasLefevre force-pushed the master-copy-paste-to-other-spreadsheets-mera branch from 1a1fcb7 to bf4cb25 Compare August 5, 2024 09:38
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.

I reformatted the commit message to have shorter lines.
A common guideline is: the title should be short (ideally < 50 chars) and all lines in
the message as well (ideally < 80 chars)

robodoo r+ rebase-ff

src/plugins/core/cell.ts Outdated Show resolved Hide resolved
src/plugins/core/cell.ts Outdated Show resolved Hide resolved
@robodoo
Copy link
Collaborator

robodoo commented Aug 5, 2024

Merge method set to rebase and fast-forward.

robodoo pushed a commit that referenced this pull request Aug 5, 2024
Task: 0
Part-of: #4053
Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
Co-authored-by: Rachico <[email protected]>
robodoo pushed a commit that referenced this pull request Aug 5, 2024
Before this commit, copying and pasting content cross spreadsheets removes
all cell style and format and only keeps cell values. This is because the
model clipboard is different from one instance to another.

This commit solves the issue by adding a new custom type in the os clipboard
(`application/o-spreadsheet`) and using the content saved in this key to
re-create the cell style and format in the new spreadsheet.

NOTE: After an assessment of the capabilities and limitations of most modern
browsers, here's the bottom line till the date of this commit:

For Google Chrome (and all chromium browsers: Opera, Edge, Brave,...):
read/write are supported for custom types; Therefore cross spreadsheet
copy/paste works like a charm.

For Safari and Mozilla Firefox: saving custom types in the os clipboard is not
supported; Therefore, cross spreadsheet copy/paste does not work.

closes #4053

Task: 3597039
Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
@robodoo robodoo closed this Aug 5, 2024
@robodoo robodoo added the 17.5 label Aug 5, 2024
@fw-bot fw-bot deleted the master-copy-paste-to-other-spreadsheets-mera branch August 19, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants