Sorting fails when a one or more sorted columns is hidden

[LordGalahad]

Describe the bug

I'm not sure if this is intended behavior though.

When I have multiColumnSort enabled, and I have multiple columns currently selected like this:

And then try to hide one of the sorted columns (like GBP), so it ends up looking like this:

And then I try to click on another column (like vNoise), I get this error:

We had no problems with the old slickgrid. But since this is new implementation, it may seem to be limitation?

Reproduction

1. Set multiColumnSort to true
2. Select two or more columns to be sorted
3. Hide one of the sorted columns
4. Click on a different column for sorting

Which Framework are you using?

Other

Environment Info

System:
    OS: Windows 10 10.0.19045
    CPU: (8) x64 11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz
    Memory: 3.39 GB / 15.73 GB
  Binaries:
    Node: 18.18.2 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.19 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 9.8.1 - C:\Program Files\nodejs\npm.CMD
    pnpm: 8.15.6 - ~\AppData\Roaming\npm\pnpm.CMD
  Browsers:
    Edge: Chromium (124.0.2478.51)
    Internet Explorer: 11.0.19041.3636
  Slickgrid: 5.8.0

Validations

• Follow our Code of Conduct
• Read the Wikis.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion.
• The provided reproduction is a minimal reproducible example of the bug.

[ghiscoding]

There's not enough info/details to help you, just doing print screens of UI and code is not helpful enough for us to understand the problem. I'm not even sure if you're asking about the new hidden property on the column definition? If that is the case then @6pac could help since I'm not using that option on my side. You should provide full repro or else we cannot help

[LordGalahad]

I actually tried it on one of the samples - https://6pac.github.io/SlickGrid/examples/example-column-hidden.html

I tried setting options.multiColumnSort to true. Then I select two columns to be sorted. Then I hide one of the two sorted columns. Then I select a different column to sort. Then I get the exception error

Here is a loom video: https://www.loom.com/share/ab26e2aa6e674a2d93cc0428da2a6b2d

[6pac]

@LordGalahad thanks, too busy today, but I'll have a look tomorrow morning (GMT+09:30). There was a lot of debate around the changes, I'll have to go back and look at exactly how we did it.

[6pac]

Sorry, I'm trying to repro but I can't get the error to happen in the examples with a .js source. I notice you are referencing a .ts source directly in your examples file. How are you doing that?
This might seem a silly question, but this getting dangerously close to being only @ghiscoding 's project. I don't use TS or any of the tooling that the current repo uses. At the moment they are a significant barrier to me being able to diagnose or patch anything. I do all my diagnosis and coding in js and then have to back-patch to ts and get the dist files to flow through.

[LordGalahad]

But I simplified it by just using the sample provided by the wiki here (I'm not using my own code anymore)
https://6pac.github.io/SlickGrid/examples/example-column-hidden.html

What I just did was set a breakpoint in the sample and set options.multiColumnSort to TRUE

Then, shift + click each column to sort

Then hide one of the sorted columns (I hid % Complete by right clicking on the header and deselecting it)

Then shift +click on another column to sort it as well. (like Start Date)

When this happens, there will an exception occurring.

If you use ordinary click, you will encounter no problems

[6pac]

@LordGalahad thanks, I could repro. I was missing the final shift-click.

@ghiscoding this is actually nothing to do with the hidden property - it's happening because the column menu is not using the hidden property but is removing the column from the columns array, while it still is listed in the column sort array. This must have been a pre-existing bug. I have created a PR to fix. (note - bugs like this are exactly why we needed the hidden property - keeping multiple column lists in sync across possibly multiple components is a nightmare)

Both of you: this does raise another usage question. If we are ordering by a column and it is then hidden, should it be removed from the sort list? I can see use cases either way. Should we create an option, say removeHiddenColsFromSort, defaulting to false so as to be non breaking? This would require additional changes, which I can do.

[ghiscoding]

@6pac that bug was also reported in the past when you introduced the hidden feature, see #546
I didn't look at the code, I just remembered that it was a similar bug

[6pac]

@ghiscoding ah yes, good catch. Any opinion on the usage question?

[ghiscoding]

Any opinion on the usage question?

I'm not sure what you mean, could you elaborate?

[6pac]

Both of you: this does raise another usage question. If we are ordering by a column and it is then hidden, should it be removed from the sort list? I can see use cases either way. Should we create an option, say removeHiddenColsFromSort, defaulting to false so as to be non breaking? This would require additional changes, which I can do.

This is referring to the use of the hidden flag, where the column is still in the list

[ghiscoding]

I was ok with your PR approach, I would find that weird that we would still sort by a column that is not visible to the user since it could bring too much confusion (I like the approach of "What you see is what you get")

[6pac]

OK, no worries, but in that case I should also filter for columns that exist but are hidden. Will tweak it some more.

Thanks for the hint on debugging too.

[ghiscoding]

should be fixed in v5.9.2

[LordGalahad]

Yup it is fixed. Thanks