BUG: hiding a column by which the table is sorted breaks multi column sorting

[arashdalir]

please use the following example:

1. remove the .txt suffix,
2. put it in the examples folder
3. order by % complete, start.
4. hide start column.
5. add finish to sort

voila! check your browser's console, and you will see the message! the behavior is visualized in the following GIF.

the reason behind this behavior is that slick-grid changes the entire column-set each time a column is set to be hidden; which causes the getColumnIndex function to return null, as the hidden column is not on the list of columns; obviously!

the solution is more complicated as far as I can tell! because many plugins and existing codes depend on the current implementation of the getColumns and getColumnIndex functions, it's not easy to add a property - for example, called visible ` to the column definition in order to control column visibility. hiding or showing columns shouldn't be a matter or setting a new column-set, but rather a matter of changing their visibility and redrawing the whole table!

example-multi-column-sort.html-broken.txt

[ghiscoding]

Unfortunately, you can't change how columns are being hidden/shown, if you do that will be a major breaking change and will break many user's grids including all my libs. I agree that it shouldn't have been done that way but that's the way it is and we're stuck with the behavior, which is to call setColumns(visibleColumns).

The only possible and acceptable way to fix this would be to add a new variable internally (allColumns) and add a new function getAllColumns() and probably a setter as well that would rarely be called setAllColumns, the idea would be to set the allColumns once on grid creation and never touch it (or almost never) and keep using setColumns for visibility which won't affect the allColumns local copy. I use this technique in Angular-Slickgrid when I need to change locale, it needs to loop through allColumns and I can't use getColumns because as you said that provides me only with the visible ones but I still need to translate the hidden ones so I use the local copy of allColumns array as shown below. I am not using this technique while sorting but this reminds me that I probably should, currently I'm discarding any columns that isn't visible from the Sort. It would be great if SlickGrid supports this out of the box and creating this new local copy of the columns and new functions is the only way I see to do it without breaking everyone's grid, it's always better to introduce something new instead of refactoring something that you know will break everyone.

[arashdalir]

@ghiscoding
technically we have the list of allColumns - at least the columnPicker does! but the function which is throwing the exception is an internal slick.grid one, which is used to create the list of columns by which the data is to be sorted, and passes the list to my actual sorting function. I cannot override that function unless I rewrite the slick.grid.js!

I believe if we could add a flag to getColumns and getColumnIndex - like showAll or so, then we might be able to resolve this issue by forcing such internal functionalities to use the list of all columns and the others would still have the same output as before.

anyways, line 1412, the 2 getColumnIndex functions are failing! any other ideas on how we can resolve this issue?

// slick.grid.js:1409-1414
/*1409*/ trigger(self.onSort, {
/*1410*/      multiColumnSort: true,
/*1411*/      sortCols: $.map(sortColumns, function(col) {
/*1412*/          return {columnId: columns[getColumnIndex(col.columnId)].id, sortCol: columns[getColumnIndex(col.columnId)], sortAsc: col.sortAsc };
/*1413*/     })
/*1414*/}, e);

[ghiscoding]

That code is not bullet proof and should be rewritten a bit to avoid throwing errors, something like this maybe.

var columnIdx = getColumnIndex(col.columnId) || -1;
var column = columnIdx >= 0 ? columns[columnIdx] : null;
return {columnId: column && column.id , sortCol: column, sortAsc: col.sortAsc };

technically we have the list of allColumns - at least the columnPicker does!

No it doesn't, it assumes that you will pass all columns when you create the column picker, it creates a local copy of the columns at grid creation then uses that copy when it needs to refer to all columns, it also has a 2nd variable visibleColumns when it refers to only visible columns. There's a comment written in the code on this line that explains well why you can't just use getColumns

So in brief, what I wrote in previous post is still the best way (probably the only way) to go, the library should have a copy of allColumns be created at grid creation and the onSort should use it. If we do that then some plugins/code could be rewritten to use the getAllColumns instead and simplify the code. There might be a slight problem though, because the setColumns is also used for column ordering while the allColumns won't always have the correct order because it would typically never be updated (which the column picker does require column ordering, so perhaps we would have to keep current working code in column picker).

[6pac]

My 2c - in setColumns() should we just iterate sortcols and remove any missing column ids? This would mean that the hidden column was removed from the sort, but it would prevent an error occurring.

[ghiscoding]

I would say Yes disregard columns that don't exist at least until someone takes the time to add something like allColumns that I've talked before.

[arashdalir]

@ghiscoding @6pac I'd rather work on theallColumns solution, especially as it product definitely needs it. Any idea which features might need it, other than column picker and sort?

[ghiscoding]

You are most welcome to create a PR to add the allColumns, the GridMenu also uses the same code as ColumnPicker to show all columns, in my libs I a copy of allColumns on ColumnPicker, Grid, on Filtering and Sorting and that seems to be it

[arashdalir]

@ghiscoding I took a more accurate look at how the setupColumnSort function, in which this error occurs, works.

unfortunately, the functions getColumns and getColumnIndex do not use the same variable; therefore, the allColumns will not resolve this issue directly. I believe, in order to resolve this specific issue I will need to define a getColumnById(id, useOnlyVisibleColumns) function. will post my solution as soon as I am finished...

[ghiscoding]

this should be fixed by #765 and released under the new v4.0.0-beta.0, so it should be safe to close this issue