Implement array-selector. Fixes #4154 #4230

Merged
merged 11 commits into from Jan 9, 2017

Projects

None yet

4 participants

@kevinpschaaf
Contributor

Implements <array-selector> for 2.x.

  • Updated to ES6 Polymer.Element class syntax
  • Removes use of Polymer.Collection; instead uses splice information to adjust linked indices between the items & selected arrays
  • When the object is reset, current selection is persisted based on object identity by synthesizing splice info via Polymer.ArraySplice (Levenshtein minimum edit distance algorithm); this makes array-selector compatible with the proposed changes to object dirty checking in 2.0. However, this is a small break in API behavior (previously setting to a new array would clear selection; it no longer necessarily does).
  • Small API improvement: when items are removed that were previously selected, the selected item is removed from the selected array/item (in 1.x you had to manually deselect first, which seemed onerous)

Reference Issue

Fixes #4154

@googlebot googlebot added the cla: yes label Dec 21, 2016
src/templatizer/array-selector.html
+ * @param {*} item Item from `items` array to select
+ */
+ select(item) {
+ let idx = this.items.indexOf(item);
@blasten
blasten Dec 21, 2016 Member

this used to be O(1) in v1, but now it's O(n), I just hit this problem in iron-list...

@kevinpschaaf
kevinpschaaf Jan 3, 2017 Contributor

The removal of Collection ultimately drives this change since collection.getKey(item) was O(1). One option would be to introduce a selectIndex(idx) and deselectIndex(idx) API which would avoid the item->index lookup when the index is known apriori (e.g. selection following tap on a dom-repeat'ed item).

When you say ' I just hit this problem in iron-list...', what do you mean? You hit a real-world performance problem due to this particular implementation? Or a similar O(n) problem elsewhere?

@blasten
blasten Jan 3, 2017 edited Member

yea. I was mostly thinking about multi-selection using the shift key or any other pattern that triggers this path to select an item. In iron-list, I'll also need to call this.items.indexOf(item) to preserve item.selected in the model.

selectIndex(idx) 👌

src/templatizer/array-selector.html
+
+ _updateSelection(multi, itemsInfo) {
+ if (itemsInfo.path == 'items') {
+ let newItems = itemsInfo.base;
@blasten
blasten Dec 21, 2016 Member

This observer might run even when newsItems is undefined causing a:
Uncaught TypeError: Cannot read property 'length' of undefined
from calculateSplices (array-splice.html:251)

@kevinpschaaf
kevinpschaaf Jan 3, 2017 Contributor

Thanks! Fixed.

src/templatizer/array-selector.html
+ let splices = Polymer.ArraySplice.calculateSplices(newItems, lastItems);
+ this._applySplices(splices, lastItems);
+ } else {
+ this.clearSelection();
@blasten
blasten Dec 21, 2016 Member

clearSelection also initializes this.selected but not always. If lastItems is an array and I set multi=true after, then selected is undefined.

@kevinpschaaf
kevinpschaaf Jan 3, 2017 Contributor

I didn't totally grok how you could have a lastItems and selected be undefined, but I think I see a case where you could switch multi and not have selected reflect a clear selection (i.e., in single selection it should be null and in multi selection it should be []), so I updated so that clearSelection is always run when multi changes.

src/templatizer/array-selector.html
@@ -158,58 +187,48 @@
}
_applySplices(splices) {
+ let toRemove = [];
@sorvell
sorvell Jan 7, 2017 Member

Need a comment here to explain a fundamental limitation of this approach: if an array is re-ordered the same element may be removed and re-added. In this case, it may not be detected as still being in the selection and therefore the selection will not be maintained in that case.

src/templatizer/array-selector.html
- this.linkPaths('selected.' + sidx, 'items.' + (idx + added - removed));
+ let selected = new IntegerSet();
+ let sidx = 0;
+ this._selectedSet.forEach(idx => {
@sorvell
sorvell Jan 7, 2017 Member

Apparently, the new set is created here because adding to a set while inside forEach is not supported. If that's the confirmed, let's add a comment.

@sorvell
sorvell Jan 7, 2017 Member

Can't seem to confirm that root issue so this needs explanation.

@sorvell
sorvell approved these changes Jan 9, 2017 View changes

LGTM

@sorvell sorvell merged commit d18764c into 2.0-preview Jan 9, 2017

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@sorvell sorvell deleted the 4154-kschaaf-array-selector branch Jan 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment