Make sure 'enumerate' changes when the underlying collection changes.

Make sure 'enumerate' changes when the underlying collection changes.

R=jmesserly@google.com

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

[Siggi Cherem (dart-lang), 2013-11-13 23:09:25]

Not sure if this is what you had in mind in order to fix 'enumerate', but here
is one attempt.

[Jennifer Messerly, 2013-11-13 23:51:45]

I'm a bit surprised by this fix ... not that smart proxying of list changes is
bad ... BUT there must be another bug somewhere inside PolymerExpressions.
Because the original enumerate code should have "just worked"

https://codereview.chromium.org/71353012/diff/1/pkg/polymer_expressions/lib/s...
File pkg/polymer_expressions/lib/src/globals.dart (right):

https://codereview.chromium.org/71353012/diff/1/pkg/polymer_expressions/lib/s...
pkg/polymer_expressions/lib/src/globals.dart:10: // Code from
https://github.com/google/quiver-dart/commit/52edc4baf37e99ff6a8f99c648b29b13...
this comment seems wrong now :)

https://codereview.chromium.org/71353012/diff/1/pkg/polymer_expressions/lib/s...
pkg/polymer_expressions/lib/src/globals.dart:22: var result = iterable.map((e)
=> new IndexedValue(i++, e));
I don't think this works. It looks awfully like a bug I fixed :)

here's the issue: if you iterate over the same iterable twice, the "i" doesn't
reset, so it keeps counting up. That's the reason why I wrote EnumerateIterable
by hand ... not just for the fun of it ;)

https://codereview.chromium.org/71353012/diff/1/pkg/polymer_expressions/lib/s...
pkg/polymer_expressions/lib/src/globals.dart:31: class IndexedValue<V> {
should mixin Observable or probably ChangeNotifier

since this object is so simple, ChangeNotifier would be good, that way you can
make "index" setter private, e.g.

int _index

get index => _index;
_setIndex(i) {
  _index = notifyPropertyChange(#index, _index, i);
}

https://codereview.chromium.org/71353012/diff/1/pkg/polymer_expressions/lib/s...
pkg/polymer_expressions/lib/src/globals.dart:32: @observable int index;
make this immutable? so people don't accidentally two-way bind it

https://codereview.chromium.org/71353012/diff/1/pkg/polymer_expressions/lib/s...
pkg/polymer_expressions/lib/src/globals.dart:42: // TODO(sigmund): maybe we can
expose ObservableList._hasListObservers so we
problem is, you'd need the pair of observed/unobserved events on the object
you're returning, so you can register/unregister on the source observable list

[Jennifer Messerly, 2013-11-13 23:53:42]

https://codereview.chromium.org/71353012/diff/1/pkg/polymer_expressions/lib/s...
File pkg/polymer_expressions/lib/src/globals.dart (right):

https://codereview.chromium.org/71353012/diff/1/pkg/polymer_expressions/lib/s...
pkg/polymer_expressions/lib/src/globals.dart:22: var result = iterable.map((e)
=> new IndexedValue(i++, e));
On 2013/11/13 23:51:45, John Messerly wrote:
> I don't think this works. It looks awfully like a bug I fixed :)
> 
> here's the issue: if you iterate over the same iterable twice, the "i" doesn't
> reset, so it keeps counting up. That's the reason why I wrote
EnumerateIterable
> by hand ... not just for the fun of it ;)
> 

original context:
https://github.com/google/quiver-dart/pull/39

[Siggi Cherem (dart-lang), 2013-11-14 02:06:21]

PTAL

John - I see now what you meant. I rewrote the CL entirely, but I'll be
interested on your thoughts here.

The issue with "expr | enumerate" is that we only trigger 'enumerate' when
changes are notified in 'changes' not in 'listChanges'.

We already have a special case for the 'in' operator using listChanges, I added
a special case for '|'. I don't like patching the code in Binary as it is right
now, I wonder if we should instead have an AST node to represent filters
instead. Maybe this is part of the optimizations that we can discuss tomorrow.

some questions/ideas:
(a) Are there any other expression that cares about ObservableList?
(b) Instead of this change, we can say that 'filter' only listens for 'changes',
but not 'listChanges', so it's in the user hands to write a smart filters for
listChanges.
(c) Another idea, just like we have Filter and Transformer, we could add a
SmartFilter. If you have a SmartFilter, we don't listen on any listChanges (or
any changes altogether) and just let the SmartFilter handle it.

Thoughts?

[Jennifer Messerly, 2013-11-14 02:50:49]

lgtm!

https://codereview.chromium.org/71353012/diff/70001/pkg/polymer_expressions/l...
File pkg/polymer_expressions/lib/src/globals.dart (right):

https://codereview.chromium.org/71353012/diff/70001/pkg/polymer_expressions/l...
pkg/polymer_expressions/lib/src/globals.dart:27: operator==(o) => o is
IndexedValue && o.index == index && o.value == value;
wonder if we should upstream this (if you haven't already :) )

[Siggi Cherem (dart-lang), 2013-11-14 17:52:54]

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

[Siggi Cherem (dart-lang), 2013-11-14 17:56:11]

https://codereview.chromium.org/71353012/diff/70001/pkg/polymer_expressions/l...
File pkg/polymer_expressions/lib/src/globals.dart (right):

https://codereview.chromium.org/71353012/diff/70001/pkg/polymer_expressions/l...
pkg/polymer_expressions/lib/src/globals.dart:27: operator==(o) => o is
IndexedValue && o.index == index && o.value == value;
On 2013/11/14 02:50:49, John Messerly wrote:
> wonder if we should upstream this (if you haven't already :) )

good idea. Will do