New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle a few sort plugin edge cases #1718

Merged
merged 1 commit into from Sep 3, 2015

Conversation

Projects
None yet
2 participants
@akagomez
Contributor

akagomez commented May 27, 2015

  • Batched list operations fired events without a batchNum (#1707)

    Swapped out all of the can.trigger's with can.batch.trigger and made it
    so that can.batch.trigger adds the can.batch.batchNum when creating an
    event object.

  • Batched list operations evaluated insert index despite batchNum (#1706)

    Fixed the logic that returns/sorts based on the current event's batchNum

  • change events caused list items to be swapped with same-value siblings (#1705)

    The default selection sort is kind of greedy in that it continues to
    search for an insert index until it finds an item with a greater value,
    or the end of the list. This means that it will move an item passed
    siblings of the same value every time the _getInsertIndex is evaluated.
    Modifying the algorithm to swap to items that are equal OR greater doesn't
    fix the issue either, it just reverses the direction. The fix was to
    check if the item to the right of its current position is the same value. If it is,
    don't move it. It's fine where it's at.

  • change events evaluated insert index when irrelevant values changed (#1722 fix 1 of 2)

    Previously, change events would naively call _getInsertIndex even if
    the property that changed wasn't the comparator. This was inefficient since
    it resulted in a loop over the items in the list needlessly. Now the changed
    attr is compared to comparator on every change event. If comparator
    is a function OR the changed attr is a substring of the comparator value, the
    insert index is evaluated.

  • Added a few tests to make development a bit easier

@@ -216,7 +216,8 @@ steal('can/util/can.js', function (can) {
// Don't send events if initalizing.
if (!item._init) {
event = typeof event === 'string' ? {
type: event
type: event,
batchNum: can.batch.batchNum

This comment has been minimized.

@akagomez

akagomez May 27, 2015

Contributor

@justinbmeyer Am I right to assume that this is a reasonable default? It seemed correct based on how I understand can.batch.batchNum to work. I wondered if it should be added to the other event objects created here and here also.

@akagomez

akagomez May 27, 2015

Contributor

@justinbmeyer Am I right to assume that this is a reasonable default? It seemed correct based on how I understand can.batch.batchNum to work. I wondered if it should be added to the other event objects created here and here also.

This comment has been minimized.

@akagomez

akagomez May 28, 2015

Contributor

TODO: Add test; Create issue;

@akagomez

akagomez May 28, 2015

Contributor

TODO: Add test; Create issue;

This comment has been minimized.

@akagomez

akagomez May 28, 2015

Contributor

Justin agreed this is fine here, but not so in the other two places.

@akagomez

akagomez May 28, 2015

Contributor

Justin agreed this is fine here, but not so in the other two places.

list._getRelativeInsertIndex = function () {
ok(true, 'This item should be evaluated independently');
return _getRelativeInsertIndex.apply(this, arguments);
};

This comment has been minimized.

@akagomez

akagomez May 27, 2015

Contributor

@daffl Is it okay that I'm testing the internal API of the sort plugin this way?

@akagomez

akagomez May 27, 2015

Contributor

@daffl Is it okay that I'm testing the internal API of the sort plugin this way?

This comment has been minimized.

@daffl

daffl May 28, 2015

Contributor

I would say yes. If we change the internal API we'll have to change the tests which is fine though.

@daffl

daffl May 28, 2015

Contributor

I would say yes. If we change the internal API we'll have to change the tests which is fine though.

this.sort();
this._lastProcessedBatchNum = ev.batchNum;
return;
}

This comment has been minimized.

@akagomez

akagomez May 27, 2015

Contributor

My previous batch handling was a joke, which inspired the can.batch.debounce discussion here: #1708

@akagomez

akagomez May 27, 2015

Contributor

My previous batch handling was a joke, which inspired the can.batch.debounce discussion here: #1708

Chris Gomez
Handle a few sort plugin edge cases
- Batched list operations fired events without a `batchNum`

  Swapped out all of the `can.trigger`'s with `can.batch.trigger` and made it
  so that `can.batch.trigger` adds the `can.batch.batchNum` when creating an
  `event` object.

- Batched list operations evaluated insert index despite `batchNum`

  Fixed the logic that returns/sorts based on the current event's `batchNum`

- `change` events caused list items to be swapped with same-value siblings

  The default selection sort is kind of greedy in that it continues to
  search for an insert index until it finds an item with a greater value,
  or the end of the list. This means that it will move an item passed
  siblings of the same value every time the `_getInsertIndex` is evaluated.
  Modifying the algorithm to swap to items that are equal OR greater doesn't
  fix the issue either, it just reverses the direction. The fix was to
  loop over the items between the current index and the suggested index
  when a "forward" swap was pending and override the insert index if an
  equal value item is found.

- `change` events evaluated insert index when irrelevant values changed

  Previously, `change` events would naively call `_getInsertIndex` even if
  the property that changed wasn't the comparator. This was inefficient since
  it resulted in a loop over the items in the list needlessly. Now the changed
  `attr` is compared to `comparator` on every `change` event. If `comparator`
  is a function OR the changed attr is substring the comparator value, the
  insert index is evaluated.

@daffl daffl added this to the 2.2.8 milestone Sep 3, 2015

daffl added a commit that referenced this pull request Sep 3, 2015

@daffl daffl merged commit 2604df6 into master Sep 3, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@daffl daffl deleted the sort-plugin-edge-cases branch Sep 3, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment