Skip to content
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

Replace MPRIS implementation #12

Closed
wants to merge 1 commit into from
Closed

Conversation

suransea
Copy link
Collaborator

@suransea suransea commented Oct 8, 2024

I created the dbus-swift and mpris-swift libraries, providing a new MPRIS implementation. The new implementation eliminates external dependencies and adds support for macOS.

Changes:

  • Removed the old MPRIS implementation.
  • Integrated a new MPRIS implementation for better compatibility.
  • Updated relevant code and dependencies to support the new implementation.

@suransea
Copy link
Collaborator Author

suransea commented Oct 8, 2024

The CI build is currently failing because Swift 6 is not available.

- Removed the old MPRIS implementation.
- Integrated a new MPRIS implementation for better performance and compatibility.
- Updated relevant code and dependencies to support the new implementation.
Copy link
Owner

@ddddxxx ddddxxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while :). The new MPRIS implementation looks brilliant. Here's my suggestion. Thank you!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not commit unrelated files (also gitignore).

let updatePlayerState: () -> Void = { [weak self] in self?.updatePlayerState() }
disposables.append(try player.player.playbackStatus.observe(updatePlayerState))
disposables.append(try player.player.metadata.observe(updatePlayerState))
disposables.append(try player.player.seeked { _ in updatePlayerState() })
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[weak self]

g_clear_signal_handler(&signal, player)
}
g_object_unref(player)
disposables.forEach { try? $0() }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is misleading, we can't just dispose of them, right?

Can you wrap them in DBus library, like this?

class DBusHandlerRegistation {
    typealias UnregisterFuction = () throws(DBus.Error) -> Void
    let unregister: UnregisterFuction
    deinit {
        try? unregister()
    }
}

}
}

extension MusicPlayers.MPRIS: MusicPlayerProtocol {

public var name: MusicPlayerName? { nil }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use bus name? or just .mpris if not feasible.

/// - instance: The instance name of the media player, if any.
/// - queue: The dispatch queue for the D-Bus method calls.
public convenience init(
name: String, instance: String? = nil, queue: DispatchQueue? = nil
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to make it as simple as possible, treat queue, connection, and timeout as implementation details and don't expose them. We can always expose them later if someone needs it.

guard let manager = playerctl_player_manager_new(nil) else {
return nil
/// Just like `NowPlaying`, but automatically find available MPRIS players.
public final class MPRISNowPlaying: Agent {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we have player selection logic twice, in different repo. How about deriving from NowPlaying, only update player list and let NowPlaying do the selection?

@suransea suransea closed this Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants