I/6123: The MergeTableCells command #290

jodator · 2020-03-27T14:44:28Z

Suggested merge commit message (convention)

Feature: Introduce MergeTableCellsCommand. Closes ckeditor/ckeditor5#6123.

Additional information

This should work OK now :)

# Conflicts: # src/tableediting.js

…ading to body.

…rsed.

jodator · 2020-03-27T14:48:31Z

Yo, @ckeditor/qa-team ! This needs some deep checks. So far I found (and fixed) some issues related to the direction of a selection (ie I had a bug when selecting from bottom right to top left).

You should not be able to merge cells from heading section (rows) with other cells.

Other than that everything should be possible, clean table should be preserved (no extra empty cells). Content (images, paragraphs) should be preserved, undo should work, etc.

The button is available for now in the merge cells menu.

coveralls · 2020-03-27T15:08:44Z

Coverage increased (+0.002%) to 99.938% when pulling e5cb4b7 on i/6123 into e2dff56 on master.

Mgsy · 2020-03-30T07:25:37Z

We've tested it and everything looks good 👌

oleq · 2020-04-01T08:42:23Z

Are you sure this makes sense?

and some weird result

jodator · 2020-04-01T08:51:15Z

Are you sure this makes sense?

Oh, forgot about this. I'll create a follow for this case: ckeditor/ckeditor5#6521.

The second thing (merging more then selected) - is this a bug or different screenshot?

oleq

Mostly questions.

src/commands/mergecellscommand.js

oleq · 2020-04-01T08:58:26Z

src/commands/mergecellscommand.js

+			for ( const tableCell of selectedTableCells ) {
+				const tableRow = tableCell.parent;
+				mergeTableCells( tableCell, firstTableCell, writer );
+				removeRowIfEmpty( tableRow, writer );
+			}


Undoing some bigger merge takes a long time. There's a noticeable lag. Can we somehow optimize it? What is the bottleneck here?

I'll look into it for a quick win. If I don't manage to find any let's move it to a follow-up.

Quick wins are in API and it doesn't look good. TL;DR: follow-up and performance research.

Fiddling with the merge cells algorithm didn't get me any noticeable performance boost (and AFAICS was adding small time). I've tried to move thing in the first step and them remove other things (cells, rows) in the next step. That didn't helped.

What did help was checking what took the most time and try to optimize those.

Original time: _undo 650ms:

The is time is from RootElement.is() - there's a .replace called on a string. I've removed that:

After removing this: _undo took 566 ms:

And self time of is dropped by ~100ms. The second one was in Position constructor. Here we do .concat() of arrays. DevTools marked that line as one that took long time. I've changed that to using spraed operator (wild guess): path = [ ...root.getPath(), ...path ]; and again perf boost, _undo took 465 ms:

and self-time of Position constructor dropped by ~100ms.

This is something in line with our performance research but I worry that here we just do a lot of stuff. Many operations that requires re-calculating positions, ranges and doing many transformations.

cc @Reinmar, @mlewand ☝️

Congrats! Great findings. Completely valid changes to me. Both Position and is() appear in many cases that I've been investigating so those changes are totally worth doing. I'd be also interested in verifying how they affect some of our performance tests (see the manual tests in ckeditor5). Could you do the comparison?

I'll create a separate ticket for them as I did not implement this yet.

src/commands/mergecellscommand.js

oleq · 2020-04-01T09:07:16Z

src/commands/mergecellscommand.js

+	// Collapsed selection or selection only one range can't contain mergeable table cells.
+	if ( selection.isCollapsed || selection.rangeCount < 2 ) {
+		return false;
+	}
+
+	const selectedTableCells = getSelectionAffectedTableCells( selection );
+
+	if ( !areCellInTheSameTableSection( selectedTableCells ) ) {
+		return false;
+	}


Wouldn't simple

const selectedTableCells = getSelectedTableCells( ... ); if ( selectedTableCells.length < 2 ) { return; }

be enough?

Would be enough but checking selection without calculating stuff is IMO a bit faster. The getSelectedTableCells() will do all sort of things and here I didn't wanted to calculate all those things.

Probably.

This code is old and I've done some refactoring here already. So WDYT about leaving this as is?

getSelectionAffectedTableCells() executes getSelectedTableCells() anyway (see the code).

