Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 57 additions & 2 deletions packages/o-spreadsheet-engine/src/plugins/ui_stateful/clipboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,53 @@ export class ClipboardPlugin extends UIPlugin {
if (zones.length > 1 || (zones[0].top === 0 && zones[0].bottom === 0)) {
return CommandResult.InvalidCopyPasteSelection;
}
break;
const zone = this.getters.getSelectedZone();
const multipleRowsInSelection = zone.top !== zone.bottom;
const copyTarget = {
...zone,
bottom: multipleRowsInSelection ? zone.top : zone.top - 1,
top: multipleRowsInSelection ? zone.top : zone.top - 1,
};
this.originSheetId = this.getters.getActiveSheetId();
Copy link
Contributor

Choose a reason for hiding this comment

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

allowDispatch should not change the state of the plugin. and this.originSheetId is not used here anyway

same comment for copy on left/copy on zone

const copiedData = this.copy([copyTarget]);
return this.isPasteAllowed(zones, copiedData, {
isCutOperation: false,
});
}
case "COPY_PASTE_CELLS_ON_LEFT": {
const zones = this.getters.getSelectedZones();
if (zones.length > 1 || (zones[0].left === 0 && zones[0].right === 0)) {
return CommandResult.InvalidCopyPasteSelection;
}
break;
const zone = this.getters.getSelectedZone();
const multipleColsInSelection = zone.left !== zone.right;
const copyTarget = {
...zone,
right: multipleColsInSelection ? zone.left : zone.left - 1,
left: multipleColsInSelection ? zone.left : zone.left - 1,
};
this.originSheetId = this.getters.getActiveSheetId();
const copiedData = this.copy([copyTarget]);
return this.isPasteAllowed(zones, copiedData, {
isCutOperation: false,
});
}
case "COPY_PASTE_CELLS_ON_ZONE": {
const zones = this.getters.getSelectedZones();
if (zones.length > 1) {
return CommandResult.InvalidCopyPasteSelection;
}
const zone = this.getters.getSelectedZone();
const copyTarget = {
...zone,
right: zone.left,
bottom: zone.top,
};
this.originSheetId = this.getters.getActiveSheetId();
const copiedData = this.copy([copyTarget]);
return this.isPasteAllowed(zones, copiedData, {
isCutOperation: false,
});
}
case "INSERT_CELL": {
const { cut, paste } = this.getInsertCellsTargets(cmd.zone, cmd.shiftDimension);
Expand Down Expand Up @@ -212,6 +251,22 @@ export class ClipboardPlugin extends UIPlugin {
});
}
break;
case "COPY_PASTE_CELLS_ON_ZONE":
{
const zone = this.getters.getSelectedZone();
const copyTarget = {
...zone,
right: zone.left,
bottom: zone.top,
};
this.originSheetId = this.getters.getActiveSheetId();
const copiedData = this.copy([copyTarget]);
this.paste([zone], copiedData, {
isCutOperation: false,
selectTarget: true,
});
}
break;
case "CLEAN_CLIPBOARD_HIGHLIGHT":
this.status = "invisible";
break;
Expand Down
5 changes: 5 additions & 0 deletions packages/o-spreadsheet-engine/src/types/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -860,6 +860,10 @@ export interface CopyPasteCellsOnLeftCommand {
type: "COPY_PASTE_CELLS_ON_LEFT";
}

export interface CopyPasteCellsOnZoneCommand {
type: "COPY_PASTE_CELLS_ON_ZONE";
}

