freezing columns after reorder, depth check?

[arthur-clifford]

@ghiscoding or @6pac I have identified an issue and what I think to be the offending lines of code, I can make the changes and do a PR if you want but I want to know if you have any insights as to why the code is doing the check it doing.

I'll share the offending code first,: (in slick.grid)

function setColumns(columnDefinitions) {
      var _treeColumns = new Slick.TreeColumns(columnDefinitions);
     
      if (_treeColumns.hasDepth()) {
        treeColumns = _treeColumns;
        columns = treeColumns.extractColumns();
      } else {
        columns = columnDefinitions;
      }
     ...
}

The conditional above is testing whether treeColumns has depth and only sets treeColumns if it does.

The problem this causes is that the functions for renedering stuff use treeColumns.extractColumns() . setColumns is called after a column reorder.

Thus, when you reorder columns they appear ok, but when you freeze a column the column order reverts to the order held by treeColumns not by columns.

Visual Aid: Grid displaying data

Dragging column cnum to a new location Note: cnum drags without a problem

Anchor/Freeze; freezing columns at cnum column at its new location.

Result of anchor: Note: the freeze happens at the right column offset but crnum jumped back to its original location.

Solution (?) I noticed in the init function this is how column definitions are initially handled

function init() {
     ...
     treeColumns = new Slick.TreeColumns(columns);
     columns = treeColumns.extractColumns();
     ...
}

Note: no depth check

Given that I tried:

    function setColumns(columnDefinitions) {
      var _treeColumns = new Slick.TreeColumns(columnDefinitions);
      treeColumns = _treeColumns;
      columns = treeColumns.extractColumns();

      /*if (_treeColumns.hasDepth()) {
        treeColumns = _treeColumns;
        columns = treeColumns.extractColumns();
      } else {
        columns = columnDefinitions;
      }*/
    ...
    }

And that solved the problem of the column jumping to its original position.

Is there a reason for the depth check?

[ghiscoding]

most probably related to this other issue #517 which the user did provide possible code change. I do not know that piece of code, not sure what it does, so I can't say much about it myself. Perhaps @6pac knows more about it.

[arthur-clifford]

From comments on #517:

"I found the treeColumns is a featurre added in the https://github.com/ddomingues/X-SlickGrid. And it is introduced when introducing the forzen feature (Related: #26 (comment))"

Umm … and I say this semi kiddingly … technically Ghislain as the one who did the merging for frozen rows and therefore integrated Slick.TreeColumns you probably are the only one that might have familiarity with the code … just sayin’. Given @6pac/SlickGrid didn’t want to implement frozen columns in the first place I’m sure he’s resisting saying “I told you so” ;). More seriously, though, I get it. You were merging from elsewhere, I’m glad you did and don’t envy the effort you had to put in, we just have to do some little tweaks to improve the implementation.

Exploring the problem space deeper: Slick.TreeColumns was introduced in commit: 553d9c0876f14c05386059b5491bb35331963b4c

As was the condition check.

treeColumns.extractColumns is referred to 4 times within slick.grid:

• in init
• in setOptions
• in setColumns
• in createColumnHeaders when setting up options.enableColumnReorder(…

In setOptions, setColumns(treeColumns.extractColumns()) is called.

My suspicion is that the condition was meant to be a way of checking for the scenario of just needing to update columns because treeColumns was already set.

Looking at the definition for Slick.TreeColumns function hasDepth()

     this.hasDepth = function () {

      for (var i in treeColumns)
        if (treeColumns[i].hasOwnProperty('columns'))
          return true;
      return false;
    };

This would only return true if columns have sub-columns.

Is that even a thing SlickGrid is supporting right now?

So hasDepth doesn’t mean hasColumns it means that there are columns below the first level in the tree.

In any regard, the condition check is definitely blocking reordering rows with freezing columns as I illustrated and which was attempted to be described in #517 which is the same topic without the visuals.

The bottom line is that if we are setting the columns for any reason the treeColumns MUST BE updated too (if they weren’t already). Setting treeColumns with the columnDefs and then setting columns using treeColumns.extractColumns guarantees treeColumns and columns are in sync.

My proposition is this … pseudocode based on typescript for simpler communication):

   setColumns(columns?:Array<ColumnDefinition>, optUpdateTreeColumns?:Boolean = true) {
                if(optUpdateTreeColumns === true) {
                                treeColumns = new Slick.TreeColumns(columnDefinitions);
                }
                columns = treeColumns.extractColumnDefinitions();
                …
    }

