im-util: Add Sort, SortBy adapters

[jplatte]

No description provided.

[Hywan]

The code looks good, thanks!, but we need to add tests for Sort and SortBy :-). I know the implementation is shared between all the sort stream adapters, but… eh…

[jplatte]

I started updating the tests and I think I found a bug. Check this test which currently passes (ignore the copy-pasted comments), I don't see how that could make any sense.

[jplatte]

Yeah so this is unrelated to my changes, this also succeeds on main when logically only a Set should be emitted:

#[test]
fn bug() {
    let mut ob = ObservableVector::<char>::from(vector!['d', 'e', 'b', 'g']);
    let (values, mut sub) = ob.subscribe().sort_by(&|a, b| b.cmp(a));

    assert_eq!(values, vector!['g', 'e', 'd', 'b']);

    ob.set(0, 'c');
    assert_next_eq!(sub, VectorDiff::Remove { index: 2 });
    assert_next_eq!(sub, VectorDiff::Insert { index: 2, value: 'c' });

    drop(ob);
    assert_closed!(sub);
}

[Hywan]

This isn’t technically a bug. The Remove + Insert can indeed be simplified into a single Set if the index is the same. I can add this as an optimisation, which is likely to be necessary for performance reason. Good catch.

[jplatte]

Right. Just very unexpected :)

I guess I should finish the tests independently of that optimization.

[Hywan]

Hmm, actually, this optimisation is already present, and even tested.

The test:

eyeball/eyeball-im-util/tests/it/sort.rs

Lines 337 to 343 in 71e0a72

	// Same value.
	ob.set(0, 'd');
	assert_next_eq!(sub, VectorDiff::Set { index: 1, value: 'd' });
	// Another value, that is sorted at the same index.
	ob.set(0, 'c');
	assert_next_eq!(sub, VectorDiff::Set { index: 1, value: 'c' });

The code:

eyeball/eyeball-im-util/src/vector/sort.rs

Lines 504 to 531 in 71e0a72

	match old_index.cmp(&new_index) {
	// `old_index` is before `new_index`.
	// Remove value at `old_index`, and insert the new value at `new_index - 1`: we need
	// to subtract 1 because `old_index` has been removed before `new_insert`, which
	// has shifted the indices.
	Ordering::Less => {
	buffered_vector.remove(old_index);
	buffered_vector.insert(new_index - 1, (new_unsorted_index, new_value.clone()));
	result.push(VectorDiff::Remove { index: old_index });
	result.push(VectorDiff::Insert { index: new_index - 1, value: new_value });
	}
	// `old_index` is the same as `new_index`.
	Ordering::Equal => {
	buffered_vector.set(new_index, (new_unsorted_index, new_value.clone()));
	result.push(VectorDiff::Set { index: new_index, value: new_value });
	}
	// `old_index` is after `new_index`.
	// Remove value at `old_index`, and insert the new value at `new_index`. No shifting
	// here.
	Ordering::Greater => {
	buffered_vector.remove(old_index);
	buffered_vector.insert(new_index, (new_unsorted_index, new_value.clone()));
	result.push(VectorDiff::Remove { index: old_index });
	result.push(VectorDiff::Insert { index: new_index, value: new_value });
	}
	}

[Hywan]

Actually, it's not a bug… Taking this code:

let mut ob = ObservableVector::<char>::from(vector!['d', 'e', 'b', 'g']);
let (values, mut sub) = ob.subscribe().sort_by(&|a, b| b.cmp(a));

assert_eq!(values, vector!['g', 'e', 'd', 'b']);

ob.set(0, 'c');
assert_next_eq!(sub, VectorDiff::Remove { index: 2 });
assert_next_eq!(sub, VectorDiff::Insert { index: 2, value: 'c' });

So, when doing set(0, 'c'), the algorithm will compute the old_index and the new_index:

eyeball/eyeball-im-util/src/vector/sort.rs

Lines 488 to 502 in 71e0a72

	// We need to _update_ the value to `new_value`, and to _move_ it (since it is a
	// new value, we need to sort it).
	//
	// Find the `old_index` and the `new_index`, respectively representing the
	// _from_ and _to_ positions of the value to move.
	let old_index = buffered_vector
	.iter()
	.position(|(unsorted_index, _)| *unsorted_index == new_unsorted_index)
	.expect("`buffered_vector` must contain an item with an unsorted index of `new_unsorted_index`");
	let new_index =
	match buffered_vector.binary_search_by(|(_, value)| compare(value, &new_value)) {
	Ok(index) => index,
	Err(index) => index,
	};

The old_index in this case is 2, which is correct: (0, 'd') is at the position 2 in the “sorted view”. The new_index in this case is 3, which is… also correct because of how binary_search_by with this compare function works. The position to insert c is at index 3 because the vector is reverse-ordered.

And indeed, if you change sort_by(&|a, b| b.cmp(a)) to sort_by(&|a, b| a.cmp(b)), you get:

let mut ob = ObservableVector::<char>::from(vector!['d', 'e', 'b', 'g']);
let (values, mut sub) = ob.subscribe().sort_by(&|a, b| a.cmp(b));

assert_eq!(values, vector!['b', 'd', 'e', 'g']);

ob.set(0, 'c');
assert_next_eq!(sub, VectorDiff::Set { index: 1, value: 'c' });

drop(ob);
assert_closed!(sub);

I know, it's surprising, but it's indeed perfectly logical.

One thing that is questionnable though: is it correct to assume that if 2 indices are side-by-side, we can optimise it, like if we have old_index == new_index - 1 (so, Remove { index: old_index } then Insert { index: new_index - 1 }, we can consider we are updating the same place?

[Hywan]

OK, see #49 for the fix. Thanks for noticing this!

[Hywan]

Why is it commented?

[jplatte]

Looks like I didn't finish the tests after finding the issue you fixed in #49. Updated.

[Hywan]

LGTM!