But getSelectionAffectedTableCells() also returns cells that have a selection inside of them (see getTableCellsContainingSelection()), which, if not for the initial condition (selection.isCollapsed || selection.rangeCount < 2), would yield invalid cells from the perspective of multi-cell merge.

So considering ☝️ , there's no point in using getSelectionAffectedTableCells. getSelectedTableCells is enough.

As for the initial condition: I understand that it spares us some unnecessary execution of getSelectedTableCells().

But the condition is weak. For instance, you can have a selection with selection.rangeCount === 3 in FF, each range spanning over some text in a cell, and that condition will pass.
2.1. It will also pass if anyone create such selection from code.
2.2. (referring to the previous section of this comment): in conjunction with getSelectionAffectedTableCells() (instead of getSelectedTableCells()) this will cause bugs

I think it would be better if this condition was moved to getSelectedTableCells() as an initial filter to speed things up everywhere, not only here.

To sum up:

the condition should land at the beginning of getSelectedTableCells() (return an empty array if false) as a speed benefit for all the sub-features,

then it's enough to check if getSelectedTableCells() has length greater than 1.

Sure fair enough. I've forgot about Firefox.

I've updated the code.

Co-Authored-By: Aleksander Nowodzinski <a.nowodzinski@cksource.com>

jodator added 11 commits January 29, 2020 12:49

Re-introduce MergeCellsCommand.

07aaab8

Merge branch 'master' into i/6123

82dc95e

# Conflicts: # src/tableediting.js

Temporally enable all merge cell buttons.

a6a237a

Merge branch 'master' into i/6123

f2375fe

Use existing util methods for obtaining selected table cells.

e2783aa

Disallow merge cells command execution on selection extending from he…

61fc962

…ading to body.

Use table editing for merge cells command tests.

7908f68

MergeCellsCommand should provide the same output if selection is reve…

061b0bf

…rsed.

Refactor MergeCellsCommand and update its docs.

8dc8f0d

Use proper indentation in jsdoc.

85317b1

Remove redundant checks.

c0ea882

jodator added 2 commits March 30, 2020 09:50

Add docs to getRowIndexes() helper function.

a8b4d89

Merge branch 'master' into i/6123

7b1ba63

oleq self-requested a review April 1, 2020 08:34

oleq self-assigned this Apr 1, 2020

Merge branch 'master' into i/6123

2165d44

jodator mentioned this pull request Apr 1, 2020

Merge cells command should not merge cells from column header with other cells ckeditor/ckeditor5#6521

Closed

oleq suggested changes Apr 1, 2020

View reviewed changes

jodator and others added 3 commits April 1, 2020 11:17

Apply suggestions from code review.

ba9080d

Co-Authored-By: Aleksander Nowodzinski <a.nowodzinski@cksource.com>

Use box-drawing characters for table samples.

0f16611

Simplify check for table cells mergeability.

e5cb4b7

jodator requested a review from oleq April 1, 2020 10:48

jodator mentioned this pull request Apr 1, 2020

Review most used code in terms of performance ckeditor/ckeditor5#6528

Closed

oleq merged commit a5a7d3e into master Apr 1, 2020

oleq deleted the i/6123 branch April 1, 2020 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

I/6123: The MergeTableCells command #290

I/6123: The MergeTableCells command #290

jodator commented Mar 27, 2020

jodator commented Mar 27, 2020

coveralls commented Mar 27, 2020 •

edited

Loading

Mgsy commented Mar 30, 2020

oleq commented Apr 1, 2020

jodator commented Apr 1, 2020 •

edited

Loading

oleq left a comment

oleq Apr 1, 2020

jodator Apr 1, 2020

jodator Apr 1, 2020

jodator Apr 1, 2020

Reinmar Apr 1, 2020

jodator Apr 1, 2020

oleq Apr 1, 2020

jodator Apr 1, 2020

oleq Apr 1, 2020

jodator Apr 1, 2020

jodator Apr 1, 2020

jodator Apr 1, 2020

I/6123: The MergeTableCells command #290

I/6123: The MergeTableCells command #290

Conversation

jodator commented Mar 27, 2020

Suggested merge commit message (convention)

Additional information

jodator commented Mar 27, 2020

coveralls commented Mar 27, 2020 • edited Loading

Mgsy commented Mar 30, 2020

oleq commented Apr 1, 2020

jodator commented Apr 1, 2020 • edited Loading

oleq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 27, 2020 •

edited

Loading

jodator commented Apr 1, 2020 •

edited

Loading