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 11 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
156 changes: 94 additions & 62 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 │ │ h │ i │ 1
* │ ├───┼───┤ will give: ├───┼───┼───┤
* 2 │ │ f │ g │ │ j │ k │ l │ 2
* │ ├───┼───┤ └───┴───┴───┘
* 3 │ │ h │ i
* 1 │ │ d │ e │ <-- remove from here │ │ d │ i │ 1
* │ │ ├───┤ will give: ├───┼───┼───┤
* 2 │ │ │ f │ │ j │ k │ l │ 2
* │ │ ├───┤ └───┴───┴───┘
* 3 │ │ │ g
* ├───┼───┼───┤
* 4 │ jkl
* 4 │ hij
* └───┴───┴───┘
jodator marked this conversation as resolved.
Show resolved Hide resolved
*
* @param {module:engine/model/element~Element} table
Expand All @@ -283,27 +283,59 @@ 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;

model.change( writer => {
for ( let i = last; i >= first; i-- ) {
removeRow( table, i, writer );
// Removing rows from table requires most calculations to be done prior to changing table structure.

// 1. Preparation

// 1a. Cells from removed rows section might stick out of. Such cells are moved to a next row after a removed section.
const cellsToMove = new Map();
// 1b. Similarly, if a cell is "above" removed rows sections we must trim their rowspan.
const cellsToTrim = [];

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

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

if ( isCellStickingOutFromRemovedRows ) {
const rowSpanToSet = rowspan - rowspanInRemovedSection;
cellsToMove.set( column, { cell, rowspan: rowSpanToSet } );
}

const headingRows = table.getAttribute( 'headingRows' ) || 0;
const isCellOverlappingRemovedRows = row < first && lastRowOfCell >= first;

if ( headingRows && first < headingRows ) {
const newRows = getNewHeadingRowsValue( first, last, headingRows );
if ( isCellOverlappingRemovedRows ) {
const rowspanAdjustment = lastRowOfCell >= last ? rowsToRemove : first - row;
Copy link
Contributor

Choose a reason for hiding this comment

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

first - row is invalid, consider this test scenario:

it( 'should decrease rowspan of table cells from previous rows', () => {
	// +----+----+----+----+
	// | 00 | 01 | 02 | 03 |
	// +----+    +    +    +
	// | 10 |    |    |    |
	// +----+----+    +    +
	// | 20 | 21 |    |    |
	// +----+----+----+    +
	// | 30 | 31 | 32 |    |
	// +----+----+----+----+
	// | 40 | 41 | 42 | 43 |
	// +----+----+----+----+
	setData( model, modelTable( [
		[ '00', { contents: '01', rowspan: 2 }, { contents: '02', rowspan: 3 }, { contents: '03', rowspan: 4 } ],
		[ '10' ],
		[ '20', '21' ],
		[ '30', '31', '32' ],
		[ '40', '41', '42', '43' ]
	] ) );

	tableUtils.removeRows( root.getChild( 0 ), { at: 2, rows: 2 } );

	// +----+----+----+----+
	// | 00 | 01 | 02 | 03 |
	// +----+    +    +    +
	// | 10 |    |    |    |
	// +----+----+----+----+
	// | 40 | 41 | 42 | 43 |
	// +----+----+----+----+
	assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [
		[ '00', { contents: '01', rowspan: 2 }, { contents: '02', rowspan: 2 }, { contents: '03', rowspan: 2 } ],
		[ '10' ],
		[ '40', '41', '42', '43' ]
	] ) );
} );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right. The code wasn't clear enough - even for me.

I've refactored it once again and moved it outside main method execution. Hopefully this is can be parsed a bit better.

As for other cases we will probably see 🤞.

Copy link
Contributor Author

@jodator jodator Apr 15, 2020

Choose a reason for hiding this comment

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

Change (fix for this issue) is here: c4bfddb.

const rowSpanToSet = rowspan - rowspanAdjustment;
cellsToTrim.push( { cell, rowspan: rowSpanToSet } );
}
}

// 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 );
} );
// 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-- ) {
writer.remove( table.getChild( i ) );
}

// 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 +762,60 @@ 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 } ) ];
// Updates heading columns attribute if removing a row from head section.
function adjustHeadingColumns( table, removedColumnIndexes, writer ) {
const headingColumns = table.getAttribute( 'headingColumns' ) || 0;

if ( headingColumns && removedColumnIndexes.first < headingColumns ) {
const headingsRemoved = Math.min( headingColumns - 1 /* Other numbers are 0-based */, removedColumnIndexes.last ) -
removedColumnIndexes.first + 1;

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

// Calculates a new heading rows value for removing rows from heading section.
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;

// 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 } ) );
// 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 );
} );
}
}

// 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 ) );
function moveCellsToRow( table, targetRowIndex, cellsToMove, writer ) {
const tableWalker = new TableWalker( table, {
includeSpanned: true,
startRow: targetRowIndex,
endRow: targetRowIndex
} );

const tableRowMap = [ ...tableWalker ];
const row = table.getChild( targetRowIndex );

// Move cells to another row.
const targetRow = rowIndex + 1;
const tableWalker = new TableWalker( table, { includeSpanned: true, startRow: targetRow, endRow: targetRow } );
let previousCell;

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

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

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

previousCell = cellToMove;
} else {
} 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;
}
}

writer.remove( tableRow );
}

// 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 );
}

return first;
}

// Updates heading columns attribute if removing a row from head section.
function adjustHeadingColumns( table, removedColumnIndexes, writer ) {
const headingColumns = table.getAttribute( 'headingColumns' ) || 0;

if ( headingColumns && removedColumnIndexes.first < headingColumns ) {
const headingsRemoved = Math.min( headingColumns - 1 /* Other numbers are 0-based */, removedColumnIndexes.last ) -
removedColumnIndexes.first + 1;

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