Async rewrite #86
Draft
Async rewrite #86
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Additionally removes the blocking code generation since it's useless now
Includes the properties and methods from the org.mpris.MediaPlayer2 and org.mpris.MediaPlayer2.Player interfaces.
MprisError is now the general error that's returned from all of the functions. TrackID now uses zbus::ObjectPath to verify if the string is a valid D-Bus path. Also includes helpful traits and methods.
The proxies use a owned BusName anyway so there's no need for a lifetime
Player now holds the BusName directly since the proxies use a Arc<str> anyway. Added `seek_forwards` and `seek_backwards` that take a Duration instead of just an i64. `get_position` and `set_position` now also take a Duration instead of i64 Also added the DurationExt trait for Duration that makes it easy to convert from Duration to a Result<i64, MprisError>
PlayerStream just holds a Vec of Futures that return a Player and polls them one by one. The new futures-util dependency is for the Stream trait and the StreamExt trait which makes using streams easier.
Both proxies are now created with join!
For some finder methods it's more efficient to stop when a matching Player is found.
Added `Player::bus_name_trimmed()` which strips the prefix and `Player::is_running()` which uses the D-Bus Ping method to see if the player is still connected. Also renamed `Player::has_track_list()` to `supports_track_list()`
Created a simple macro that adds the Display trait for the Errors. This is just a placeholder for now.
Instead of using `Duration` and working around the type conversions now there's `MprisDuration` which will always be between 0 and i64::MAX because the MPRIS spec returns length and position as a i64 but it can't be negative. `MprisDuration` uses a u64 internally to avoid having to deal with negative numbers when doing math operations. For Serde it acts like a i64 since that's the type in the spec but it can be converted to and from i64, u64 and Duration.
Added `From<T> for MetadataValue` and `TryFrom<MetadataValue> for T` for all of the variants of MetadataValue.
`From<String>` and `From<&str>` have been added in the display macro for errors.
No point in wasting time converting it since we don't actually care what the data is
The integer types will now try to covert to the other integer type without losing any data when using the `TryFrom` trait. `TryFrom` for String will now also work for `MetadataValue::Strings` if there's only one String inside.
The whole struct is now generated through the `gen_metadata_struct` macro. This is done to avoid having to copy and paste the fields and keys around while also making it easy to change the fields. The macro generates a couple of methods and traits for the struct which are documented on the macro itself. Additionally `From<RawMetadata>` has been changed to `TryFrom` because if there's a problem with the type the conversion should fail instead of silently changing the values.
Most of them are just `try_from().ok()` anyway
Switched TrackID to holding `OwnedObjectPath` directly and changed the conversion traits. All of them now check for the "/org/mpris" prefix and you can no longer convert from borrowed types (outside of &str). And more tests
`AsRef<OwnedObjectPath> for TrackID` to make it easier to use `TackID` as an argument for the Proxy and `From<OwnedValue> for MetadataValue`
Added the lint rules that were present before which meant that a Debug implementation for `Mpris` and `PlayerStream` had to be added. Also added a way to get to the `Connection` and `Executor` from `Mpris`.
Instead of holding a `Vec` of futures it now holds a `VecDeque` of bus names, a`Connection` and the currently worked on future (if any). This allows for a much more useful `Debug` output and prevents cloning things unless they are needed. Also added `Clone` for `Player` and made the `Debug` formatting for `Mpris` and `PlayerStream` skip the `Connection` fields because it's a lot of noise and if needed you can easily just debug print the connection directly.
Added ops traits (with tests) for `Self` and `Duration`. The ops tests now also use a macro. Renamed the argument names for `new_from_u64()` and `new_from_i64()` to make it more obvious what the value means. Added `new_from_duration()`, `as_u64()` and `as_i64()`
All of the errors right now are the same so they can easily be generated with a macro. Also publicly imported `zbus::Error` so that it's included in the documentation.
Added `Metadata::is_valid()`. `Metadata::get_metadata_key()` is no longer a method. Added `PlaylistOrdering::as_str()` that returns the struct variant name as &str and the `Display` now also uses it. Returned `ObjectPath` now have a lifetime to make it more obvious that they are borrowed.
`Player` no longer needs a reference to `Mpris` for `new()`. Added missing fullscreen related methods on the proxy and removed a method that isn't part of MPRIS. Added argument checks for `set_position()` and `set_playback_rate()`
Everything has been documented. NO_TRACK is now a associated constant in `TrackID` and it's public. `RawMetadata` was made public.
Players seem to ignore the "/org/mpris" restriction so it's no longer required. It can be checked with the `is_valid()` method. This also means that converting from `ObjectPath` can now be `From` instead of `TryFrom`.
`Player::unique_bus_name()` will return the unique bus name of the player if a player is connected. To make this work `Player::new()` is back to taking a `&Mpris` so that it can copy `DBusProxy` which is used to look up the unique name. Other changes: `Mpris::into_stream()` renamed to `stream_players()` because it doesn't consume Mpris. `PlayerStream` now also holds `DBusProxy` `MetadataIter` renamed to `MetadataIntoIter` Some minor formatting changes
Created a `PlaylistsInterface` struct that holds the `PlaylistsProxy` and saves the changes received from the PlaylistChanged signal so that `Playlist` can get updated with the new values.
Added `Mpris::new_no_executor()` which disables the executor and added the `ExecutorLoop` future that ticks the executor in a loop. This way the user doesn't need to interact with the zbus library directly. To make this possible `Mpris` now holds the DBusProxy inside a `OnceCell` that gets initialized the first time some method needs it. This is needed because when you create the `Connection` without the internal executor trying to create the Proxy will hang. All of the Executor stuff (including some documentation) is hidden if you enable the tokio feature. TrackID got it's Debug changed.
`MetadataValue` now (de)serializes directly into the contained value. `Unsupported` turns into unit and it's the default values if no other matches. When serializing from a digit `i64` is preferred and it will only deserialize into a `u64` if the value is too big to fit `i64`. Tests included. `Playlist.icon` will now turn from/into a `String` the same way the `From` traits work. Also includes minor documentation and formatting changes.
Instead of being just an alias it's now a struct that just wraps the HashMap. This is done so that it's possible to implement From traits making it a lot easier to covert from the HashMap received from DBus. It implements Deref and other traits to make it act like a HashMap.
The proxies now return owned values since they are getting cloned anyway.
No point in going to and back from a `String` when we known that it's an `ObjectPath`. Since only the track id metadata key should have a path we can just use `TrackID`. Also added `TrackID::from_str_unchecked()` to make tests easier to write.
zbus allows to directly use custom types in the proxy macro but do that some of the types need to implement serde because of this serde is no longer an optional dependency. This commit adds all of the needed changes needed to integrate the types into zbus. This includes serde implementations, adding `From<Value>` and `Into<Value`, implementing the `zbus::zvariant::Type` trait and `Into<MprisError>` for the zbus Error types. In the cases where it was impossible to integrate the type (`MetadataValue` and `Playlist`) in a nice way the proxy methods are just wrapped with new methods that avoid the issue.
|
i see that the last commit on this is from 2months ago, are there any plans on continuing the work on this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I decided to give the async rewrite a go and this PR for now is just to get some feedback and see if I'm going in the right direction. This is still very much a WIP but the basics are there. The 4 interfaces are implemented and tested (well 95% of it because so far I didn't find a player that implements all of them) and there's documentation written for them so it should be usable for basic player control.
Since this is a complete rewrite because of async anyway I did not follow the current implementation very closely. It should be similar enough to be relatively easy to switch to but it's not a 1 to 1 rewrite. This of course is completely subjective and I'm fine with trying to make it similar to the way it currently is.
The big things that are missing:
PlayerEventsandProgressTrackerare not implemented yet.checked_*methods onPlayer. I don't really think they are essential but they also don't really hurt anything so I'm not against them.The nice thing about switching to async is that it's now possible to pretty easily create background tasks (through
zbus) that would listen to the signals from theTracklistandPlaylistsinterfaces and keep the data updated. The downside is thatProgressTracker::tick()is more tricky because there's no good runtime agnostic way to sleep and zbus doesn't have timeouts as far as I know. One solution is to use theasync_iocrate for the sleeping sincezbusalready uses it when used without thetokiofeature orProgressTickcould implementFuture(I haven't really looked into this but I'm sure there's a way to make that work) and it would be up to the user to useselect!to create a timeout. There's also the option to not be runtime agnostic and just usetokio.