select multi: $render is called twice when $viewValue reference changes

[tomyam1]

In the select directive there is a deep watch that calls $render if $viewValue deep-changes. This happens either if it changes inside or if it's reference changed.

It should only call $render in the first cases.

If the reference of $viewValue changes than this call is redundant because the shallow watch in ngModel will call $render.

I suggest something like:

    var lastView, lastViewRef = NaN;
    scope.$watch(fucntion() {
      if (lastViewRef === ngModelCtrl.$viewValue and !equals(lastView, ngModelCtrl.$viewValue)) {
        lastView = shallowCopy(modelCtrl.$viewValue);
        modelCtrl.$render();
      }
      lastViewRef = ngModelCtrl.$viewValue;
    })

Edit: Here is a plunker that show the issue
http://plnkr.co/edit/dWpTKH?p=preview

[petebacondarwin]

Sounds reasonable. @tepez would you like to try to create a Pull Request with this fix - including a unit test?

[tomyam1]

Sure @petebacondarwin.
A sufficient unit test would be that modelCtrl.$render is called once when either the model changes inside or when its reference change?

[petebacondarwin]

Sounds good

[Narretz]

Here's a test: @tepez Do you want to open a PR which includes this test or should I open one with your fix?

    it('should not call $render twice when the reference to the viewValue changes', function() {
      compile(
        '<select name="select" ng-model="selection" multiple>' +
          '<option>A</option>' +
          '<option>B</option>' +
        '</select>');

      var ngModelCtrl = element.controller('ngModel');
      spyOn(ngModelCtrl, '$render').andCallThrough();

      scope.$apply(function() {
        scope.selection = ['A'];
      });

      expect(element).toEqualSelect(['A'], 'B');
      expect(ngModelCtrl.$render).toHaveBeenCalled();
      expect(ngModelCtrl.$render.calls.length).toEqual(1);
    });

[tomyam1]

Thank you @Narretz I'd love if you could it.

My two cents on this:

• toHaveBeenCalled() is redundant since you check calls.length
• You could use toBe(1) instead of toEqual(1)
• I would not test toEqualSelect here since its already being tested in other tests.
• I would also test that $render is called once when viewValue deep-changes.

I guess these are mostly personal style preferences so feel free to ignore them.

describe('calls to $render', function() {

  var ngModelCtrl;

  beforeEach(function() {
    compile(
      '<select name="select" ng-model="selection" multiple>' +
      '<option>A</option>' +
      '<option>B</option>' +
      '</select>');

    ngModelCtrl = element.controller('ngModel');
    spyOn(ngModelCtrl, '$render').andCallThrough();
  });

  it('should call $render once when the reference to the viewValue changes', function() {
    scope.$apply(function() {
      scope.selection = ['A'];
    });
    expect(ngModelCtrl.$render.calls.length).toBe(1);

    scope.$apply(function() {
      scope.selection = ['A', 'B'];
    });
    expect(ngModelCtrl.$render.calls.length).toBe(2);

    scope.$apply(function() {
      scope.selection = [];
    });
    expect(ngModelCtrl.$render.calls.length).toBe(3);
  });

  it('should call $render once when the viewValue deep-changes', function() {
    scope.$apply(function() {
      scope.selection = ['A'];
    });
    expect(ngModelCtrl.$render.calls.length).toBe(1);

    scope.$apply(function() {
      scope.selection.push('B');
    });
    expect(ngModelCtrl.$render.calls.length).toBe(2);

    scope.$apply(function() {
      scope.selection.length = 0;
    });
    expect(ngModelCtrl.$render.calls.length).toBe(3);
  });

});