// Anything currently calling setColumns would end up setting the treeColumns and columns.
// In setOptions the call to setColumns call would be changed to:
                setColumns(null,true);

                // in other words ,the tree columns already have the current columns so just update the columns from that and do the rest of setColumns stuff.

If I were to implement this for the 6pac repo I would check for whether optUpdateTreeColumns is set and set it to a default etc and not do the typescripty stuff.

The other option is to make a copy of setColumns called setReorderedColumns that sets treeColumns and extracts the columns. That function could be passed to the reorder function rather than setColumns.

[ghiscoding]

You were merging from elsewhere, I’m glad you did and don’t envy the effort you had to put in, we just have to do some little tweaks to improve the implementation.

At the time, I had spent my Christmas holiday and more on this merge exercise, probably close to a month in total, that is not something I want to redo (not a fun part but I wanted the feature) and yes there were some part of the code that I missed when merging (some part removed by mistake, some part not merged correctly for them to work correctly) and without too much testing (E2E or any other tests), it's hard to catch everything. Like I said, I don't understand that piece of code, so I can't comment much on it, if you find something that works and that does fix or enhance it closer to the expected behavior then go for it (I would suggest to run test the UI yourself but also to run all the Cypress E2E tests in Angular-Slickgrid to make sure it doesn't break anything).

Here I found the commit for that tree column thing on the X-SlickGrid repo, here's the commit that introduces tree column. It has code about checkbox and grouping, so it might be 1 of the 2 (might be more the latter), so it might just be a bad naming

[6pac]

We are in a position where there are a great number of plugins and core extensions to SlickGrid. While all of them have been tested and work in isolation, we can't guarantee that they will work in combination with each other.
It looks like here the TreeColumns feature snuck in as part of a larger change. I'm not aware of what it's supposed to do, and I don't think we have an example or test for it (which is as close as we get to documentation :-0).
So if @ghiscoding concurs, I think we'd be happy for that feature to either be removed entirely, or if you have code to fix it and make sense of it (explanatory comments in the code would be most welcome!) then that would be great too.

[arthur-clifford]

Hi Ben (and Ghislain),

So reviewing what is going on in the code. It seems to be what is happening is that the frozen column tech is leveraging TreeColumns to have a shallow tree of columns dividing things for the left and right columns. The extractColumns function basically flattens the column defintions back into a single array.

I don't know that I would recommend trying to purge TreeColumns unless Ghislain or you ,and likely not me has a holiday to burn on it. Its got tendrils in slick.grid and slick.core and it wouldn't be a quick task.

What the treeColumns variable provides you is access to the grid columns whether they are flat or grouped (left and right groups) in a way that you can access the column definitions without having to go to the left column headers vs the right column headers or doing math with the frozen column id/index to work with frozen vs non frozen columns; not a bad thing to have around if you find a need for such a thing. And, again, its extractColumns function can give you a flat column list which works whether you are freezing columns are not.

What I can't quite understand based on code I've seen is why anyone would not want to set treeColumns. It fundamentally boils down to the code in setColumns:

    var _treeColumns = new Slick.TreeColumns(columnDefinitions);
      if (_treeColumns.hasDepth()) {
        treeColumns = _treeColumns;
        columns = treeColumns.extractColumns();
      } else {
        columns = columnDefinitions;
      }

The only thing I can think of is there is no point in setting treeColumns if there isn't depth. It may be possible that the grid would do something more if treeColumns is set, but most things that expect it seem to be checking for depth.

I can tell you that in my current project where I've hacked slickgrid a bit in ways I'll share back to you as time permits, I've leveraged:

