FixedColumns: scroller DOM setup hardcoded classnames

dbr

The scroller DOM are hardcoded class names, they should be dynamic.
If for some reason the class names are modified in the DataTable (via oClasses) the fixedColumns fail to select the correct dom elements to work correctly.

@file FixedColumns.js @version 2.0.3 [line: 389]
[code]
/* Set up the DOM as we need it and cache nodes */
this.dom.grid.dt = $(this.s.dt.nTable).parents('div.dataTables_scroll')[0];
this.dom.scroller = $('div.dataTables_scrollBody', this.dom.grid.dt )[0];
[/code]


A fix I guess would be:
[code]
/* Set up the DOM as we need it and cache nodes */
this.dom.grid.dt = $(this.s.dt.nTable).parents('div.'+this.s.dt.oClasses.sScrollWrapper)[0];
this.dom.scroller = $('div.'+this.s.dt.oClasses.sScrollBody, this.dom.grid.dt )[0];
[/code]

allan

Thanks for pointing this out! Completely agree.

I've opened an issue on this here: https://github.com/DataTables/Scroller/issues/12 . I've got a bit of work to do on Scroller tomorrow actually, so I'll take a look at this then.

Allan

dbr

I am actually taking a look at the FixedColumns in the master git (2.5.0dev) at the moment. I have written an extra (i will release it later) which allows for different preset column views on the same table to be toggled via a toggle bar of buttons. The FixedColumns extra does not play nicely with fnSetColumnVis, it should!

Also the cloning on the FixedColumns doesnt seem to be deep enough? I have a callback on fnCreatedRow, that adds some extra script functionality on some cells. This functionality is being lost when FixedColumns clones the table parts. I will post my findings as soon as I have something to show.

allan

> The FixedColumns extra does not play nicely with fnSetColumnVis, it should!

Ideally yes - however, it adds a reasonable amount of complexity which is why it currently does not. This is a feature that I would like to add in future when I have the time available to do so.

> Also the cloning on the FixedColumns doesnt seem to be deep enough?

It does an HTML clone with jQuery events and data (using $().clone). Is the functionality being added after the clone is done?

Allan

dbr

No I don't think so. Because I add a class name to the cell and a object in the cell, These class names appear in the clone, so the HTML is being copied, but I also add a click event to an object in the cell at the same time. It is this click event which is being lost. So the HTML is being copied but not any JS events bound to the objects in the HTML.

Just a quick look: it does seem to be about deep cloning (http://api.jquery.com/clone/ ), it seems you use .clone(true) for copying data and events but it should be .clone(true,true) to capture anything put inside the cells as well. I haven't tried it yet to see if this is the issue.. Lunch! ;-)

dbr

Just a note about visibility of columns, it appears to me that in the settings of DataTable the dt.s.aoColumns[i].bVisible property of a column is set false even when the property has not been set by any external configuration. It would make sense to me that this property is used to track the visibility state of a column. So by default it should be set true (because by default all columns show) unless told otherwise.

Throughout all the scripts I have looked at, I have notice dt.s.aoColumns is used to track the current state of the columns. So to me it seems important that if this variable is key for keeping track of the state of the rendered table, that it should be kept up-to-date.

I understand the complexity is a bit boggling (DataTables is quite amazing), but I am sure it can be tightly controlled. I need this to work, so I will have a peck at it.

allan

I didn't know about `.clone(true,true)` I must say. I thought it would just do that automatically. Yes it does look like that should be used.

Yes the bVisible variable of the column configuration object is used to track what the column's visibility state is and that is always correct (it is a read only semi-private variable). So yes, FixedColumns would need to be updated to take account of that variable. Thinking about it, with my recent changes in FixedColumns 2.5, it probably won't actually be too difficult to support.

Allan

dbr

I have kind of done it. I will try and provide a demo later today. I am stuck with some client work at the moment.

But the general gist is that when you clone the columns you have take in account the visibility state. So if you set 4 right columns to be fixed but only 2 of them are visible, you just clone the ones that are visible.

So in the looping of "_fnCloneRight","_fnCloneLeft" you need to skip the invisible columns.

(FixedColumns 2.5)
[code]
for ( i=this.s.iTableColumns-this.s.iRightColumns ; i

dbr

Just another point on the same line of investigation (bear with me):
When you are changing the visibility of columns and you have a scroller involved and the xScrollInner is not set, the width of the table needs to be removed.

Currently jquery.dataTables.js 1.10-dev (line:3537)
[code]
// When all else fails
tableStyle.width = _fnStringToCss( sanityWidth );
[/code]

There is an assumption made here "When all else fails" and "Not possible to take account of it" that the current width of the table is what it currently is. When column visibility is at play this may not be so because the width of the entire table may be changing due to columns being added or removed.

It may be safer to unset the width at this point and let the table size itself, rather than assume the current width was the intended width.

[code]
// When all else fails
tableStyle.width = "";
[/code]

Or maybe we need a case check of the visibilty state of the columns first and set the table width and accordingly switch between unsetting the width or use the sanityWidth. Or maybe we need a variable to track the change in column removal/addition.

I know that this maybe a bit complex of a situation to imagine, but I am looking quite deep at this point at this particular scenario.

BTW this is the case only when xScrollInner is not set!! When xScrollInner is set you are doing the right thing and observing the setting.

It is funny when I read this code and the names of the variables "iSanityWidth" and comments "When all else fails" it seems like you were trying to maintain your sanity to get through it. Actually it is great that you comment like this because it gives the reader the sense of what you were going through when writing it, and the possiblity to question the sanity! It is not easy to get your head around some of this stuff. lol :-)

dbr

Looking at my cloning issue it seems I am getting duplicated attributes Id's, so it is also my problem.

BTW there is an inherent conceptual issue with FixedColumns in general that I have noticed because of this.

Fixed columns are applied after the datatable is drawn and not at the same time.

So on that first render if you are using a fnCreatedRow callback you have no idea whether or not fixed columns are intended to be applied or not because it hasn't happend yet. Ultimately the ideal situation is that FixedColumns should be rendered simultaneously with the DataTable and not generated as a patch after the DataTable has been drawn.

I also think doing so would also be *MUCH* more efficient, because you would be creating the fixed columns in the same iteration as creating the table. At the moment you are iterating over the dataset again; cloning rather than directly "re-using" elements at creation time and recalculating widths of known dimensions creating unnecessary extra effort for the browser.

To fix this however would be quite an undertaking as you would need some low level hooks into the datatable draw process, and a complete rewrite of FixedColumns... version 3? The nice thing about doing this is you wouldn't need to call "new FixedColumns(table,config)" after initialising the datatable, it would be nicely integrated together with the configuration of the FixedColumns applied with the configuration of the DataTable. However it would be bound to a particular version of DataTables that has the required hooks, so no backwards compatibility.

allan

Hi,

Just a quick note to say I haven't forgotten about this! I will be coming back to it - just a little overwhelmed with everything else to do at the moment :-)

Allan

allan

Awesome stuff :-).

I've just committed this change into FixedColumns to support hidden columns: https://github.com/DataTables/FixedColumns/commit/d29e19e . I've also added a new example showing that ability to update dynamically with ColVis. I'll be releasing this tomorrow as FixedColumns 2.5.0 :-).

Regarding the deep integration - I think you are right - there are performance enhancements to be had integrating deeply into DataTables. Certainly something to look at in the future!

Regards,
Allan