Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

ng-repeat fails after reordering collection #1154

Closed
michalpie opened this issue Jun 17, 2014 · 14 comments
Closed

ng-repeat fails after reordering collection #1154

michalpie opened this issue Jun 17, 2014 · 14 comments

Comments

@michalpie
Copy link

We found a bug when using ng-repeat over a collection of objects. If the collection is reordered several times, ng-repeat looses or duplicates some elements, also the $index property is not showing consecutive numbers. Tested with AngularDart 0.12.0 from pub and the newest version from git master. The bug present in Dartium and in compiled version.

Example program:

index.html

<!DOCTYPE html>
<html>
<head>
    <script type="text/javascript" src="packages/web_components/platform.concat.js?"></script>
    <script type="text/javascript" src="packages/web_components/dart_support.js"></script>
    <script type="application/dart" src="bugs.dart"></script>
    <script type="text/javascript" src="packages/browser/dart.js"></script>
</head>
<body>
  <my-component></my-component>
</body>
</html>

bugs.dart

import 'dart:html';
import 'package:angular/angular.dart';
import 'package:angular/application_factory.dart';
import 'dart:async';

class Item {
  String value;
  Item(this.value);
  toString() => value;
}

@Component(
    template: r'''
<button ng-click="ctrl.addItem()">add</button>
<button ng-click="ctrl.sortAsc()">sort asc</button>
<button ng-click="ctrl.sortDesc()">sort desc</button>
<button ng-click="ctrl.random()">random</button>
{{ctrl.items}}
<ul>
  <li ng-repeat="item in ctrl.items track by item.value">index : {{$index}}  value : {{item.value}}  </li>
</ul>
''',
    publishAs: "ctrl",
    selector: "my-component"
)
class MyComponent {
  int n = 0;
  List<Item> items = [];

  MyComponent() {
    for (int i = 0; i < 14; i++ ) {
      items.add(new Item("$n"));
      n++;
    }
  }

  void addItem() {
    items.add(new Item("$n"));
    n++;
  }
  random() {
    var temp = new List.from(items);
    temp.shuffle();
    items.clear();
    items.addAll(temp);
  }
  sortAsc() {
    items.sort((a,b)=>b.value.compareTo(a.value));
  }
  sortDesc() {
    items.sort((a,b)=>a.value.compareTo(b.value));
  }
}

void main() {
  applicationFactory()..addModule(new Module()..type(MyComponent)).run();
}

After running the program, initially we have the correct output:

 [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13]
index : 0 value : 0
index : 1 value : 1
index : 2 value : 2
index : 3 value : 3
index : 4 value : 4
index : 5 value : 5
index : 6 value : 6
index : 7 value : 7
index : 8 value : 8
index : 9 value : 9
index : 10 value : 10
index : 11 value : 11
index : 12 value : 12
index : 13 value : 13

After several clicks on the "random" button, the collection is reordered, however ng-repeat shows different order of elements and invalid values of $index

 [3, 5, 9, 2, 11, 1, 13, 10, 4, 8, 6, 7, 0, 12]
index : 0 value : 3
index : 1 value : 5
index : 2 value : 9
index : 4 value : 11
index : 5 value : 1
index : 6 value : 13
index : 7 value : 10
index : 8 value : 4
index : 9 value : 8
index : 10 value : 6
index : 11 value : 7
index : 3 value : 2
index : 12 value : 0
index : 13 value : 12

Adding or removing 'track by' has no effect.
@vicb
Copy link
Contributor

vicb commented Jun 17, 2014

I would say this might be linked to #1097 but not if this is reproducible on master

@vicb
Copy link
Contributor

vicb commented Jun 17, 2014

Thanks for the clean repro, however I can not reproduce this on master. What are the steps to get the bug on master w/ Dart ?

@michalpie
Copy link
Author

I was just clicking on the "random" button several times. It happens once in 3-5 cases. On version from pub it sometimes loses some rows and leads to complete disaster. On version from master it is much better, only order is sometimes incorrect.