export interface RepeatPasteCommand {
type: "REPEAT_PASTE";
target: Zone[];
Expand Down Expand Up @@ -1237,6 +1241,7 @@ export type LocalCommand =
| PasteCommand
| CopyPasteCellsAboveCommand
| CopyPasteCellsOnLeftCommand
| CopyPasteCellsOnZoneCommand
| RepeatPasteCommand
| CleanClipBoardHighlightCommand
| AutoFillCellCommand
Expand Down
3 changes: 3 additions & 0 deletions src/actions/insert_actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ export const insertImage: ActionSpec = {

export const insertTable: ActionSpec = {
name: () => _t("Table"),
description: "Alt+T",
execute: ACTIONS.INSERT_TABLE,
isVisible: (env) =>
ACTIONS.IS_SELECTION_CONTINUOUS(env) && !env.model.getters.getFirstTableInSelection(),
Expand Down Expand Up @@ -275,6 +276,7 @@ export const categoriesFunctionListMenuBuilder: ActionBuilder = () => {

export const insertLink: ActionSpec = {
name: _t("Link"),
description: "Ctrl+K",
execute: ACTIONS.INSERT_LINK,
icon: "o-spreadsheet-Icon.INSERT_LINK",
};
Expand Down Expand Up @@ -336,6 +338,7 @@ export const insertDropdown: ActionSpec = {

export const insertSheet: ActionSpec = {
name: _t("Insert sheet"),
description: "Shift+F11",
execute: (env) => {
const activeSheetId = env.model.getters.getActiveSheetId();
const position = env.model.getters.getSheetIds().indexOf(activeSheetId) + 1;
Expand Down
24 changes: 21 additions & 3 deletions src/components/grid/grid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
useRef,
useState,
} from "@odoo/owl";
import { insertSheet, insertTable } from "../../actions/insert_actions";
import {
CREATE_IMAGE,
INSERT_COLUMNS_BEFORE_ACTION,
Expand All @@ -27,7 +28,11 @@ import {
import { canUngroupHeaders } from "../../actions/view_actions";
import { isInside } from "../../helpers/index";
import { interactiveCut } from "../../helpers/ui/cut_interactive";
import { interactivePaste, interactivePasteFromOS } from "../../helpers/ui/paste_interactive";
import {
handleCopyPasteResult,
interactivePaste,
interactivePasteFromOS,
} from "../../helpers/ui/paste_interactive";
import { cellMenuRegistry } from "../../registries/menus/cell_menu_registry";
import { colMenuRegistry } from "../../registries/menus/col_menu_registry";
import {
Expand Down Expand Up @@ -359,8 +364,15 @@ export class Grid extends Component<Props, SpreadsheetChildEnv> {
const position = this.env.model.getters.getActivePosition();
this.env.model.selection.selectZone({ cell: position, zone: newZone });
},
"Ctrl+D": async () => this.env.model.dispatch("COPY_PASTE_CELLS_ABOVE"),
"Ctrl+R": async () => this.env.model.dispatch("COPY_PASTE_CELLS_ON_LEFT"),
"Ctrl+D": () => {
handleCopyPasteResult(this.env, { type: "COPY_PASTE_CELLS_ABOVE" });
},
"Ctrl+R": () => {
handleCopyPasteResult(this.env, { type: "COPY_PASTE_CELLS_ON_LEFT" });
},
"Ctrl+Enter": () => {
handleCopyPasteResult(this.env, { type: "COPY_PASTE_CELLS_ON_ZONE" });
},
"Ctrl+H": () => this.sidePanel.open("FindAndReplace", {}),
"Ctrl+F": () => this.sidePanel.open("FindAndReplace", {}),
"Ctrl+Shift+E": () => this.setHorizontalAlign("center"),
Expand Down Expand Up @@ -409,6 +421,12 @@ export class Grid extends Component<Props, SpreadsheetChildEnv> {
"Shift+PageUp": () => {
this.env.model.dispatch("ACTIVATE_PREVIOUS_SHEET");
},
"Shift+F11": () => {
insertSheet.execute?.(this.env);
},
"Alt+T": () => {
insertTable.execute?.(this.env);
},
PageDown: () => this.env.model.dispatch("SHIFT_VIEWPORT_DOWN"),
PageUp: () => this.env.model.dispatch("SHIFT_VIEWPORT_UP"),
"Ctrl+K": () => INSERT_LINK(this.env),
Expand Down
18 changes: 17 additions & 1 deletion src/helpers/ui/paste_interactive.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,32 @@
import { RemoveDuplicateTerms } from "@odoo/o-spreadsheet-engine/components/translations_terms";
import {
MergeErrorMessage,
RemoveDuplicateTerms,
} from "@odoo/o-spreadsheet-engine/components/translations_terms";
import { getCurrentVersion } from "@odoo/o-spreadsheet-engine/migrations/data";
import { _t } from "@odoo/o-spreadsheet-engine/translation";
import { SpreadsheetChildEnv } from "@odoo/o-spreadsheet-engine/types/spreadsheet_env";
import {
ClipboardPasteOptions,
CommandResult,
CopyPasteCellsAboveCommand,
CopyPasteCellsOnLeftCommand,
CopyPasteCellsOnZoneCommand,
DispatchResult,
ParsedOSClipboardContent,
ParsedOsClipboardContentWithImageData,
Zone,
} from "../../types";

export const handleCopyPasteResult = (
env: SpreadsheetChildEnv,
command: CopyPasteCellsAboveCommand | CopyPasteCellsOnLeftCommand | CopyPasteCellsOnZoneCommand
) => {
const result = env.model.dispatch(command.type);
if (result.isCancelledBecause(CommandResult.WillRemoveExistingMerge)) {
env.raiseError(MergeErrorMessage);
}
};

export const PasteInteractiveContent = {
wrongPasteSelection: _t("This operation is not allowed with multiple selections."),
willRemoveExistingMerge: RemoveDuplicateTerms.Errors.WillRemoveExistingMerge,
Expand Down
28 changes: 28 additions & 0 deletions tests/clipboard/clipboard_plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
copy,
copyPasteAboveCells,
copyPasteCellsOnLeft,
copyPasteCellsOnZone,
createDynamicTable,
createImage,
createSheet,
Expand Down Expand Up @@ -2412,6 +2413,33 @@ describe("clipboard: pasting outside of sheet", () => {
expect(getCellContent(model, "D2")).toBe("d1");
});

test("do not fill down if filling down would unmerge cells", () => {
const model = new Model();
setCellContent(model, "A1", "a1");
merge(model, "A2:A3");
setSelection(model, ["A1:A3"]);
const result = copyPasteAboveCells(model);
expect(result).toBeCancelledBecause(CommandResult.WillRemoveExistingMerge);
});

test("do not fill right if filling right would unmerge cells", () => {
const model = new Model();
setCellContent(model, "A1", "a1");
merge(model, "B1:C1");
setSelection(model, ["A1:C1"]);
const result = copyPasteCellsOnLeft(model);
expect(result).toBeCancelledBecause(CommandResult.WillRemoveExistingMerge);
});

test("do not fill if filling would unmerge cells", () => {
const model = new Model();
setCellContent(model, "A1", "a1");
merge(model, "A2:A3");
setSelection(model, ["A1:A3"]);
const result = copyPasteCellsOnZone(model);
expect(result).toBeCancelledBecause(CommandResult.WillRemoveExistingMerge);
});

test("fill right selection with single column -> for each cell, replicates the cell on its left", async () => {
const model = new Model();
setCellContent(model, "B1", "b1");
Expand Down
72 changes: 62 additions & 10 deletions tests/grid/grid_component.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@ import { resetTimeoutDuration } from "../../src/components/helpers/touch_scroll_
import { PaintFormatStore } from "../../src/components/paint_format_button/paint_format_store";
import { CellPopoverStore } from "../../src/components/popover";
import { buildSheetLink, toCartesian, toZone, zoneToXc } from "../../src/helpers";
import { handleCopyPasteResult } from "../../src/helpers/ui/paste_interactive";
import { Store } from "../../src/store_engine";
import { ClientFocusStore } from "../../src/stores/client_focus_store";
import { HighlightStore } from "../../src/stores/highlight_store";
import { NotificationStore } from "../../src/stores/notification_store";
import { Align, ClipboardMIMEType } from "../../src/types";
import { FileStore } from "../__mocks__/mock_file_store";
import { MockTransportService } from "../__mocks__/transport_service";
Expand Down Expand Up @@ -127,6 +129,14 @@ let composerFocusStore: Store<ComposerFocusStore>;

jest.useFakeTimers();
mockChart();
jest.mock("../../src/actions/menu_items_actions.ts", () => {
const originalModule = jest.requireActual("../../src/actions/menu_items_actions.ts");
return {
__esModule: true,
...originalModule,
INSERT_TABLE: jest.fn(originalModule.INSERT_TABLE),
};
});

describe("Grid component", () => {
beforeEach(async () => {
Expand Down Expand Up @@ -942,6 +952,14 @@ describe("Grid component", () => {
expect(model.getters.getActiveSheetId()).toBe("third");
});

test("Pressing Shift+F11 insert a new sheet", () => {
expect(model.getters.getSheetIds()).toHaveLength(1);
keyDown({ key: "F11", shiftKey: true });
const sheetIds = model.getters.getSheetIds();
expect(sheetIds).toHaveLength(2);
expect(model.getters.getActiveSheetId()).toBe(sheetIds[1]);
});

test("pressing Ctrl+K opens the link editor", async () => {
await keyDown({ key: "k", ctrlKey: true });
expect(fixture.querySelector(".o-link-editor")).not.toBeNull();
Expand Down Expand Up @@ -1791,7 +1809,7 @@ describe("Copy paste keyboard shortcut", () => {
const fileStore = new FileStore();
beforeEach(async () => {
clipboardData = new MockClipboardData();
({ parent, model, fixture } = await mountSpreadsheet({
({ parent, model, fixture, env } = await mountSpreadsheet({
model: new Model({}, { external: { fileStore } }),
}));
sheetId = model.getters.getActiveSheetId();
Expand Down Expand Up @@ -1953,27 +1971,61 @@ describe("Copy paste keyboard shortcut", () => {
setCellContent(model, "B2", "b2");
selectCell(model, "B2");
keyDown({ key: "D", ctrlKey: true });
expect(model.dispatch("COPY_PASTE_CELLS_ABOVE")).toBeSuccessfullyDispatched();
expect(getCell(model, "B2")?.content).toBe("b1");

setCellContent(model, "C1", "c1");
setCellContent(model, "D1", "d1");
setSelection(model, ["B2:D2"]);
keyDown({ key: "D", ctrlKey: true });
expect(model.dispatch("COPY_PASTE_CELLS_ABOVE")).toBeSuccessfullyDispatched();
expect(getCell(model, "B2")?.content).toBe("b1");
expect(getCell(model, "C2")?.content).toBe("c1");
expect(getCell(model, "D2")?.content).toBe("d1");
});

test("can copy and paste cell(s) on left using CTRL+R", async () => {
test("banane", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

🍌 🍌 🍌

setCellContent(model, "A1", "a1");
setCellContent(model, "B1", "b1");
selectCell(model, "B1");
keyDown({ key: "R", ctrlKey: true });
expect(model.dispatch("COPY_PASTE_CELLS_ON_LEFT")).toBeSuccessfullyDispatched();
merge(model, "A2:A3");
setSelection(model, ["A1:A3"]);
handleCopyPasteResult(env, { type: "COPY_PASTE_CELLS_ON_ZONE" });
// @ts-ignore
const notificationStore = env.__spreadsheet_stores__.get(NotificationStore);
Comment on lines +1990 to +1991
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid ts-ignore when possible. I'm guessing env.getStore(NotificationStore) doesn't work because of the proxy ? Otherwise you could create a mock raiseError function and give it to the env (see raiseError tests in bottom_bar_component.test.ts)

expect(notificationStore.raiseError).toHaveBeenCalled();
});

test("can copy and paste cell(s) on left using CTRL+R", async () => {
setCellContent(model, "A2", "a2");
setCellContent(model, "B2", "b2");
selectCell(model, "B2");
keyDown({ key: "R", ctrlKey: true });
expect(getCell(model, "B2")?.content).toBe("a2");

setCellContent(model, "A3", "a3");
setSelection(model, ["B1:B3"]);
setCellContent(model, "A4", "a4");
setSelection(model, ["B2:B4"]);
keyDown({ key: "R", ctrlKey: true });
expect(model.dispatch("COPY_PASTE_CELLS_ON_LEFT")).toBeSuccessfullyDispatched();
expect(getCell(model, "B2")?.content).toBe("a2");
expect(getCell(model, "B3")?.content).toBe("a3");
expect(getCell(model, "B4")?.content).toBe("a4");
});

test("can copy and paste cell(s) on zone using CTRL+ENTER", async () => {
setCellContent(model, "A1", "a1");
setSelection(model, ["A1:B2"]);
keyDown({ key: "Enter", ctrlKey: true });
expect(getCell(model, "A1")?.content).toBe("a1");
expect(getCell(model, "A2")?.content).toBe("a1");
expect(getCell(model, "B1")?.content).toBe("a1");
expect(getCell(model, "B2")?.content).toBe("a1");
});

test("Alt+T -> Table", async () => {
setSelection(model, ["A1:A5"]);
await keyDown({ key: "T", altKey: true });
expect(model.getters.getTable({ sheetId, row: 0, col: 0 })).toMatchObject({
range: { zone: toZone("A1:A5") },
});
const { INSERT_TABLE } = require("../../src/actions/menu_items_actions");
expect(INSERT_TABLE as jest.Mock).toHaveBeenCalled();
Comment on lines +2027 to +2028
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove this (and the jest.mock("../../src/actions/menu_items_actions.ts" at top of the file), you already tested that a table was inserted.

In general avoid mocking imports unless absolutely necessary. It makes tests harder to understand, and breaks if we ever move files around.

Testing what the result of an action is is the superior option over testing expect(helper).toHaveBeenCalled() anyway. Testing that the helper has been called doesn't actually test much, the helper could be broken and not do anything, and the test wouldn't notice. Just test that a table was inserted, and it's enough 🙂

});

test("Clipboard visible zones (copy) will be cleaned after hitting esc", async () => {
Expand Down
6 changes: 4 additions & 2 deletions tests/link/link_display_component.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,15 @@ describe("link display component", () => {
setCellContent(model, "A1", "HELLO");
await rightClickCell(model, "A1");
expect(
fixture.querySelector(".o-menu .o-menu-item[data-name='insert_link']")?.textContent
fixture.querySelector(".o-menu .o-menu-item[data-name='insert_link'] .o-menu-item-name")
?.textContent
).toBe("Insert link");
Comment on lines 99 to 102
Copy link
Contributor

@hokolomopo hokolomopo Dec 5, 2025

Choose a reason for hiding this comment

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

nitpick: expect(".o-menu .o-menu-item[data-name='insert_link'] .o-menu-item-name").toHaveText("Insert link")

see our custom jest matchers in jest_extend.ts


setCellContent(model, "A1", "[label](url.com)");
await rightClickCell(model, "A1");
expect(
fixture.querySelector(".o-menu .o-menu-item[data-name='insert_link']")?.textContent
fixture.querySelector(".o-menu .o-menu-item[data-name='insert_link'] .o-menu-item-name")
?.textContent
).toBe("Edit link");
});

Expand Down
Loading