introduce ObservableList.listChanges to keep list changes separate from property changes

introduce ObservableList.listChanges to keep list changes separate from property changes
this is part of addressing: 13554, 14157, 13359

R=sigmund@google.com

Committed: https://code.google.com/p/dart/source/detail?r=29630

[Jennifer Messerly, 2013-10-30 23:45:26]

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
File pkg/observe/lib/src/list_diff.dart (right):

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
pkg/observe/lib/src/list_diff.dart:5: library observe.src.list_diff;
hmmm; git totally failed to recognize the move. I'll comment on unchanged
methods

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
pkg/observe/lib/src/list_diff.dart:16: class ListChangeRecord {
some changes here to this class:

* merged ListChangeRecord/ListChangeDelta
* .removed returns unmodifiable list.
* added "object" property

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
pkg/observe/lib/src/list_diff.dart:80: List<List<int>> _calcEditDistances(List
current, int currentStart,
unchanged

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
pkg/observe/lib/src/list_diff.dart:121: List<int>
_spliceOperationsFromEditDistances(List<List<int>> distances) {
unchanged

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
pkg/observe/lib/src/list_diff.dart:195: List<ListChangeRecord> calcSplices(List
current, int currentStart,
unchanged

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
pkg/observe/lib/src/list_diff.dart:277: int _intersect(int start1, int end1, int
start2, int end2) {
newly ported from observe-js

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
pkg/observe/lib/src/list_diff.dart:295: void _mergeSplice(List<ListChangeRecord>
splices, ListChangeRecord record) {
newly ported from observe-js

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
pkg/observe/lib/src/list_diff.dart:367: List<ListChangeRecord>
_createInitialSplices(List<Object> list,
newly ported from observe-js

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
pkg/observe/lib/src/list_diff.dart:393: List<ListChangeRecord>
projectListSplices(List list,
newly ported from observe-js

https://codereview.chromium.org/53743002/diff/1/pkg/observe/test/list_change_...
File pkg/observe/test/list_change_test.dart (left):

https://codereview.chromium.org/53743002/diff/1/pkg/observe/test/list_change_...
pkg/observe/test/list_change_test.dart:267: // the array. Thus, we get 14
instead of 8.
fyi -- I previously got these TODO comments wrong. This was 7, not 8.

[Siggi Cherem (dart-lang), 2013-10-31 00:38:45]

lgtm, just some minor comments and a question about .changes vs .listChanges.

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
File pkg/observe/lib/src/list_diff.dart (right):

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
pkg/observe/lib/src/list_diff.dart:292: }
maybe not worth it, but this whole function could be written as:

  var overlap = min(end1, end2) - max(start1, start2);
  return overlap < 0 ? -1 : overlap;

actually, it seems you don't care about -1 below, just whether or not it's
negative. so maybe just:

  return min(end1, end2) - max(start1, start2);

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
pkg/observe/lib/src/list_diff.dart:306: if (inserted) continue;
It took me a while to get why this code works. This almost looked like if you
are breaking out of the loop because inserted is never set back to false.
Reading this more closely, I think the way this works is:
 - the loop finds where the merge should happen
 - it applies the merge in a particular splice
 - then continues and updates the subsequent splices with any offset diff.

Is that right? Maybe we should add some comments about it?

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/observabl...
File pkg/observe/lib/src/observable_list.dart (right):

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/observabl...
pkg/observe/lib/src/observable_list.dart:62: *     #<ListChangeRecord index: 3,
removed: [b], addedCount: 0>
I'm not sure I follow this summary.

The operations above seem to change things as follows:

['a', 'b']
==>
['b']
==>
['c', 'd', 'e', 'b']
==>
['c', 'b']
=>
['c', 'f', 'b']

So, I was expecting:

#<ListChangeRecord index: 0, removed: [], addedCount: 2>
#<ListChangeRecord index: 2, removed: [a], addedCount: 0>

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/observabl...
pkg/observe/lib/src/observable_list.dart:101: var oldValue = _list[index];
So what happens if I use .changes instead of .listChanges? I was expecting to
get raw change events (like you had by calling notifyPropertyChange in
set-length).

[Jennifer Messerly, 2013-10-31 02:19:34]

Thanks!

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
File pkg/observe/lib/src/list_diff.dart (right):

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
pkg/observe/lib/src/list_diff.dart:292: }
On 2013/10/31 00:38:45, Siggi Cherem (dart-lang) wrote:
> maybe not worth it, but this whole function could be written as:
> 
>   var overlap = min(end1, end2) - max(start1, start2);
>   return overlap < 0 ? -1 : overlap;
> 
> actually, it seems you don't care about -1 below, just whether or not it's
> negative. so maybe just:
> 
>   return min(end1, end2) - max(start1, start2);

nice. done. hmm. maybe I should send them a pull request :)

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/list_diff...
pkg/observe/lib/src/list_diff.dart:306: if (inserted) continue;
On 2013/10/31 00:38:45, Siggi Cherem (dart-lang) wrote:
> It took me a while to get why this code works. This almost looked like if you
> are breaking out of the loop because inserted is never set back to false.

hey, not my code ;-)

> Reading this more closely, I think the way this works is:
>  - the loop finds where the merge should happen
>  - it applies the merge in a particular splice
>  - then continues and updates the subsequent splices with any offset diff.
> 
> Is that right? Maybe we should add some comments about it?

uh, sure. :)

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/observabl...
File pkg/observe/lib/src/observable_list.dart (right):

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/observabl...
pkg/observe/lib/src/observable_list.dart:62: *     #<ListChangeRecord index: 3,
removed: [b], addedCount: 0>
On 2013/10/31 00:38:45, Siggi Cherem (dart-lang) wrote:
> I'm not sure I follow this summary.
> 
> The operations above seem to change things as follows:
> 
> ['a', 'b']
> ==>
> ['b']

it's removeAt(1) ... so it removes 'b' not 'a' :)

https://codereview.chromium.org/53743002/diff/1/pkg/observe/lib/src/observabl...
pkg/observe/lib/src/observable_list.dart:101: var oldValue = _list[index];
On 2013/10/31 00:38:45, Siggi Cherem (dart-lang) wrote:
> So what happens if I use .changes instead of .listChanges? I was expecting to
> get raw change events (like you had by calling notifyPropertyChange in
> set-length).

.changes only gives property changes (including "length"). Essentially list
changes and property changes are distinct APIs.

[Jennifer Messerly, 2013-10-31 02:32:15]

Committed patchset #2 manually as r29630 (presubmit successful).