Queue ListView incorrect shuffle representation.

[ThomasFrans]

Describe the bug
ListView::get_indexes_of() doesn't take order into account, and disabling/enabling shuffle doesn't force an update of the search indexes.

See #465 for more info

To Reproduce
Steps to reproduce the behavior:
Search in queue view and shuffle and unshuffle.

Additional context
This is less detailed, I just put this issue to track this so I don't forget to solve this. I didn't think about this when adding the order feature (#1060, reverted), so I'll fix it 🙂

[ThomasFrans]

@hrkfdn I was trying to solve this problem but the way it would currently be solved is pretty messy. Most of the methods in ListView would have to be aware of how there is an explicit order on the content of Queue, which could be done now, but would probably result in more bugs in the future if someone has to work on ListView and would forget that there is the order to take into account.

I was wondering whether it would make more sense to combine the items and the order in the Queue into a separate struct with methods that hide the fact that an explicit order is in use. It would result in quite a bit of refactoring and extra code but I think putting the order and the items together would help when representing the state of the Queue in the UI, as the UI generally doesn't care about the raw data, it just cares about the data in the correct order as given by random_order in Queue. It could probably also simplify the implementation of Queue itself as it wouldn't have to worry about the ordering anymore, although I don't know if it ever needs direct access to the raw data which would be private in the struct (or could be given read only as a reference).

Maybe I'm over-complicating things, I have a habit of doing that. I just think it is a bit strange for the ListView to have to worry about the implementation of the item ordering in Queue. I wanted to ask what you thought about this issue?

[hrkfdn]

Maybe I'm over-complicating things, I have a habit of doing that. I just think it is a bit strange for the ListView to have to worry about the implementation of the item ordering in Queue.

Totally agree. However, I'm not sure how simple a layer between ListView and Queue could be. The problem is that ListView needs to be able to manipulate the queue and vice versa, as the queue can change (i.e. when a new track is added) which would have to end up back in ListView with the "shuffe-layer" in between. Do you have an idea on how to solve this already?

FYI, I wanted to cut a release today, so I should probably revert the queue order display change. But we can merge it again once we have a solution for this. Hope that's ok.

[ThomasFrans]

Maybe I'm over-complicating things, I have a habit of doing that. I just think it is a bit strange for the ListView to have to worry about the implementation of the item ordering in Queue.

Totally agree. However, I'm not sure how simple a layer between ListView and Queue could be. The problem is that ListView needs to be able to manipulate the queue and vice versa, as the queue can change (i.e. when a new track is added) which would have to end up back in ListView with the "shuffe-layer" in between. Do you have an idea on how to solve this already?

FYI, I wanted to cut a release today, so I should probably revert the queue order display change. But we can merge it again once we have a solution for this. Hope that's ok.

Yes I was maybe also going to suggest to revert it. For the implementation for Queue and ListView, I was thinking to keep the shared state using the Arc but put the new version of the 'orderable' Vec in there. Like that the updates would still be visible to both, but neither would have to be concerned with translating the explicit order.

So the Queue and ListView would have an Arc<RWlock<OrderVec>>. I think that might be the simplest.

[ThomasFrans]

Seems like I once again forgot about the existence of Box and other heap allocation API's. I guess it would be far, far simpler to have a simple Vec of heap allocated objects (either Box or Rc/Arc) and shuffle those. Then the ListView doesn't have to change. I was so hyper-focused on the current approach with the separate indexes that I didn't care to think whether there was a simpler approach...

[ThomasFrans]

Note

I might have over-complicated the solution or missed an obvious, correct way to solve this. I try my best 😝

Solution So Far

I think I will give up on solving this issue for now. I've tried three methods to solve it and spent quite a bit of time but I find it very hard to correctly implement this in a way that doesn't require too much changes to the rest of the code or doesn't make it even harder for the next person to maintain and reason about. Here are the things I tried in case anyone else wants to have a go:

1. First I tried the simplest approach of just sharing both the data and the random order between the Queue and the ListView. That wouldn't be too hard to implement but it requires ListView to handle all the ordering logic from Queue again which isn't ideal as I mentioned in the start of this issue.

2. Second I tried to implement a thing that combined both data and order and handled the order internally (the OrderedVec approach) which doesn't really work either as ListView needs a Vec and changing that is again a lot of refactoring work.

3. Third I tried with heap allocations which seemed like the best approach but I forgot that there is no way to mutate the data in the Rc's that are used to share the data between the 'raw data' and 'order' Vec. Unless each one uses an interior mutability pattern or unless they don't share the data but use a Box instead[1] which could cause other syncing issues, I don't see a way to cleanly solve it.

The big problem I'm having is that Queue, ListItem and ListView are very much dependent on each other which makes it hard to change one without having to change all the others as well. I've tried to make them less dependent before in #1011 but I'll be honest, that wasn't a perfect implementation either. Therefore I think I'll leave this attempt for now. Like I said it's possible and not that hard to implement it right now using approach 1, but I don't want to do that knowing that it would only increase the dependency of the two which makes maintenance that much harder for the next person. I would rather work on a way to reduce that dependency first which would make this issue far easier to solve.
.
.
.

[1] roughly this with ordered shared with ListView

struct Queue {
    queue: Vec<Playable>,    // To know initial order
    ordered: Arc<RwLock<Vec<Box<Playable>>>>,    // Box for cheap in place sorting, especially with lots of Playables
    ...
}