diff --git a/src/js/modules/Sort/Sort.js b/src/js/modules/Sort/Sort.js index 116046777..d1ca9d023 100644 --- a/src/js/modules/Sort/Sort.js +++ b/src/js/modules/Sort/Sort.js @@ -166,19 +166,20 @@ export default class Sort extends Module{ sorters=[], match = false; - if(column.modules.sort){ - if(column.modules.sort.tristate){ - if(column.modules.sort.dir == "none"){ - dir = column.modules.sort.startingDir; + const sortModule = column.modules.sort; + if(sortModule){ + if(sortModule.tristate){ + if(sortModule.dir == "none"){ + dir = sortModule.startingDir; }else{ - if(column.modules.sort.dir == column.modules.sort.startingDir){ - dir = column.modules.sort.dir == "asc" ? "desc" : "asc"; + if(sortModule.dir == sortModule.startingDir){ + dir = sortModule.dir == "asc" ? "desc" : "asc"; }else{ dir = "none"; } } }else{ - switch(column.modules.sort.dir){ + switch(sortModule.dir){ case "asc": dir = "desc"; break; @@ -188,7 +189,7 @@ export default class Sort extends Module{ break; default: - dir = column.modules.sort.startingDir; + dir = sortModule.startingDir; } } @@ -355,32 +356,29 @@ export default class Sort extends Module{ if(this.table.options.sortMode !== "remote"){ //build list of valid sorters and trigger column specific callbacks before sort begins - sortList.forEach(function(item, i){ - var sortObj; - - if(item.column){ - sortObj = item.column.modules.sort; - + for(const item of sortList) { + const column = item.column; + if(column){ + const sortObj = column.modules.sort; if(sortObj){ - //if no sorter has been defined, take a guess if(!sortObj.sorter){ - sortObj.sorter = self.findSorter(item.column); + sortObj.sorter = self.findSorter(column); } - item.params = typeof sortObj.params === "function" ? sortObj.params(item.column.getComponent(), item.dir) : sortObj.params; + item.params = typeof sortObj.params === "function" ? sortObj.params(column.getComponent(), item.dir) : sortObj.params; sortListActual.push(item); } if(!sortOnly) { - self.setColumnHeader(item.column, item.dir); + self.setColumnHeader(column, item.dir); } } - }); + } //sort data - if (sortListActual.length) { + if (sortListActual.length && data.length) { self._sortItems(data, sortListActual); } @@ -439,42 +437,57 @@ export default class Sort extends Module{ //sort each item in sort list _sortItems(data, sortList){ - var sorterCount = sortList.length - 1; - - data.sort((a, b) => { - var result; - - for(var i = sorterCount; i>= 0; i--){ - let sortItem = sortList[i]; - - result = this._sortRow(a, b, sortItem.column, sortItem.dir, sortItem.params); - + const sortMeta = sortList.map((sortItem) => { + return { + column: sortItem.column, + dir: sortItem.dir, + params: sortItem.params, + sorter: sortItem.column.modules.sort.sorter, + columnComponent: sortItem.column.getComponent(), + asc: sortItem.dir === "asc", + }; + }); + + const sorterCount = sortMeta.length - 1; + const length = data.length; + + //extract each row's sort keys and component once, then sort the decorated array + const decorated = new Array(length); + for(let k = 0; k < length; k++){ + const row = data[k]; + const rowData = row.getData(); + const values = new Array(sortMeta.length); + for(let j = 0; j <= sorterCount; j++){ + const value = sortMeta[j].column.getFieldValue(rowData); + values[j] = typeof value !== "undefined" ? value : ""; + } + decorated[k] = {values, component: row.getComponent(), row}; + } + + decorated.sort((a, b) => { + for(let i = sorterCount; i >= 0; i--){ + const sortItem = sortMeta[i]; + const asc = sortItem.asc; + const result = sortItem.sorter.call(this, + asc ? a.values[i] : b.values[i], + asc ? b.values[i] : a.values[i], + asc ? a.component : b.component, + asc ? b.component : a.component, + sortItem.columnComponent, + sortItem.dir, + sortItem.params + ); + if(result !== 0){ - break; + return result; } } - - return result; + + return 0; }); + + for(let k = 0; k < length; k++){ + data[k] = decorated[k].row; + } } - - //process individual rows for a sort function on active data - _sortRow(a, b, column, dir, params){ - var el1Comp, el2Comp; - - //switch elements depending on search direction - var el1 = dir == "asc" ? a : b; - var el2 = dir == "asc" ? b : a; - - a = column.getFieldValue(el1.getData()); - b = column.getFieldValue(el2.getData()); - - a = typeof a !== "undefined" ? a : ""; - b = typeof b !== "undefined" ? b : ""; - - el1Comp = el1.getComponent(); - el2Comp = el2.getComponent(); - - return column.modules.sort.sorter.call(this, a, b, el1Comp, el2Comp, column.getComponent(), dir, params); - } -} \ No newline at end of file +} diff --git a/src/js/modules/Sort/defaults/sorters/number.js b/src/js/modules/Sort/defaults/sorters/number.js index d96dcd364..4ee4a7648 100644 --- a/src/js/modules/Sort/defaults/sorters/number.js +++ b/src/js/modules/Sort/defaults/sorters/number.js @@ -1,40 +1,58 @@ //sort numbers export default function(a, b, aRow, bRow, column, dir, params){ + //fast-path: both already finite numbers, no separator/empty handling needed + if(typeof a === "number" && typeof b === "number" && isFinite(a) && isFinite(b)){ + return a - b; + } + var alignEmptyValues = params.alignEmptyValues; var decimal = params.decimalSeparator; var thousand = params.thousandSeparator; var emptyAlign = 0; + var aEmpty = a === "" || a === null || typeof a === "undefined"; + var bEmpty = b === "" || b === null || typeof b === "undefined"; - a = String(a); - b = String(b); + if(aEmpty){ + emptyAlign = bEmpty ? 0 : -1; + }else if(bEmpty){ + emptyAlign = 1; + }else{ + a = parseValue(a, decimal, thousand); + b = parseValue(b, decimal, thousand); - if(thousand){ - a = a.split(thousand).join(""); - b = b.split(thousand).join(""); + //handle non numeric values + if(isNaN(a)){ + emptyAlign = isNaN(b) ? 0 : -1; + }else if(isNaN(b)){ + emptyAlign = 1; + }else{ + //compare valid values + return a - b; + } } - if(decimal){ - a = a.split(decimal).join("."); - b = b.split(decimal).join("."); + //fix empty values in position + if((alignEmptyValues === "top" && dir === "desc") || (alignEmptyValues === "bottom" && dir === "asc")){ + emptyAlign *= -1; } - a = parseFloat(a); - b = parseFloat(b); + return emptyAlign; +} - //handle non numeric values - if(isNaN(a)){ - emptyAlign = isNaN(b) ? 0 : -1; - }else if(isNaN(b)){ - emptyAlign = 1; - }else{ - //compare valid values - return a - b; +function parseValue(value, decimal, thousand){ + if(typeof value === "number"){ + return value; } - //fix empty values in position - if((alignEmptyValues === "top" && dir === "desc") || (alignEmptyValues === "bottom" && dir === "asc")){ - emptyAlign *= -1; + value = String(value); + + if(thousand){ + value = value.replaceAll(thousand, ""); } - return emptyAlign; -} \ No newline at end of file + if(decimal && decimal !== "." ){ + value = value.replaceAll(decimal, "."); + } + + return parseFloat(value); +} diff --git a/test/unit/modules/SortItems.spec.js b/test/unit/modules/SortItems.spec.js new file mode 100644 index 000000000..8b1acc4ff --- /dev/null +++ b/test/unit/modules/SortItems.spec.js @@ -0,0 +1,120 @@ +import TabulatorFull from "../../../src/js/core/TabulatorFull"; + +// Correctness coverage for Sort._sortItems decorate-sort-undecorate rewrite. +// Builds real rows/columns via TabulatorFull (the Sort.spec.js pattern) and +// validates ordering + stability against a reference implementation of the +// stock per-comparison algorithm, so the tests exercise the real Row/Column +// contract rather than hand-built fakes. + +describe("Sort._sortItems (decorate-sort-undecorate)", () => { + /** @type {TabulatorFull} */ + let tabulator; + let sortMod; + + const columns = [ + { title: "A", field: "a", sorter: "number" }, + { title: "B", field: "b", sorter: "number" }, + { title: "Name", field: "name", sorter: "string" }, + { title: "Id", field: "id", sorter: "number" }, + ]; + + beforeEach(() => { + const el = document.createElement("div"); + el.id = "tabulator"; + document.body.appendChild(el); + tabulator = new TabulatorFull("#tabulator", { data: [], columns }); + sortMod = tabulator.module("sort"); + return new Promise((resolve) => tabulator.on("tableBuilt", resolve)); + }); + + afterEach(() => { + tabulator.destroy(); + document.getElementById("tabulator")?.remove(); + }); + + // Reference: an independent per-comparison oracle (the pre-rewrite sort algorithm), + // run over the same real rows/columns _sortItems sees. + function referenceSort(rows, sortList) { + const sorterCount = sortList.length - 1; + return rows.slice().sort((a, b) => { + let result = 0; + for (let i = sorterCount; i >= 0; i--) { + const { column, dir } = sortList[i]; + const { sorter, params } = column.modules.sort; + const el1 = dir === "asc" ? a : b; + const el2 = dir === "asc" ? b : a; + let av = column.getFieldValue(el1.getData()); + let bv = column.getFieldValue(el2.getData()); + av = typeof av !== "undefined" ? av : ""; + bv = typeof bv !== "undefined" ? bv : ""; + result = sorter.call(sortMod, av, bv, el1.getComponent(), el2.getComponent(), column.getComponent(), dir, params); + if (result !== 0) break; + } + return result; + }); + } + + // Load data, apply sort, and return { actual, expected, original } where actual is + // produced by Sort.sort() (which delegates to _sortItems) and expected by the reference. + async function sortData(data, sortSpec) { + await tabulator.setData(data); + const sortList = sortSpec.map(({ field, dir }) => ({ column: tabulator.columnManager.findColumn(field), dir })); + sortMod.setSort(sortList); + const original = tabulator.rowManager.activeRows.slice(); + const actual = sortMod.sort(original.slice()); + return { actual, expected: referenceSort(original, sortList) }; + } + + const ids = (rows) => rows.map((row) => row.data.id); + const field = (rows, key) => rows.map((row) => row.data[key]); + + test("single numeric column ascending", async () => { + const { actual } = await sortData([{ id: 1, a: 3 }, { id: 2, a: 1 }, { id: 3, a: 2 }], [{ field: "a", dir: "asc" }]); + expect(field(actual, "a")).toEqual([1, 2, 3]); + }); + + test("single numeric column descending", async () => { + const { actual } = await sortData([{ id: 1, a: 3 }, { id: 2, a: 1 }, { id: 3, a: 2 }], [{ field: "a", dir: "desc" }]); + expect(field(actual, "a")).toEqual([3, 2, 1]); + }); + + test("sorts the passed data array in place (same reference)", async () => { + await tabulator.setData([{ id: 1, a: 2 }, { id: 2, a: 1 }]); + sortMod.setSort([{ column: tabulator.columnManager.findColumn("a"), dir: "asc" }]); + const rows = tabulator.rowManager.activeRows.slice(); + const ref = rows; + const out = sortMod.sort(rows); + expect(out).toBe(ref); + expect(field(rows, "a")).toEqual([1, 2]); + }); + + test("multi-column ordering matches reference", async () => { + const data = [ + { id: 1, a: 2, b: 1 }, { id: 2, a: 1, b: 2 }, { id: 3, a: 1, b: 1 }, { id: 4, a: 2, b: 2 }, + ]; + const { actual, expected } = await sortData(data, [{ field: "b", dir: "asc" }, { field: "a", dir: "asc" }]); + expect(ids(actual)).toEqual(ids(expected)); + }); + + test("stability: equal keys preserve original order", async () => { + const { actual } = await sortData([{ id: 1, a: 1 }, { id: 2, a: 1 }, { id: 3, a: 1 }], [{ field: "a", dir: "asc" }]); + expect(ids(actual)).toEqual([1, 2, 3]); + }); + + test("matches reference over randomized multi-column (numeric + string, asc + desc)", async () => { + const sortSpec = [{ field: "name", dir: "desc" }, { field: "a", dir: "asc" }]; + let seed = 99; + const rnd = () => { seed = (seed * 1103515245 + 12345) & 0x7fffffff; return seed / 0x7fffffff; }; + for (let trial = 0; trial < 10; trial++) { + const data = []; + for (let i = 0; i < 60; i++) data.push({ id: i, a: Math.floor(rnd() * 5), name: "n" + Math.floor(rnd() * 5) }); + const { actual, expected } = await sortData(data, sortSpec); + expect(ids(actual)).toEqual(ids(expected)); + } + }); + + test("undefined field values are coerced to empty string (as stock)", async () => { + const { actual, expected } = await sortData([{ id: 1, a: 5 }, { id: 2 }, { id: 3, a: 2 }], [{ field: "a", dir: "asc" }]); + expect(ids(actual)).toEqual(ids(expected)); + }); +}); diff --git a/test/unit/modules/SortNumberSorter.spec.js b/test/unit/modules/SortNumberSorter.spec.js new file mode 100644 index 000000000..02b7cf571 --- /dev/null +++ b/test/unit/modules/SortNumberSorter.spec.js @@ -0,0 +1,79 @@ +import numberSorter from "../../../src/js/modules/Sort/defaults/sorters/number"; + +describe("number sorter", () => { + const asc = "asc"; + const desc = "desc"; + + const defaultParams = { + alignEmptyValues: undefined, + decimalSeparator: undefined, + thousandSeparator: undefined, + }; + + test("sorts plain numeric values", () => { + expect(numberSorter(2, 10, null, null, null, asc, defaultParams)).toBe(-8); + expect(numberSorter(10, 2, null, null, null, asc, defaultParams)).toBe(8); + expect(numberSorter(2, 2, null, null, null, asc, defaultParams)).toBe(0); + }); + + test("supports thousand separator normalization", () => { + const params = { + ...defaultParams, + thousandSeparator: ",", + }; + + expect(numberSorter("1,200", "900", null, null, null, asc, params)).toBe(300); + expect(numberSorter("900", "1,200", null, null, null, asc, params)).toBe(-300); + }); + + test("supports decimal separator normalization", () => { + const params = { + ...defaultParams, + decimalSeparator: ",", + }; + + expect(numberSorter("1,5", "1,25", null, null, null, asc, params)).toBe(0.25); + }); + + test("handles both values as non-numeric", () => { + expect(numberSorter("foo", "bar", null, null, null, asc, defaultParams)).toBe(0); + }); + + test("default empty alignment keeps non-numeric first in asc path", () => { + expect(numberSorter("foo", "1", null, null, null, asc, defaultParams)).toBe(-1); + expect(numberSorter("1", "foo", null, null, null, asc, defaultParams)).toBe(1); + }); + + test("treats empty primitive values as non-numeric", () => { + expect(numberSorter("", 1, null, null, null, asc, defaultParams)).toBe(-1); + expect(numberSorter(null, 1, null, null, null, asc, defaultParams)).toBe(-1); + expect(numberSorter(undefined, 1, null, null, null, asc, defaultParams)).toBe(-1); + }); + + test("handles mixed numeric primitives and numeric strings", () => { + const params = { + ...defaultParams, + thousandSeparator: ",", + }; + + expect(numberSorter(1200, "900", null, null, null, asc, params)).toBe(300); + expect(numberSorter("1,200", 900, null, null, null, asc, params)).toBe(300); + }); + + test("alignEmptyValues top and bottom flip as expected by direction", () => { + const topParams = { + ...defaultParams, + alignEmptyValues: "top", + }; + const bottomParams = { + ...defaultParams, + alignEmptyValues: "bottom", + }; + + expect(numberSorter("foo", "1", null, null, null, asc, topParams)).toBe(-1); + expect(numberSorter("foo", "1", null, null, null, desc, topParams)).toBe(1); + + expect(numberSorter("foo", "1", null, null, null, asc, bottomParams)).toBe(1); + expect(numberSorter("foo", "1", null, null, null, desc, bottomParams)).toBe(-1); + }); +});