This repository has been archived by the owner on Jun 26, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 17
I/6123: The MergeTableCells command #290
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
07aaab8
Re-introduce MergeCellsCommand.
jodator 82dc95e
Merge branch 'master' into i/6123
jodator a6a237a
Temporally enable all merge cell buttons.
jodator f2375fe
Merge branch 'master' into i/6123
jodator e2783aa
Use existing util methods for obtaining selected table cells.
jodator 61fc962
Disallow merge cells command execution on selection extending from he…
jodator 7908f68
Use table editing for merge cells command tests.
jodator 061b0bf
MergeCellsCommand should provide the same output if selection is reve…
jodator 8dc8f0d
Refactor MergeCellsCommand and update its docs.
jodator 85317b1
Use proper indentation in jsdoc.
jodator c0ea882
Remove redundant checks.
jodator a8b4d89
Add docs to getRowIndexes() helper function.
jodator 7b1ba63
Merge branch 'master' into i/6123
jodator 2165d44
Merge branch 'master' into i/6123
oleq ba9080d
Apply suggestions from code review.
jodator 0f16611
Use box-drawing characters for table samples.
jodator e5cb4b7
Simplify check for table cells mergeability.
jodator File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,241 @@ | ||
/** | ||
* @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved. | ||
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license | ||
*/ | ||
|
||
/** | ||
* @module table/commands/mergecellscommand | ||
*/ | ||
|
||
import Command from '@ckeditor/ckeditor5-core/src/command'; | ||
import TableWalker from '../tablewalker'; | ||
import { findAncestor, updateNumericAttribute } from './utils'; | ||
import TableUtils from '../tableutils'; | ||
import { getRowIndexes, getSelectedTableCells } from '../utils'; | ||
|
||
/** | ||
* The merge cells command. | ||
* | ||
* The command is registered by the {@link module:table/tableediting~TableEditing} as `'mergeTableCells'` editor command. | ||
* | ||
* For example, to merge selected table cells: | ||
* | ||
* editor.execute( 'mergeTableCells' ); | ||
* | ||
* @extends module:core/command~Command | ||
*/ | ||
export default class MergeCellsCommand extends Command { | ||
/** | ||
* @inheritDoc | ||
*/ | ||
refresh() { | ||
this.isEnabled = canMergeCells( this.editor.model.document.selection, this.editor.plugins.get( TableUtils ) ); | ||
} | ||
|
||
/** | ||
* Executes the command. | ||
* | ||
* @fires execute | ||
*/ | ||
execute() { | ||
const model = this.editor.model; | ||
const tableUtils = this.editor.plugins.get( TableUtils ); | ||
|
||
model.change( writer => { | ||
const selectedTableCells = getSelectedTableCells( model.document.selection ); | ||
|
||
// All cells will be merge into the first one. | ||
const firstTableCell = selectedTableCells.shift(); | ||
|
||
// This prevents the "model-selection-range-intersects" error, caused by removing row selected cells. | ||
writer.setSelection( firstTableCell, 'on' ); | ||
|
||
// Update target cell dimensions. | ||
const { mergeWidth, mergeHeight } = getMergeDimensions( firstTableCell, selectedTableCells, tableUtils ); | ||
updateNumericAttribute( 'colspan', mergeWidth, firstTableCell, writer ); | ||
updateNumericAttribute( 'rowspan', mergeHeight, firstTableCell, writer ); | ||
|
||
for ( const tableCell of selectedTableCells ) { | ||
const tableRow = tableCell.parent; | ||
mergeTableCells( tableCell, firstTableCell, writer ); | ||
removeRowIfEmpty( tableRow, writer ); | ||
} | ||
|
||
writer.setSelection( firstTableCell, 'in' ); | ||
} ); | ||
} | ||
} | ||
|
||
// Properly removes the empty row from a table. Updates the `rowspan` attribute of cells that overlap the removed row. | ||
// | ||
// @param {module:engine/model/element~Element} row | ||
// @param {module:engine/model/writer~Writer} writer | ||
function removeRowIfEmpty( row, writer ) { | ||
if ( row.childCount ) { | ||
return; | ||
} | ||
|
||
const table = row.parent; | ||
const removedRowIndex = table.getChildIndex( row ); | ||
|
||
for ( const { cell, row, rowspan } of new TableWalker( table, { endRow: removedRowIndex } ) ) { | ||
const overlapsRemovedRow = row + rowspan - 1 >= removedRowIndex; | ||
|
||
if ( overlapsRemovedRow ) { | ||
updateNumericAttribute( 'rowspan', rowspan - 1, cell, writer ); | ||
} | ||
} | ||
|
||
writer.remove( row ); | ||
} | ||
|
||
// Merges two table cells - will ensure that after merging cells with empty paragraphs the result table cell will only have one paragraph. | ||
// If one of the merged table cells is empty, the merged table cell will have contents of the non-empty table cell. | ||
// If both are empty, the merged table cell will have only one empty paragraph. | ||
// | ||
// @param {module:engine/model/element~Element} cellBeingMerged | ||
// @param {module:engine/model/element~Element} targetCell | ||
// @param {module:engine/model/writer~Writer} writer | ||
function mergeTableCells( cellBeingMerged, targetCell, writer ) { | ||
if ( !isEmpty( cellBeingMerged ) ) { | ||
if ( isEmpty( targetCell ) ) { | ||
writer.remove( writer.createRangeIn( targetCell ) ); | ||
} | ||
|
||
writer.move( writer.createRangeIn( cellBeingMerged ), writer.createPositionAt( targetCell, 'end' ) ); | ||
} | ||
|
||
// Remove merged table cell. | ||
writer.remove( cellBeingMerged ); | ||
} | ||
|
||
// Checks if the passed table cell contains an empty paragraph. | ||
// | ||
// @param {module:engine/model/element~Element} tableCell | ||
// @returns {Boolean} | ||
function isEmpty( tableCell ) { | ||
return tableCell.childCount == 1 && tableCell.getChild( 0 ).is( 'paragraph' ) && tableCell.getChild( 0 ).isEmpty; | ||
} | ||
|
||
// Checks if the selection contains mergeable cells. | ||
// | ||
// In a table below: | ||
// | ||
// ┌───┬───┬───┬───┐ | ||
// │ a │ b │ c │ d │ | ||
// ├───┴───┼───┤ │ | ||
// │ e │ f │ │ | ||
// ├ ├───┼───┤ | ||
// │ │ g │ h │ | ||
// └───────┴───┴───┘ | ||
// | ||
// Valid selections are these which create a solid rectangle (without gaps), such as: | ||
// - a, b (two horizontal cells) | ||
// - c, f (two vertical cells) | ||
// - a, b, e (cell "e" spans over four cells) | ||
// - c, d, f (cell d spans over a cell in the row below) | ||
// | ||
// While an invalid selection would be: | ||
// - a, c (cell "b" not selected creates a gap) | ||
// - f, g, h (cell "d" spans over a cell from row of "f" cell - thus creates a gap) | ||
// | ||
// @param {module:engine/model/selection~Selection} selection | ||
// @param {module:table/tableUtils~TableUtils} tableUtils | ||
// @returns {boolean} | ||
function canMergeCells( selection, tableUtils ) { | ||
const selectedTableCells = getSelectedTableCells( selection ); | ||
|
||
if ( selectedTableCells.length < 2 || !areCellInTheSameTableSection( selectedTableCells ) ) { | ||
return false; | ||
} | ||
|
||
// A valid selection is a fully occupied rectangle composed of table cells. | ||
// Below we will calculate the area of a selected table cells and the area of valid selection. | ||
// The area of a valid selection is defined by top-left and bottom-right cells. | ||
const rows = new Set(); | ||
const columns = new Set(); | ||
|
||
let areaOfSelectedCells = 0; | ||
|
||
for ( const tableCell of selectedTableCells ) { | ||
const { row, column } = tableUtils.getCellLocation( tableCell ); | ||
const rowspan = parseInt( tableCell.getAttribute( 'rowspan' ) || 1 ); | ||
const colspan = parseInt( tableCell.getAttribute( 'colspan' ) || 1 ); | ||
|
||
// Record row & column indexes of current cell. | ||
rows.add( row ); | ||
columns.add( column ); | ||
|
||
// For cells that spans over multiple rows add also the last row that this cell spans over. | ||
if ( rowspan > 1 ) { | ||
rows.add( row + rowspan - 1 ); | ||
} | ||
|
||
// For cells that spans over multiple columns add also the last column that this cell spans over. | ||
if ( colspan > 1 ) { | ||
columns.add( column + colspan - 1 ); | ||
} | ||
|
||
areaOfSelectedCells += ( rowspan * colspan ); | ||
} | ||
|
||
// We can only merge table cells that are in adjacent rows... | ||
const areaOfValidSelection = getBiggestRectangleArea( rows, columns ); | ||
|
||
return areaOfValidSelection == areaOfSelectedCells; | ||
} | ||
|
||
// Calculates the area of a maximum rectangle that can span over provided row & column indexes. | ||
// | ||
// @param {Array.<Number>} rows | ||
// @param {Array.<Number>} columns | ||
// @returns {Number} | ||
function getBiggestRectangleArea( rows, columns ) { | ||
const rowsIndexes = Array.from( rows.values() ); | ||
const columnIndexes = Array.from( columns.values() ); | ||
|
||
const lastRow = Math.max( ...rowsIndexes ); | ||
const firstRow = Math.min( ...rowsIndexes ); | ||
const lastColumn = Math.max( ...columnIndexes ); | ||
const firstColumn = Math.min( ...columnIndexes ); | ||
|
||
return ( lastRow - firstRow + 1 ) * ( lastColumn - firstColumn + 1 ); | ||
} | ||
|
||
function areCellInTheSameTableSection( tableCells ) { | ||
const table = findAncestor( 'table', tableCells[ 0 ] ); | ||
|
||
const rowIndexes = getRowIndexes( tableCells ); | ||
const headingRows = parseInt( table.getAttribute( 'headingRows' ) || 0 ); | ||
|
||
const firstCellIsInBody = rowIndexes.first > headingRows - 1; | ||
const lastCellIsInBody = rowIndexes.last > headingRows - 1; | ||
|
||
return firstCellIsInBody === lastCellIsInBody; | ||
} | ||
|
||
function getMergeDimensions( firstTableCell, selectedTableCells, tableUtils ) { | ||
let maxWidthOffset = 0; | ||
let maxHeightOffset = 0; | ||
|
||
for ( const tableCell of selectedTableCells ) { | ||
const { row, column } = tableUtils.getCellLocation( tableCell ); | ||
|
||
maxWidthOffset = getMaxOffset( tableCell, column, maxWidthOffset, 'colspan' ); | ||
maxHeightOffset = getMaxOffset( tableCell, row, maxHeightOffset, 'rowspan' ); | ||
} | ||
|
||
// Update table cell span attribute and merge set selection on a merged contents. | ||
const { row: firstCellRow, column: firstCellColumn } = tableUtils.getCellLocation( firstTableCell ); | ||
|
||
const mergeWidth = maxWidthOffset - firstCellColumn; | ||
const mergeHeight = maxHeightOffset - firstCellRow; | ||
|
||
return { mergeWidth, mergeHeight }; | ||
} | ||
|
||
function getMaxOffset( tableCell, start, currentMaxOffset, which ) { | ||
const dimensionValue = parseInt( tableCell.getAttribute( which ) || 1 ); | ||
|
||
return Math.max( currentMaxOffset, start + dimensionValue ); | ||
} |
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undoing some bigger merge takes a long time. There's a noticeable lag. Can we somehow optimize it? What is the bottleneck here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 fromRootElement.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 inPosition
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @Reinmar, @mlewand ☝️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats! Great findings. Completely valid changes to me. Both
Position
andis()
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll create a separate ticket for them as I did not implement this yet.