Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

freezing columns after reorder, depth check? #592

Closed
arthur-clifford opened this issue Mar 30, 2021 · 20 comments
Closed

freezing columns after reorder, depth check? #592

arthur-clifford opened this issue Mar 30, 2021 · 20 comments

Comments

@arthur-clifford
Copy link
Contributor

arthur-clifford commented Mar 30, 2021 •
edited
Loading

@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
image

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

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

Result of anchor:
image
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
Copy link
Collaborator

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
Copy link
Contributor Author

arthur-clifford commented Mar 30, 2021 via email •
edited by ghiscoding
Loading

@ghiscoding
Copy link
Collaborator

ghiscoding commented Mar 30, 2021 •
edited
Loading

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
Copy link
Owner

6pac commented Mar 30, 2021

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

arthur-clifford commented Apr 5, 2021 via email

@ghiscoding
Copy link
Collaborator

  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.

  1. 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
Copy link
Contributor Author

arthur-clifford commented Apr 5, 2021 via email

@arthur-clifford
Copy link
Contributor Author

arthur-clifford commented Apr 5, 2021 via email

@ghiscoding
Copy link
Collaborator

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
Copy link
Contributor Author

arthur-clifford commented Apr 8, 2021 via email

@ghiscoding
Copy link
Collaborator

ghiscoding commented Apr 8, 2021 •
edited
Loading

@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
Copy link
Collaborator

ghiscoding commented Apr 20, 2021 •
edited
Loading

@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
Copy link
Owner

6pac commented Apr 20, 2021

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
Copy link
Collaborator

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
Copy link
Contributor

aasdkl commented Apr 21, 2021 •
edited
Loading

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
Copy link
Owner

6pac commented Apr 21, 2021

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.

@ghiscoding ghiscoding mentioned this issue Jan 28, 2022
Closed
@xinchao-zhang
Copy link
Contributor

xinchao-zhang commented Jan 28, 2022 •
edited
Loading

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 ghiscoding mentioned this issue Feb 1, 2023
Closed
@ghiscoding
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@6pac @ghiscoding @aasdkl @arthur-clifford @xinchao-zhang