Bug in _fnOrderColumns() when no changes

[dswitzer]

There's a bug in _fnOrderColumns() that when there have been no changes, the invalidate() method is called which forces the table to redraw. Since no event is fired when this happens, the table can be left in an invalid state.

The problem is here in the code:

for ( var i=0, iLen=a.length ; i<iLen ; i++ )
{
	var currIndex = $.inArray( i, a );
	if ( i != currIndex )
	{
		/* Reorder our switching array */
		fnArraySwitch( a, currIndex, i );

		/* Do the column reorder in the table */
		this.s.dt.oInstance.fnColReorder( currIndex, i, true, false );

		changed = true;
	}
}

$.fn.dataTable.Api( this.s.dt ).rows().invalidate();

this._fnSetColumnIndexes();

// Has anything actually changed? If not, then nothing else to do
if ( ! changed ) {
	return;
}

I believe the ! changed line should be moved before the call to $.fn.dataTable.Api( this.s.dt ).rows().invalidate().

Once it's determined that the order hasn't changed, then nothing should be performed.

If the the call to invalidate() and _fnSetColumnIndexes() are needed, then I there needs to be an event that's fired to at least indicate the table has been invalidated and the table has been redrawn.

In my use case, I have some code that adds some DOM helpers that get added to the table. I hook into the column-reorder event to make sure the DOM is updated, but since this event doesn't get fired when there are no changes but the invalidate() redraws the table anyway, my table is left in an unexpected state.

[Deleted user]

Agreed! Thanks for posting this with your excellent description. I've committed the suggested changed.

[dswitzer]

I think this changed caused a regression. In some cases, I'm getting an exception in the _fnMouseDown() event because $(nTh).attr('data-column-index') does not exist.

I believe the problem is when stateSave is enabled and the columns are re-ordered on initialization. What's strange is not every state change causes the issue, but once it happens I can't resolve the issue until I clear the state.

This gets set in the _fnSetColumnIndexes() and I think there are cases where no changes are detected, but that function still needs to run.

I think the following line needs to be above the ! changed condition:

this._fnSetColumnIndexes();

[dswitzer]

I think I've found the source of the problem. In the _fnConstruct function there's the following code:

		/* If we have an order to apply - do so */
		if ( aiOrder )
		{
			/* We might be called during or after the DataTables initialisation. If before, then we need
			 * to wait until the draw is done, if after, then do what we need to do right away
			 */
			if ( !that.s.dt._bInitComplete )
			{
				var bDone = false;
				$(table).on( 'draw.dt.colReorder', function () {
					if ( !that.s.dt._bInitComplete && !bDone )
					{
						bDone = true;
						var resort = fnInvertKeyValues( aiOrder );
						that._fnOrderColumns.call( that, resort );
					}
				} );
			}
			else
			{
				var resort = fnInvertKeyValues( aiOrder );
				that._fnOrderColumns.call( that, resort );
			}
		}
		else {
			this._fnSetColumnIndexes();
		}

When it's failing, the !that.s.dt._bInitComplete conditional branch is running. It's expecting the draw.dt.colReorder event to fire off this._fnSetColumnIndexes(), but since there are no changes to the sort order, the code never runs.

I'm wondering if really the only logic that should be short circuited in the _fnOrderColumns function is the _fnSaveState, either that or during initialization it seems like we need to enforce that the _fnSetColumnIndexes() is called at least once.

[dswitzer]

I added the following line under the aiOrder check and it appears to resolve the issue:

this._fnSetColumnIndexes();

If that is the correct fix, the best option would be to just move it before the aiOrder check and then the conditional at the bottom will be dropped.

This does mean it's possible the attributes get added twice during initialization, in the case where a change is somehow detected.

[Deleted user]

Thanks! I've fixed that in bfde741. I decided to go with just calling the set column indexes function regardless of changes - it very low overhead compared to the invalidation, and the order between it and the invalidation doesn't matter.