@vicb
Copy link
Contributor

vicb commented Jun 17, 2014

The linked issue explain the pb on the pub version. I'll take a look at the master issue (I was not paying attention to the indexes but I can reproduce it now)

@lavep
Copy link

lavep commented Jun 17, 2014

I've checked it and on compiled version is even worse. After indexes are in wrong order next shufle makes some rows disappear. Also:
1 sort
2 add
3 sort desc
4 sort asc
has the same problems.

@vicb
Copy link
Contributor

vicb commented Jun 17, 2014

@lavep pub version ? if yes this is expected as stated above.

@michalpie I have now a simplified version of your code and a good idea of what could go wrong. I hope I can come with a fix quickly.

@michalpie
Copy link
Author

Great! Thank you, Victor.

@lavep
Copy link

lavep commented Jun 17, 2014

@vicb yes I've tried it on all versions from 0.10 to git

@vicb
Copy link
Contributor

vicb commented Jun 17, 2014

I have the fix, I need to write a test and I'll push the PR

vicb added a commit to vicb/angular.dart that referenced this issue Jun 17, 2014
@vicb
fix(ng_repeat): fix ng_repeat not moving views for elements that have…
40302fb 
… not moved

Closes dart-archive#1154
@vicb
Copy link
Contributor

vicb commented Jun 17, 2014

Could you please test the PR and report your results. Thanks.

vicb added a commit to vicb/angular.dart that referenced this issue Jun 17, 2014
@vicb
fix(ng_repeat): fix ng_repeat not moving views for elements that have…
ee7934e 
… not moved

Closes dart-archive#1154
@vicb vicb mentioned this issue Jun 17, 2014
Closed
@michalpie
Copy link
Author

It works fine on Dartium. @lavep has tested the compiled version and it works fine as well. Victor, thank you very much for this quick fix, you were unbelievably fast!

@vicb
Copy link
Contributor

vicb commented Jun 17, 2014

no pb. Your code really helped me.

@michalpie
Copy link
Author

Hi Victor,
We must deploy our application to production very soon, but when we work with version 0.12 with your pull request, we still sometimes see the problem. Should we include any other pull requests to fix the problem completely?
Thanks in advance.

@vicb
Copy link
Contributor

vicb commented Jul 4, 2014

0.13 will focus on perf. The best would be to wait until 0.14 which will include several bug fixes. I hope it can be released in about 1 month

vicb added a commit that referenced this issue Jul 23, 2014
@vicb @chirayuk
fix(ng_repeat): fix ng_repeat not moving views for elements that have…
2466652 
… not moved

Closes #1154

Closes #1155
vicb added a commit that referenced this issue Jul 23, 2014
@vicb @chirayuk
fix(ng_repeat): fix ng_repeat not moving views for elements that have…
4738b48 
… not moved

Closes #1154

Closes #1155
vicb added a commit that referenced this issue Jul 23, 2014
@vicb @chirayuk
fix(ng_repeat): fix ng_repeat not moving views for elements that have…
d06f590 
… not moved

Closes #1154

Closes #1155
vicb added a commit that referenced this issue Jul 23, 2014
@vicb @chirayuk
fix(ng_repeat): fix ng_repeat not moving views for elements that have…
8367998 
… not moved

Closes #1154

Closes #1155
@vicb vicb closed this as completed in 559a685 Jul 24, 2014
vicb added a commit that referenced this issue Jul 24, 2014
@vicb @chirayuk
fix(ng_repeat): fix ng_repeat not moving views for elements that have…
e058e51 
… not moved

Closes #1154

Closes #1155
vicb added a commit that referenced this issue Jul 24, 2014
@vicb @chirayuk
fix(ng_repeat): fix ng_repeat not moving views for elements that have…
20e5c18 
… not moved

Closes #1154

Closes #1155
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(ng_repeat): fix ng_repeat not moving views for elements that have no... vicb/angular.dart
3 participants
@vicb @michalpie @lavep