fixed issue #546 - broken multi column sort with invisible columns

[arashdalir]

added new property allColumns and 3 new functions setAllColumns(columnDefinitions) getAllColumns(defaultToColumns) and getColumnById(columnId, useVisibleColumns)- the last function is used insetupColumnSortin order to get correct column: ifallColumns is set then useallColumnselse usecolumns` as reference list

[arashdalir]

@ghiscoding , @6pac this is a basic solution based on our discussion on #546! I have a fundamental issue with this solution though! if a user updates columns by using setColumns, the change is not necessarily reflected in allColumns which will mean that using my getColumnById might not return the correct column. I believe a more elegant solution is to somehow keep columns and allColumns synchronised in case properties of one changes. I still believe the only "right" solution is to add a property named visible: true to the columns and adapt the functions needed for rendering etc. to consider that attribute. otherwise the values in allColumns and columns with drift apart fast if setColumns or setAllColumns is set often...

[arashdalir]

or well, another solution is to add a setVisibleColumns(columnIds) function to slick.grid which will internally manage the state of allColumns and columns. in that case, using setColumns actually sets both allColumns and columns values, and setVisibleColumns reduces the columns by filtering columns according to given columnIds but keeps allColumns whenever called. in case setColumns is called after setVisibleColumns, they are synchronized again and the user must set the visibility again. this way, the getColumnById could always refer to allColumns and other functions would refer to columns.

[ghiscoding]

I would say that in any case we want to avoid pushing breaking changes as much as possible because there are thousands of people using the lib and few hundreds using my libs but I can update mine quickly. However what could potentially be done is to make the setColumns intelligent and detect, by the columns length and their position, if the lib should call the old code or the new code of setVisibleColumns that you're proposing. What I mean is that we could propose/add some new functions and if the user doesn't use it then it would use the old code, in some areas we also added some console.warn to tell the user this function is deprecated. So there are couple of alternatives you can look at, just try to avoid breaking changes

[6pac]

I've been staying out of this one, mainly because I use my own customised version of the grid, and a different customised DataView that doesn't have a lot of the limitations of the public one. Not sure if I even have this issue. I'd like to merge but the two have diverged so far that it would be a lot of work.

So I'd make two comments:

1. the easiest workaround would simply be to rewrite the sort code so it skips nonexistent columns

2. ultimately, the underlying data is the important thing. The column are just an overlay. So if the sort code was written to depend on the data properties rather than the columns, we would avoid all this mess. Perhaps it needs to cache the column property name first time around?

[arashdalir]

I would say that in any case we want to avoid pushing breaking changes as much as possible because there are thousands of people using the lib and few hundreds using my libs but I can update mine quickly. However what could potentially be done is to make the setColumns intelligent and detect, by the columns length and their position, if the lib should call the old code or the new code of setVisibleColumns that you're proposing. What I mean is that we could propose/add some new functions and if the user doesn't use it then it would use the old code, in some areas we also added some console.warn to tell the user this function is deprecated. So there are couple of alternatives you can look at, just try to avoid breaking changes

I was thinking to leave the solution to skip the not found columns in place and instead of adding the setAllColumns, and use the setVisibleColumnsById as a new feature! for that, the following changes in grid would be necessary:

1. add a new setVisibleColumnsById() and getVisibleColumns(). the getAllColumns and setAllColumns will be removed.
2. keep the allColumns and columns in the background and manage their values using setVisibleColumnsById
3. change the signature of getColumns() to getColumns(returnAllColumns:boolean=false); which will behave the same without the optional parameter (or with false) - i.e. returning visible columns; and return all columns if the parameter is set to true.

if someone doesn't use the new feature, the old logic would still work by changing the complete column set; but the new way would let them have a list of all columns in the background (and get it using getColumns(true) for returning visible and invisible columns) or just the visible ones using getColumns().

we could then extend the columnPicker to:

1. also have a setVisibleColumns which would call the setVisibleColumns in the grid.
2. use getColumns() in getVisibleColumns() and getColumns(true) in getAllColumns().
3. use setVisibleColumns function instead of grid.setColumns() here,

that way, the current functionality will stay unchanged, as the grid.getColumns() function will return the visible columns as before and expert users can use grid.getColumns(true) to use the new features.

[arashdalir]

I've been staying out of this one, mainly because I use my own customised version of the grid, and a different customised DataView that doesn't have a lot of the limitations of the public one. Not sure if I even have this issue. I'd like to merge but the two have diverged so far that it would be a lot of work.

So I'd make two comments:

1. the easiest workaround would simply be to rewrite the sort code so it skips nonexistent columns
2. ultimately, the underlying data is the important thing. The column are just an overlay. So if the sort code was written to depend on the data properties rather than the columns, we would avoid all this mess. Perhaps it needs to cache the column property name first time around?

I'd really like to see your version! :D

tbh, we are considering changing from slick-grid to another solutions like elastic-ui or quasar because slick-grid has some underlying issues which cannot be resolved that easily; like its use of absolute positioning for rows or the way detailview handles detail rows or... if your version has resolved these issues, we might be able to create a new v3 based on it and keep the 2 versions separate: the users of the current version will be able to use their branch and those who will be starting, could use the newer one. just saying...

[6pac]

Unfortunately, those are not things I changed. If anything, the UI capabilities are much less than the current grid. I don't have the split grid or the details view at all in my version (it forked about four years ago, before all that).
Most of the smaller grid features I added to my version, like column autosize, have been fed back to the public grid.
What it does have is:

• FlexTable and FlexView, which are an improved DataView, with full undo/redo, change tracking for client/server edit saves, id not needed for row logic (row index is used instead), support for array or property based rows, plus everything DataView does (FlexTable is an underlying data source and you can have multiple FlexViews each with its own sorting/filtering/grouping, etc, plus they both use the same interface for the data provider)
• a data provider to allow the grid to be interfaced with a plain js object, DataView or FlexTable/FlexView, and any other object that offers the standard interface

The idea of FlexTable/FlexView was that it would offer all the sorting/filtering/offline capabilities as a single object that could be used by any client side js component rather than each one 'rolling their own' filtering/sorting, and that it could be used as a lightweight wrapper for a plain js object, just progressively instantiating needed/used features. Ideally it could be used as a wrapper for a whole array of js 'class' business objects/models to add offline sync, filtering or aggregation.

[6pac]

Hey folks, is this still relevant or should I close it?

[arashdalir]

Hey folks, is this still relevant or should I close it?

I have actually implemented the solution as described here and it works in my version. but because of this, my deployment branch is not the active main branch anymore.

honestly, I have not checked the newest version to see the issue has been already resolved or not. I suppose, if none of us has worked on it, this will still be an open issue?

[ghiscoding]

closing in favor of #723 which is up to date with latest master branch