• Grouping
• Aggregation
• Draggable Columns
• Frozen Columns And have used/added abilities to: -Move column to the front (after any frozen columns) -Anchor columns (freeze up to a column using menu command) -Anchor to front (moves a column to the start and then sets that as the frozen column) -I've even created a custom filter that allows me to filter the unfrozen columns rather than the rows and to anchor/freeze and/or move filtered columns to the front in a way that when the filter is removed the frozen columns are still at the front.

Nothing above involving column ordering works with the conditional above in place. I made it work by commenting it out and keeping:

      treeColumns = new Slick.TreeColumns(columnDefinitions);;
      columns = treeColumns.extractColumns();

That guarantees treeColumns is set properly after setColumns is called. And it works when frozenColumn === -1

[ghiscoding]

That is good to know it's useful, perhaps you can push whatever necessary fix for it to work properly and possible a new Example that could demo what you talked about.

[arthur-clifford]

I will try to get that done in the next couple days.

I’m not sure its doing what I thought it was.

It definitely allows for hierarchical column structure. Almost everything it does is providing recursive variants of what you would normally need to do with a set of columns, like map to id will recursively go through whatever the column hierarchy is and give you back a flat mapping.

However, I have looked at the example page examples/example-frozen-columns-and-rows.html and monitored the setColumns function and treeColumns doesn’t have any depth. Meaning it is a flat tree even when there is column and row grouping.

That means that treeColumns is not necessary for column or row freezing.

It is also not detecting anything regarding the group property so its getColumnsInGroup function is confusing, it is more like getColumnsForNode

One clue as to why there’s an issue is:

examples/example-frozen-columns-and-rows.html

You may note that in that demo the columns are dragable (only within frozen or within unfrozen but not between)

However, if you look at included scripts the DraggableGrouping pluging is NOT included in the page,

There are minor differences between the internal drop/stop handler and the DgrabbleColumnsPlugin stop handelrs. The slick.grid stop handler uses treeColumns to validate whether the columns are validly allowed in their “groups” if it has a depth and defines a limit variable passed to the event triggered at the end. The DraggableGrouping does not.

I think I’ll need to setup a copy of frozen rows and columns with the DraggableGroupingPlugin included and see if I can demonstrate the problem.

A couple questions:

1. Is there a workflow you prefer for putting up more of a problem example rather than a product example? Is there an official 6pac/slickgrid JSFiddle or similar something somewhere I should use, or a folder you can exclude from official release commits?
2. Why does:

a. examples/example-multi-grid-basic.html not allow dragging columns without any obvious options to enable it? b. but examples/example-frozen-columns-and-rows.html does allow draggable columns without a plugin and without any options turning it on?

