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

I/6569: Remove array sorting flaws from table code #305

Merged
merged 6 commits into from
Apr 17, 2020
Merged

I/6569: Remove array sorting flaws from table code #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
39487ac
Add tests for indexes sorting bug.
jodator Apr 15, 2020
55dc956
Introduce getColumnIndexes helper method.
jodator Apr 15, 2020
2a2d192
Fix sorting indexes array bug.
jodator Apr 15, 2020
81b7727
Merge branch 'master' into i/6569
jodator Apr 15, 2020
fb04591
Use proper variable names in getFirstLastIndexesObject() internals.
jodator Apr 17, 2020
55ba7c1
Match tests with their descriptions.
jodator Apr 17, 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
12 changes: 5 additions & 7 deletions src/commands/removecolumncommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
import Command from '@ckeditor/ckeditor5-core/src/command';

import TableWalker from '../tablewalker';
import { getSelectionAffectedTableCells } from '../utils';
import { getColumnIndexes, getSelectionAffectedTableCells } from '../utils';
import { findAncestor } from './utils';

/**
* The remove column command.
Expand All @@ -32,15 +33,12 @@ export default class RemoveColumnCommand extends Command {
const firstCell = selectedCells[ 0 ];

if ( firstCell ) {
const table = firstCell.parent.parent;
const table = findAncestor( 'table', firstCell );
const tableColumnCount = this.editor.plugins.get( 'TableUtils' ).getColumns( table );

const tableMap = [ ...new TableWalker( table ) ];
const columnIndexes = tableMap.filter( entry => selectedCells.includes( entry.cell ) ).map( el => el.column ).sort();
const minColumnIndex = columnIndexes[ 0 ];
const maxColumnIndex = columnIndexes[ columnIndexes.length - 1 ];
const { first, last } = getColumnIndexes( selectedCells );

this.isEnabled = maxColumnIndex - minColumnIndex < ( tableColumnCount - 1 );
this.isEnabled = last - first < ( tableColumnCount - 1 );
} else {
this.isEnabled = false;
}
Expand Down
28 changes: 9 additions & 19 deletions src/commands/setheadercolumncommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,8 @@

import Command from '@ckeditor/ckeditor5-core/src/command';

import {
updateNumericAttribute,
isHeadingColumnCell
} from './utils';
import { getSelectionAffectedTableCells } from '../utils';
import { findAncestor, isHeadingColumnCell, updateNumericAttribute } from './utils';
import { getColumnIndexes, getSelectionAffectedTableCells } from '../utils';

/**
* The header column command.
Expand Down Expand Up @@ -66,26 +63,19 @@ export default class SetHeaderColumnCommand extends Command {
* the `forceValue` parameter instead of the current model state.
*/
execute( options = {} ) {
const model = this.editor.model;
const tableUtils = this.editor.plugins.get( 'TableUtils' );

const selectedCells = getSelectionAffectedTableCells( model.document.selection );
const firstCell = selectedCells[ 0 ];
const lastCell = selectedCells[ selectedCells.length - 1 ];
const tableRow = firstCell.parent;
const table = tableRow.parent;

const [ selectedColumnMin, selectedColumnMax ] =
// Returned cells might not necessary be in order, so make sure to sort it.
[ tableUtils.getCellLocation( firstCell ).column, tableUtils.getCellLocation( lastCell ).column ].sort();

if ( options.forceValue === this.value ) {
return;
}

const headingColumnsToSet = this.value ? selectedColumnMin : selectedColumnMax + 1;
const model = this.editor.model;
const selectedCells = getSelectionAffectedTableCells( model.document.selection );
const { first, last } = getColumnIndexes( selectedCells );

const headingColumnsToSet = this.value ? first : last + 1;

model.change( writer => {
const table = findAncestor( 'table', selectedCells[ 0 ] );

updateNumericAttribute( 'headingColumns', headingColumnsToSet, table, writer, 0 );
} );
}
Expand Down
24 changes: 8 additions & 16 deletions src/commands/setheaderrowcommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@

import Command from '@ckeditor/ckeditor5-core/src/command';

import { createEmptyTableCell, updateNumericAttribute } from './utils';
import { getSelectionAffectedTableCells } from '../utils';
import { createEmptyTableCell, findAncestor, updateNumericAttribute } from './utils';
import { getRowIndexes, getSelectionAffectedTableCells } from '../utils';
import TableWalker from '../tablewalker';

