Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

I/6502: Refactor TableUtils.removeRows() logic. #303

Merged
merged 23 commits into from
Apr 17, 2020
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
301438d
Add a test case for ckeditor/ckeditor#6502.
jodator Apr 8, 2020
125bafa
Merge branch 'master' into i/6502
jodator Apr 9, 2020
acbc8fc
Refactor multiple rows removal algorithm.
jodator Apr 10, 2020
741aaa7
Add test case for truncating overlapping cells.
jodator Apr 10, 2020
ac998f7
Reduce complexity of calculation notation of remove rows algorithm.
jodator Apr 10, 2020
e353a4a
Add comments to remove rows algorithm.
jodator Apr 10, 2020
33b3cd0
Update remove rows test cases.
jodator Apr 10, 2020
52d6f42
Add special case for handling spanned cells over removed rows.
jodator Apr 10, 2020
91b8ba6
Use TableEditing in some of table utils tests. See ckeditor/ckeditor5…
jodator Apr 10, 2020
bbe2740
Use cached table map because modifying table during TableWalker break…
jodator Apr 10, 2020
0daeb49
Merge branch 'master' into i/6502
oleq Apr 15, 2020
71c6334
Fix sample in codeblock.
jodator Apr 15, 2020
ebb7ff4
Update TableUtils.removeRow test cases.
jodator Apr 15, 2020
c4bfddb
Fix the logic behind adjusting cell's rowspan on removing row.
jodator Apr 15, 2020
2a3ff7a
Refactor TableUtils.removeRow() to make algorithm less mangled.
jodator Apr 15, 2020
a4a048c
Fix code comments in table utils tests.
jodator Apr 15, 2020
6e934c1
Typo fix.
jodator Apr 15, 2020
d04641f
Use same row-/col-spanned naming in tests.
jodator Apr 15, 2020
3fb7231
Fix table example in comments.
jodator Apr 16, 2020
75f6f35
Refactor internal getCellsToMoveAndTrimOnRemoveRow() function.
jodator Apr 16, 2020
1df89fb
Update src/tableutils.js
oleq Apr 16, 2020
b0bd21a
Update src/tableutils.js
oleq Apr 16, 2020
b04996d
Update src/tableutils.js
oleq Apr 16, 2020
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
197 changes: 134 additions & 63 deletions src/tableutils.js
Original file line number Diff line number Diff line change
Expand Up @@ -267,13 +267,13 @@ export default class TableUtils extends Plugin {
* ┌───┬───┬───┐ `at` = 1 ┌───┬───┬───┐
* 0 │ a │ b │ c │ `rows` = 2 │ a │ b │ c │ 0
* │ ├───┼───┤ │ ├───┼───┤
* 1 │ │ d │ e │ <-- remove from here │ │ hi │ 1
* │ ├───┼───┤ will give: ├───┼───┼───┤
* 2 │ │ f │ g │ │ jkl │ 2
* │ ├───┼───┤ └───┴───┴───┘
* 3 │ │ h │ i
* 1 │ │ d │ e │ <-- remove from here │ │ dg │ 1
* │ │ ├───┤ will give: ├───┼───┼───┤
* 2 │ │ │ f │ │ hij │ 2
* │ │ ├───┤ └───┴───┴───┘
* 3 │ │ │ g
* ├───┼───┼───┤
* 4 │ jkl
* 4 │ hij
* └───┴───┴───┘
*
* @param {module:engine/model/element~Element} table
Expand All @@ -283,27 +283,35 @@ export default class TableUtils extends Plugin {
*/
removeRows( table, options ) {
const model = this.editor.model;
const first = options.at;
const rowsToRemove = options.rows || 1;

const rowsToRemove = options.rows || 1;
const first = options.at;
const last = first + rowsToRemove - 1;

// Removing rows from table requires most calculations to be done prior to changing table structure.

// 1. Preparation - get row-spanned cells that have to be modified after removing rows.
const { cellsToMove, cellsToTrim } = getCellsToMoveAndTrimOnRemoveRow( table, last, first, rowsToRemove );

// 2. Execution
model.change( writer => {
// 2a. Move cells from removed rows that extends over a removed section - must be done before removing rows.
// This will fill any gaps in a rows below that previously were empty because of row-spanned cells.
const rowAfterRemovedSection = last + 1;
moveCellsToRow( table, rowAfterRemovedSection, cellsToMove, writer );

// 2b. Remove all required rows.
for ( let i = last; i >= first; i-- ) {
removeRow( table, i, writer );
writer.remove( table.getChild( i ) );
}

const headingRows = table.getAttribute( 'headingRows' ) || 0;

if ( headingRows && first < headingRows ) {
const newRows = getNewHeadingRowsValue( first, last, headingRows );

// Must be done after the changes in table structure (removing rows).
// Otherwise the downcast converter for headingRows attribute will fail. ckeditor/ckeditor5#6391.
model.enqueueChange( writer.batch, writer => {
updateNumericAttribute( 'headingRows', newRows, table, writer, 0 );
} );
// 2c. Update cells from rows above that overlaps removed section. Similar to step 2 but does not involve moving cells.
oleq marked this conversation as resolved.
Show resolved Hide resolved
for ( const { rowspan, cell } of cellsToTrim ) {
updateNumericAttribute( 'rowspan', rowspan, cell, writer );
}

// 2d. Adjust heading rows if removed rows were in a heading section.
updateHeadingRows( table, first, last, model, writer.batch );
} );
}

Expand Down Expand Up @@ -730,60 +738,123 @@ function breakSpanEvenly( span, numberOfCells ) {
return { newCellsSpan, updatedSpan };
}

function removeRow( table, rowIndex, writer ) {
const cellsToMove = new Map();
const tableRow = table.getChild( rowIndex );
const tableMap = [ ...new TableWalker( table, { endRow: rowIndex } ) ];

// Get cells from removed row that are spanned over multiple rows.
tableMap
.filter( ( { row, rowspan } ) => row === rowIndex && rowspan > 1 )
.forEach( ( { column, cell, rowspan } ) => cellsToMove.set( column, { cell, rowspanToSet: rowspan - 1 } ) );

// Reduce rowspan on cells that are above removed row and overlaps removed row.
tableMap
.filter( ( { row, rowspan } ) => row <= rowIndex - 1 && row + rowspan > rowIndex )
.forEach( ( { cell, rowspan } ) => updateNumericAttribute( 'rowspan', rowspan - 1, cell, writer ) );

// Move cells to another row.
const targetRow = rowIndex + 1;
const tableWalker = new TableWalker( table, { includeSpanned: true, startRow: targetRow, endRow: targetRow } );
let previousCell;
// Updates heading columns attribute if removing a row from head section.
function adjustHeadingColumns( table, removedColumnIndexes, writer ) {
const headingColumns = table.getAttribute( 'headingColumns' ) || 0;

for ( const { row, column, cell } of [ ...tableWalker ] ) {
if ( cellsToMove.has( column ) ) {
const { cell: cellToMove, rowspanToSet } = cellsToMove.get( column );
const targetPosition = previousCell ?
writer.createPositionAfter( previousCell ) :
writer.createPositionAt( table.getChild( row ), 0 );
writer.move( writer.createRangeOn( cellToMove ), targetPosition );
updateNumericAttribute( 'rowspan', rowspanToSet, cellToMove, writer );
previousCell = cellToMove;
} else {
previousCell = cell;
}
}
if ( headingColumns && removedColumnIndexes.first < headingColumns ) {
const headingsRemoved = Math.min( headingColumns - 1 /* Other numbers are 0-based */, removedColumnIndexes.last ) -
removedColumnIndexes.first + 1;

writer.remove( tableRow );
writer.setAttribute( 'headingColumns', headingColumns - headingsRemoved, table );
}
}

// Calculates a new heading rows value for removing rows from heading section.
function getNewHeadingRowsValue( first, last, headingRows ) {
if ( last < headingRows ) {
return headingRows - ( last - first + 1 );
function updateHeadingRows( table, first, last, model, batch ) {
const headingRows = table.getAttribute( 'headingRows' ) || 0;

if ( first < headingRows ) {
const newRows = last < headingRows ? headingRows - ( last - first + 1 ) : first;

// Must be done after the changes in table structure (removing rows).
// Otherwise the downcast converter for headingRows attribute will fail. ckeditor/ckeditor5#6391.
model.enqueueChange( batch, writer => {
updateNumericAttribute( 'headingRows', newRows, table, writer, 0 );
} );
}
}

// Finds cells that will be:
// - trimmed - Cells that are "above" removed rows sections and overlaps removed section - their rowspan must be trimmed.
oleq marked this conversation as resolved.
Show resolved Hide resolved
// - moved - Cells from removed rows section might stick out of. Such cells are moved to a next row after a removed section.
oleq marked this conversation as resolved.
Show resolved Hide resolved
//
// Sample table with overlapping & sticking out cells:
//
// +----+----+----+----+----+
// | 00 | 01 | 02 | 03 | 04 |
// +----+ + + + +
// | 10 | | | | |
// +----+----+ + + +
// | 20 | 21 | | | | <-- removed row
// + + +----+ + +
// | | | 32 | | | <-- removed row
// +----+ + +----+ +
// | 40 | | | 43 | |
// +----+----+----+----+----+
//
// In a table above:
// - cells to trim: '02', '03' & '04'.
// - cells to move: '21' & '32'.
function getCellsToMoveAndTrimOnRemoveRow( table, last, first, rowsToRemove ) {
jodator marked this conversation as resolved.
Show resolved Hide resolved
const cellsToMove = new Map();
const cellsToTrim = [];

for ( const { row, column, rowspan, cell } of new TableWalker( table, { endRow: last } ) ) {
const lastRowOfCell = row + rowspan - 1;

const isCellStickingOutFromRemovedRows = row >= first && row <= last && lastRowOfCell > last;

if ( isCellStickingOutFromRemovedRows ) {
const rowspanInRemovedSection = last - row + 1;
const rowSpanToSet = rowspan - rowspanInRemovedSection;

cellsToMove.set( column, {
cell,
rowspan: rowSpanToSet
} );
}

return first;
const isCellOverlappingRemovedRows = row < first && lastRowOfCell >= first;

if ( isCellOverlappingRemovedRows ) {
let rowspanAdjustment;

// Cell fully covers removed section - trim it by removed rows count.
if ( lastRowOfCell >= last ) {
rowspanAdjustment = rowsToRemove;
}
// Cell partially overlaps removed section - calculate cell's span that is in removed section.
else {
rowspanAdjustment = lastRowOfCell - first + 1;
}

cellsToTrim.push( {
cell,
rowspan: rowspan - rowspanAdjustment
} );
}
}
return { cellsToMove, cellsToTrim };
}

// Updates heading columns attribute if removing a row from head section.
function adjustHeadingColumns( table, removedColumnIndexes, writer ) {
const headingColumns = table.getAttribute( 'headingColumns' ) || 0;
function moveCellsToRow( table, targetRowIndex, cellsToMove, writer ) {
const tableWalker = new TableWalker( table, {
includeSpanned: true,
startRow: targetRowIndex,
endRow: targetRowIndex
} );

if ( headingColumns && removedColumnIndexes.first < headingColumns ) {
const headingsRemoved = Math.min( headingColumns - 1 /* Other numbers are 0-based */, removedColumnIndexes.last ) -
removedColumnIndexes.first + 1;
const tableRowMap = [ ...tableWalker ];
const row = table.getChild( targetRowIndex );

writer.setAttribute( 'headingColumns', headingColumns - headingsRemoved, table );
let previousCell;

for ( const { column, cell, isSpanned } of tableRowMap ) {
if ( cellsToMove.has( column ) ) {
const { cell: cellToMove, rowspan } = cellsToMove.get( column );

const targetPosition = previousCell ?
writer.createPositionAfter( previousCell ) :
writer.createPositionAt( row, 0 );

writer.move( writer.createRangeOn( cellToMove ), targetPosition );
updateNumericAttribute( 'rowspan', rowspan, cellToMove, writer );

previousCell = cellToMove;
} else if ( !isSpanned ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the line that caused a problem in previous implementation.

// If cell is spanned then `cell` holds reference to overlapping cell. See ckeditor/ckeditor5#6502.
previousCell = cell;
}
}
}
Loading