From: Ghislain B. @.> Sent: Sunday, April 4, 2021 8:33 AM To: 6pac/SlickGrid @.> Cc: arthur-clifford @.>; Author @.> Subject: Re: [6pac/SlickGrid] freezing columns after reorder, depth check? (#592)

That is good to know it's useful, perhaps you can push whatever necessary fix for it to work properly and possible a new Example that could demo what you talked about.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/6pac/SlickGrid/issues/592#issuecomment-813052984 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6BJW24ZN3XURGMNK3F2UTTHCBBXANCNFSM42BADZ7Q . https://github.com/notifications/beacon/AE6BJW6VOW6KTGNCHIKF643THCBBXA5CNFSM42BADZ72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGB3DIOA.gif

[ghiscoding]

1. Is there a workflow you prefer for putting up more of a problem example rather than a product example? Is there an official 6pac/slickgrid JSFiddle or similar something somewhere I should use, or a folder you can exclude from official release commits?

There's no official fidle or anything, you just take an Example and modify it a bit.

2. Why does: a. examples/example-multi-grid-basic.html not allow dragging columns without any obvious options to enable it? b. but examples/example-frozen-columns-and-rows.html does allow draggable columns without a plugin and without any options turning it on?

Probably because of this line grid.setSelectionModel(new Slick.RowSelectionModel());, in Angular-Slickgrid I added a lot of grid options flags but in SlickGrid directly, you need to call and instantiate all the plugins you need while in Angular-Slickgrid it's with flags (which is purposely easier and it also helps to really instantiate only when flag are enabled)

I wonder if there's anything related to this Tree Example in any ways.

[arthur-clifford]

I wonder if there's anything related to this Tree Example https://6pac.github.io/SlickGrid/examples/example5-collapsing.html in any ways.

If you go there then,

In the browser, putting a stop in slick.grid.js setColumns function first line(~2852) Add a watches:

treeColumns.hasDepth()

treeColumns.getDepth()

I got:

treeColumns.hasDepth(): false

treeColumns.getDepth(): 1

I confirmed also that the incoming column definitions did not have a columns property with an array of column definitiosn which is an indication that it was not intended for TreeColumns

That makes sense; the tree is of rows.

TreeColumns seems to be more about hierarchical columns

You can kind of imagine how that would be useful for column groups where the first level is a set of headers and then the second row is headers for each column for the child columns like in

You would think it would be implemented here:

https://6pac.github.io/SlickGrid/examples/example-frozen-columns-and-column-group.html

But looking at the columns going in they don’t have sub columns either just a grouping property. When I have a bit more time to do exploratory dev I might try to do a simpler variant of that example that uses columns with sub-columns rather than what is done in the demo and see what we get.

There is a section of init that looks at whether treeColumns has depth and if so populates $groupsHeadersL and $groupHeadersR which are apparently arrays where each index is a depth. And the values are div tags with appropriate classes for left or right pane column headers. It then sets the $groupHeaders array $groupHeaders = $().add($groupHeadersL).add($groupHeadersR);

I’m not sure what the effect of that is.

Nothing I’ve seen thus far is actually resulting in a treeColumns that has a depth unless that depth is getting called later than I’m watching for it.

Did the slickgrid repo you got the frozen tech from allow for column grouping like in the link above? I know groping in this repo is a bit different. I’m wondering if that is the disjoint?

Art

From: Ghislain B. @.> Sent: Sunday, April 4, 2021 8:11 PM To: 6pac/SlickGrid @.> Cc: arthur-clifford @.>; Author @.> Subject: Re: [6pac/SlickGrid] freezing columns after reorder, depth check? (#592)

1. Is there a workflow you prefer for putting up more of a problem example rather than a product example? Is there an official 6pac/slickgrid JSFiddle or similar something somewhere I should use, or a folder you can exclude from official release commits?

There's no official fidle or anything, you just take an Example and modify it a bit.

2. Why does: a. examples/example-multi-grid-basic.html not allow dragging columns without any obvious options to enable it? b. but examples/example-frozen-columns-and-rows.html does allow draggable columns without a plugin and without any options turning it on?

Probably because of this line grid.setSelectionModel(new Slick.RowSelectionModel());, in Angular-Slickgrid I added a lot of grid options flags but in SlickGrid directly, you need to call and instantiate all the plugins you need while in Angular-Slickgrid it's with flags (which is purposely easier and it also helps to really instantiate only when flag are enabled)

I wonder if there's anything related to this Tree Example https://6pac.github.io/SlickGrid/examples/example5-collapsing.html in any ways.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/6pac/SlickGrid/issues/592#issuecomment-813157372 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6BJW7WSPJE77CJEOU7QM3THES5PANCNFSM42BADZ7Q . https://github.com/notifications/beacon/AE6BJW26S6WZONNALYOU6GLTHES5PA5CNFSM42BADZ72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGB34X7A.gif

[arthur-clifford]

2. Why does: a. examples/example-multi-grid-basic.html not allow dragging columns without any obvious options to enable it? b. but examples/example-frozen-columns-and-rows.html does allow draggable columns without a plugin and without any options turning it on?

There is actually a grid option for enableColumnReorder which I didn’t see.

That has to be enabled and the jquery.ui.min.js has to be included for the JQuery Sortable tech to work.

The DraggableGroupingPlugin is still important because that allows dragging out to the dropbox.

Here’s where my understanding is as of now:

Primary Situation

1. DraggableGroupingPlugin when enabled takes over the header column drag and drop from the internal handlers. But it is not treeColumn aware.

2. The init script by default inits treeColumns and sets the columns based on treeColumns.

3. There is an internal header drag and drop capability enabled by the enableColumnReorder gird option (though it requires jquery.ui and the its Sortable as a dependency)

4. Normally, treeColumns would be reorder aware as it is part of the internal drag and drop.

Therefore:

By taking over header drag and drop from internal functionality and being treeColumns un-aware the DraggableColumns tech after a reorder de-synching the columns and treeColumns.

Secondary Considerations

1. we don’t know if anybody has figured out a way that Slick.TreeColumns can be useful in their project and whether there is a dependency.

2. Getting rid of it would probably not hurt anything in the core repo but it would probably take a reasonable effort to find all its tendrils and remove it.

3. Taking time doesn’t mean it should not be removed, or at least plugin/extenstion-ified it just means nobody is eager to do it right now.

4. It may be useful for grouping purposes if someone were to take the time to really grok it.

5. If it is to be kept ditching a conditional someone somehere thought was important is a bad idea.

Conclusion based on current factors:

DraggableGrouping Plugin at a minimum should be made treeColumns aware because whatever is using treeColumns gets out of synch after draggable grouping reorders stuff. If it updates.

The options would be:

A: merge functionality missing from slick.grid.js’s setupColumnReorder $headers.sortable implementation into the DraggableGroupingPlugin B: explicity call something like a new updateColumns(columns) function in slick.grid and then call it from DraggableGroupingPluin. That function would set tree columns and columns.

Then encourage people to start using updateColumns rather than setColumns.

For those who say setColumns with is creating a synching problem, point them to updateColumns.

If somebody complains that using updateColumns breaks their treeColumn implementation, we can tell them it is ok to continue using setColumns, and then ask them what they are doing with Slick.TreeColumns.

Sorry to blast you with so much text over such a small thing. I’m just trying to make sure whatever we do doesn’t mess people up and that we can have as much a grip on what is going on as possible.

From: Ghislain B. @.> Sent: Sunday, April 4, 2021 8:11 PM To: 6pac/SlickGrid @.> Cc: arthur-clifford @.>; Author @.> Subject: Re: [6pac/SlickGrid] freezing columns after reorder, depth check? (#592)

1. Is there a workflow you prefer for putting up more of a problem example rather than a product example? Is there an official 6pac/slickgrid JSFiddle or similar something somewhere I should use, or a folder you can exclude from official release commits?

There's no official fidle or anything, you just take an Example and modify it a bit.

2. Why does: a. examples/example-multi-grid-basic.html not allow dragging columns without any obvious options to enable it? b. but examples/example-frozen-columns-and-rows.html does allow draggable columns without a plugin and without any options turning it on?

Probably because of this line grid.setSelectionModel(new Slick.RowSelectionModel());, in Angular-Slickgrid I added a lot of grid options flags but in SlickGrid directly, you need to call and instantiate all the plugins you need while in Angular-Slickgrid it's with flags (which is purposely easier and it also helps to really instantiate only when flag are enabled)

I wonder if there's anything related to this Tree Example https://6pac.github.io/SlickGrid/examples/example5-collapsing.html in any ways.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/6pac/SlickGrid/issues/592#issuecomment-813157372 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6BJW7WSPJE77CJEOU7QM3THES5PANCNFSM42BADZ7Q . https://github.com/notifications/beacon/AE6BJW26S6WZONNALYOU6GLTHES5PA5CNFSM42BADZ72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGB34X7A.gif

[ghiscoding]

Your investigation seems quite thorough to cover as much use case as possible, that is great. So go ahead with a PR of what you think is best and that will probably be good enough for us. Are you expecting to do that soon? If not, I'll ask Ben for a new release soon since a new TypeScript change is on the way and we have things that weren't released yet.

[arthur-clifford]

Go ahead with a new release. I will likely be doing a new PR next week some time.

From: Ghislain B. @.> Sent: Tuesday, April 6, 2021 6:17 AM To: 6pac/SlickGrid @.> Cc: arthur-clifford @.>; Author @.> Subject: Re: [6pac/SlickGrid] freezing columns after reorder, depth check? (#592)

Your investigation seems quite thorough to cover as much use case as possible, that is great. So go ahead with a PR of what you think is best and that will probably be good enough for us. Are you expecting to do that soon? If not, I'll ask Ben for a new release soon since a new TypeScript change is on the way and we have things that weren't released yet.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/6pac/SlickGrid/issues/592#issuecomment-814111847 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6BJWY3VPVS5ZWI7CB5XSDTHMCVRANCNFSM42BADZ7Q . https://github.com/notifications/beacon/AE6BJW3FEGBQHYOCAVOJCQLTHMCVRA5CNFSM42BADZ72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGCDFYZY.gif

[ghiscoding]

@6pac could you cut a release with what we have before the TypeScript stuff gets merged. Actually wait, I just did another PR #599 that I'd like to be included as well.

Thanks mate

[ghiscoding]

@arthur-clifford oh gosh I can't believe, this just bite me today and I lost the entire day on it to find out that it comes from this same treeColumns.extractColumns(). In my use case, I change the freeze columns via setOptions({ frozenColumn: -1, frozenRow: -1, frozenBottom: false, enableMouseWheelScrollHandler: false }); and then I was calling setColumns(newColumns) (with less columns) and it was always returning the wrong columns set (it was what I had the columns that I before not the new columns), then I found out that I need to set that 3rd argument to True to get rid of this problem like so setOptions({ frozenColumn: -1, frozenRow: -1, frozenBottom: false, enableMouseWheelScrollHandler: false }, false, true);

So did you get any further with your investigation/code change?

[6pac]

Sorry, I feel like it would be good for me to show some leadership here and make an executive decision around removing treeColumns or making it good. However I'm neck deep in a messy rearchitecture of another major project, and staying interstate to boot. Not going to be able to do anything for about three months, I think.

My first impression is that treeColumns came in inadvertently from another fork, it may well have been experimental in nature anyway, and if there is no compelling use for it, then I think it would best be removed.

[ghiscoding]

no worries, and it did came from the X-slickgrid fork when I merged that fork with ours to add the frozen feature. I'm able to get it going after using those other flags in setOptions so I'm not necessary block per say, just that I lost a day on it lol

[aasdkl]

Hello @ghiscoding @6pac @arthur-clifford, I'd like to share a codepen about TreeCol:

https://codepen.io/aasdkl/pen/RwKeOWg

The TreeCol is used for column-group which implemented by the X-SlickGrid:

var oldColumns = [
  { name: "",     columns: [{id: "title", name: "old - Title", field: "title"}] }, 
  { name: "test", columns: [{id: "duration", name: "old - Duration", field: "duration"}, {id: "testTree", name: "old - testTree", field: "testTree"}] }
];

Seems we already have a column-group feature(Example), I think it would best to be removed.

[6pac]

thank you @aasdkl !!! let's get rid of treeColumns then. I think adding a nested data structure for that feature is way overkill and will only unnecessarily complicate things.

[xinchao-zhang]

I have discovered the same issue recently and @ghiscoding pointed me here. Technically I think all of us have done exactly the same change based on the same rationale - setColumns() should set the columns at least in the same way as that of init(). By looking at the code itself I think it is just a careless bug.

As far as I can tell, the effect of bug is that setColumns() is not usable, you just cannot change your column collections, and you cannot do anything that requires setColumns() to be called (to recreate the columns and hence fire the relevant events). Sometimes you can pass true to setOptions to suppress column set. But that really depends on what you are changing.

The extra functions (getColumnsInDepth) introduced by the TreeColumns are indeed only invoked by the column grouping feature (the method createColumnGroupHeaders()). But the column grouping example in the document does not advertise this out-of-box support for column grouping. Instead it leverages a plugin to draw the additional header.

If the decision is to remove the TreeColumns we can also remove the related column grouping methods.

[ghiscoding]

TreeColumns got removed in #775 and released under the new v4.0.0-beta.0, so it should be safe to close this issue