/**
Expand Down Expand Up @@ -62,24 +62,16 @@ export default class SetHeaderRowCommand extends Command {
* the `forceValue` parameter instead of the current model state.
*/
execute( options = {} ) {
const model = this.editor.model;

const selectedCells = getSelectionAffectedTableCells( model.document.selection );
const firstCell = selectedCells[ 0 ];
const lastCell = selectedCells[ selectedCells.length - 1 ];
const table = firstCell.parent.parent;

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

const [ selectedRowMin, selectedRowMax ] =
// Returned cells might not necessary be in order, so make sure to sort it.
[ firstCell.parent.index, lastCell.parent.index ].sort();

if ( options.forceValue === this.value ) {
return;
}
const model = this.editor.model;
const selectedCells = getSelectionAffectedTableCells( model.document.selection );
const table = findAncestor( 'table', selectedCells[ 0 ] );

const headingRowsToSet = this.value ? selectedRowMin : selectedRowMax + 1;
const { first, last } = getRowIndexes( selectedCells );
const headingRowsToSet = this.value ? first : last + 1;
const currentHeadingRows = table.getAttribute( 'headingRows' ) || 0;

model.change( writer => {
if ( headingRowsToSet ) {
Expand Down
43 changes: 37 additions & 6 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import { isWidget, toWidget } from '@ckeditor/ckeditor5-widget/src/utils';
import { findAncestor } from './commands/utils';
import TableWalker from './tablewalker';

/**
* Converts a given {@link module:engine/view/element~Element} to a table widget:
Expand Down Expand Up @@ -146,16 +147,46 @@ export function getSelectionAffectedTableCells( selection ) {
*
* console.log( `Selected rows ${ first } to ${ last }` );
*
* @package {Array.<module:engine/model/element~Element>}
* @param {Array.<module:engine/model/element~Element>}
* @returns {Object} Returns an object with `first` and `last` table row indexes.
*/
export function getRowIndexes( tableCells ) {
const allIndexesSorted = tableCells.map( cell => cell.parent.index ).sort();
const indexes = tableCells.map( cell => cell.parent.index );

return {
first: allIndexesSorted[ 0 ],
last: allIndexesSorted[ allIndexesSorted.length - 1 ]
};
return getFirstLastIndexesObject( indexes );
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

A couple of documentation issues here. Wording, wrong param name, lack of param name in the @param line, etc. I'm going to fix that on master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

* Returns a helper object with `first` and `last` column index contained in given `tableCells`.
*
* const selectedTableCells = getSelectedTableCells( editor.model.document.selection );
*
* const { first, last } = getColumnIndexes( selectedTableCells );
*
* console.log( `Selected columns ${ first } to ${ last }` );
*
* @param {Array.<module:engine/model/element~Element>}
* @returns {Object} Returns an object with `first` and `last` table column indexes.
*/
export function getColumnIndexes( selectedCells ) {
const table = findAncestor( 'table', selectedCells[ 0 ] );
const tableMap = [ ...new TableWalker( table ) ];

const indexes = tableMap
.filter( entry => selectedCells.includes( entry.cell ) )
.map( entry => entry.column );

return getFirstLastIndexesObject( indexes );
}

// Helper method to get an object with `first` and `last` indexes from an unsorted array of indexes.
function getFirstLastIndexesObject( indexes ) {
const allIndexesSorted = indexes.sort( ( indexA, indexB ) => indexA - indexB );

const first = allIndexesSorted[ 0 ];
const last = allIndexesSorted[ allIndexesSorted.length - 1 ];

return { first, last };
}

function sortRanges( rangesIterator ) {
Expand Down
29 changes: 29 additions & 0 deletions tests/commands/mergecellscommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,35 @@ describe( 'MergeCellsCommand', () => {

expect( command.isEnabled ).to.be.false;
} );

it( 'should be false if more than 10 rows selected and some are in heading section', () => {
setData( model, modelTable( [
[ '0' ],
[ '1' ],
[ '2' ],
[ '3' ],
[ '4' ],
[ '5' ],
[ '6' ],
[ '7' ],
[ '8' ],
[ '9' ],
[ '10' ],
[ '11' ],
[ '12' ],
[ '13' ],
[ '14' ]
], { headingRows: 10 } ) );

const tableSelection = editor.plugins.get( TableSelection );
const modelRoot = model.document.getRoot();
tableSelection._setCellSelection(
modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
modelRoot.getNodeByPath( [ 0, 12, 0 ] )
);

expect( command.isEnabled ).to.be.false;
} );
} );

describe( 'execute()', () => {
Expand Down
15 changes: 15 additions & 0 deletions tests/commands/removecolumncommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,21 @@ describe( 'RemoveColumnCommand', () => {
expect( command.isEnabled ).to.be.false;
} );

it( 'should be false if all columns are selected - table with more than 10 columns (array sort bug)', () => {
setData( model, modelTable( [
[ '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12' ]
] ) );

const tableSelection = editor.plugins.get( TableSelection );
const modelRoot = model.document.getRoot();
tableSelection._setCellSelection(
modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
modelRoot.getNodeByPath( [ 0, 0, 12 ] )
);

expect( command.isEnabled ).to.be.false;
} );

it( 'should be false if selection is outside a table', () => {
setData( model, '<paragraph>11[]</paragraph>' );

Expand Down
62 changes: 62 additions & 0 deletions tests/commands/removerowcommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,33 @@ describe( 'RemoveRowCommand', () => {

expect( command.isEnabled ).to.be.false;
} );

it( 'should be false if all the rows are selected - table with more than 10 rows (array sort bug)', () => {
setData( model, modelTable( [
[ '0' ],
[ '1' ],
[ '2' ],
[ '3' ],
[ '4' ],
[ '5' ],
[ '6' ],
[ '7' ],
[ '8' ],
[ '9' ],
[ '10' ],
[ '11' ],
[ '12' ]
] ) );

const tableSelection = editor.plugins.get( TableSelection );
const modelRoot = model.document.getRoot();
tableSelection._setCellSelection(
modelRoot.getNodeByPath( [ 0, 0, 0 ] ),
modelRoot.getNodeByPath( [ 0, 12, 0 ] )
);

expect( command.isEnabled ).to.be.false;
} );
} );

describe( 'execute()', () => {
Expand Down Expand Up @@ -339,6 +366,41 @@ describe( 'RemoveRowCommand', () => {

expect( createdBatches.size ).to.equal( 1 );
} );

it( 'should properly remove more than 10 rows selected (array sort bug)', () => {
setData( model, modelTable( [
[ '0' ],
[ '1' ],
[ '2' ],
[ '3' ],
[ '4' ],
[ '5' ],
[ '6' ],
[ '7' ],
[ '8' ],
[ '9' ],
[ '10' ],
[ '11' ],
[ '12' ],
[ '13' ],
[ '14' ]
] ) );

const tableSelection = editor.plugins.get( TableSelection );
const modelRoot = model.document.getRoot();
tableSelection._setCellSelection(
modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
modelRoot.getNodeByPath( [ 0, 12, 0 ] )
);

command.execute();

assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [
[ '0' ],
[ '13' ],
[ '14' ]
] ) );
} );
} );

describe( 'with entire row selected', () => {
Expand Down
47 changes: 47 additions & 0 deletions tests/commands/selectrowcommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,53 @@ describe( 'SelectRowCommand', () => {
[ 0, 0 ]
] );
} );

it( 'should properly select more than 10 rows selected (array sort bug)', () => {
setData( model, modelTable( [
[ '0', 'x' ],
[ '1', 'x' ],
[ '2', 'x' ],
[ '3', 'x' ],
[ '4', 'x' ],
[ '5', 'x' ],
[ '6', 'x' ],
[ '7', 'x' ],
[ '8', 'x' ],
[ '9', 'x' ],
[ '10', 'x' ],
[ '11', 'x' ],
[ '12', 'x' ],
[ '13', 'x' ],
[ '14', 'x' ]
] ) );

const tableSelection = editor.plugins.get( TableSelection );
const modelRoot = model.document.getRoot();
tableSelection._setCellSelection(
modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
modelRoot.getNodeByPath( [ 0, 12, 0 ] )
);

command.execute();

assertSelectedCells( model, [
[ 0, 0 ], // '0'
[ 1, 1 ], // '1'
[ 1, 1 ], // '2'
[ 1, 1 ], // '3'
[ 1, 1 ], // '4'
[ 1, 1 ], // '5'
[ 1, 1 ], // '6'
[ 1, 1 ], // '7'
[ 1, 1 ], // '8'
[ 1, 1 ], // '9'
[ 1, 1 ], // '10'
[ 1, 1 ], // '11'
[ 1, 1 ], // '12'
[ 0, 0 ], // '13'
[ 0, 0 ] // '14
] );
} );
} );

describe( 'with entire row selected', () => {
Expand Down
19 changes: 19 additions & 0 deletions tests/commands/setheadercolumncommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,25 @@ describe( 'SetHeaderColumnCommand', () => {
[ 0, 1, 1, 0 ]
] );
} );

it( 'should set it correctly in table with more than 10 columns (array sort bug)', () => {
setData( model, modelTable( [
[ '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12', '13', '14' ]
] ) );

const tableSelection = editor.plugins.get( TableSelection );
const modelRoot = model.document.getRoot();
tableSelection._setCellSelection(
modelRoot.getNodeByPath( [ 0, 0, 2 ] ),
modelRoot.getNodeByPath( [ 0, 0, 13 ] )
);

command.execute();

assertEqualMarkup( getData( model, { withoutSelection: true } ), modelTable( [
[ '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12', '13', '14' ]
], { headingColumns: 14 } ) );
} );
} );
} );

Expand Down
Loading