PlayerFinder iterator

[Kanjirito]

As mentioned in #57, PlayerFinder would ideally have a way to iterate over the players found. This would make it easier for users to create their own filters by checking each player without the need to create a Vec<Player>. The problem with this is that currently (as far as I know) it's not possible to implement the Iterator trait that would yield a struct with a named lifetime because "generic associated types" are not yet supported. There are 3 solutions to this:

1. Wait until Generic Associated Types (GAT) get stabilized
2. Get rid of the named lifetime in Player
3. Don't use the Iterator trait

The first option would be ideal but there's no telling when it will happen (it's been in the works for at least 5 years now).

The second option can be done now but has a drawback. It would require changing Player.dbus_name and Player.path into something else, like String which seems like a fine replacement since both BusName and Path are basically just wrappers around Cow. The downside is that every time Player.connection_path() is called the Strings will need to be turned into BusName and Path which sounds less than perfect since they will always be the same. I'm not sure how much of a difference it would realistically make compared to the current implementation which clones the variables each time but it will probably be worse. One possible solution would be to make connection_path() a separate function and try to use some sort of cache but that sounds very awkward.

The last option is to just let the iterator struct have a next_player(&mut self) -> Option<Result<Player, DBusError>> method. You wouldn't be able to use it in a for loop but you could do a while let loop. This would also lose out on all of the provided methods that come with Iterator

let mut player_iter = PlayerFinder.iter_all();
while let Some(player_result) = player_iter.next_player() {
        // Do something
}
// Or if you only care about Ok cases
let mut player_iter = PlayerFinder.iter_all();
while let Some(Ok(player)) = player_iter.next_player() {
        // Do something
}

There is a 4th option and that's switching away from dbus-rs but that's a whole different topic that might not even solve this problem in the end.

[Mange]

The lifetimes were the original cause for me to not being able to implement the trait initially.

dbus-rs might already be numbered (#63), so it's not a completely unrealistic candidate.

However, I'm leaning towards just using owned types in Player instead and not worry about the micro-optimization of having to clone values sometimes. This is most likely always run on systems where that sort of micro optimization is not needed; who is running a media player on a small microcontroller where you're worried more about the time taken to clone a 25 character string rather than the playback of a media player?

There might also be a fourth option, which is to have an iterator of a new type that can then be converted into the real Player when you're done iterating it. Perhaps that type could be less capable, which also means it might not need the same number of methods. This approach all depends on what sort of thing people would want in a custom iterator.

If you just want to compare the name of the player against some sort of config file or regex, then it would suffice. If you want to check what each of them is doing, then it's not.

[Kanjirito]

Sorry for taking this long to respond.

Switching away from dbus-rs might a good idea in the long term but that's a lot of work that might or might not happen and the iterator can definitely happen before that.

I kinda like the idea of creating a "dummy Player" that the iterator would yield instead of the actual Player struct, something like this:

struct DummyPlayer {
    bus_name: String,
    connection: Rc<PooledConnection>,
}

impl DummyPlayer {
    fn initialize(self) -> Result<Player, DBusError> {
        Player::for_pooled_connection(self.connection, self.bus_name, ...)
    }
}

The problem is that at that point we are basically just iterating over a Vec<String> with the only benefit being not having to deal with the connection part since it will be shared. It's not bad and maybe there could be some different useful methods with it but I can't come up with any. Because of that I think it might be better to just remove the lifetime from Player. That is of course more work and a pretty big change but it feels like less of a hack than the dummy Player idea. There are some details that I want to talk about but that will be easier to do in a PR (that I'll send soon). Oh and now that I looked at this again I don't think the performance will change that much because currently it's copying stuff on every call anyway.

The other thing I want to ask is how the iterator itself will work. The simplest solution is to just call all_player_buses() to get the bus names and then just try to create a Player with them on every next() call. The issue would be that after the iterator was created one of the players could have exited or a new one started. I don't think there's any way to solve this without over engineering something and just letting the user deal with outdated data seems to be the best idea but maybe I'm missing something.

[Mange]

Good thoughts. I've commented your approach in #66, and it looks good to me.

[Kanjirito]

With #66 being merged I believe this issue and #57 can be closed.