Moved columns reset when new column added

[AgDude]

If columns are reordered (moved) by the user, then a new column is added to the grid, all columns return to their original position when notifyDataChange is called.

http://plnkr.co/edit/k27uZeIOr8NEmuq66KWV?p=preview

[ingshtrom]

I noticed that this is verify similar to #4188. It looks like no matter what, we are overriding our current grid layout/config with our column defs... which seems silly to me. Does anyone know why we would want to do that?

[edit]: My issue is actually fixed by #4005, but this issue is not. Sorry for the spam.

[AgDude]

@ingshtrom, you are correct that does appear to be the same issue. The design intent behind the current system is that gridOptions, and thus columnDefs are defined by the application developer, and should not be modified by user interaction (unless the application developer explicitly allows it). I am very hesitant to change this.

I haven't looked in detail at options for dealing with this issue, but my first thought is that features which override columnDefs should use the saveState api to save and restore themselves. My guess is that there is some modification needed in saveState to effectively support this, and probably also in the notifyDataChange callbacks to look in saveState.

In this particular case, as well as in the issue with column width, the current state is saved on grid.columns, which could be referenced by notifyDataChange callback. However, without an api for other features to leverage this, it really smells of a feature bleeding into the core.

saveState is currently simple and works very well for it original intended use, but I think a refactor to become more of a pubsub type system would be good. That way it doesn't need to know about all of other features, those features can register themselves and listen for save and restore events.

[swalters]

This may have been due to #3897. I know that PR affected caused the last big bug that we had with column's resizing whenever data was refreshed. #4005

I haven't had time to really investigate it, but it seems we are in a bug/fix/bug loop because the actual design is not known.

[AgDude]

This seems to pre-date that PR. I can reproduce it on RC19 and RC22.

[ChrisWiGit]

I also encountered this problem in a similar way because i use an asyn call to create the columns (returned by the server). See my plunk
http://plnkr.co/edit/p7KyfF?p=preview

1. change the column position
2. Restore the columns to the value given in state (gender will be first instead of name)
   The grid may flicker, and shortly you can see the columns were moved but then changed back to its original value as given in $scope.gridoptions.columnDefs.

However, if you do a "restore" in a $timeout, as shown with button "Restore ok", it works.

The reason behind this:

1. gridOptions.columnDefs is watched by the function columnDefsWatchFunction (a $watchCollection(columnDefs || columnDefs.length) is set in place) :
   function columnDefsWatchFunction(n, o) { if (n && n !== o) { self.grid.options.columnDefs = $scope.uiGrid.columnDefs; self.grid.buildColumns({ orderByColumnDefs: true }) .then(function(){
   So, we understand, why the second restore button works (since restore is called after this watcher function).
2. columnDefsWatchFunction in turn calls buildColumns() with parameter { orderByColumnDefs: true } which resets the column position depending on the gridOptions.columnDefs. However, columnDefs never changes or gets touched by the restore function. So the column positions are lost, since the positions are determined by the position in the array. All other properties come from the column objects itself and are not touched (hence pinning, sorting and such work)

The consequence is to use a $timeout after changing columnDefs to restore the columns.
I'm not sure why orderByColumnDefs must be true here.