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

MPRIS DBus integration & multiple media players #98

Merged
merged 14 commits into from
Feb 11, 2025

Conversation

mazei513
Copy link
Contributor

@mazei513 mazei513 commented Feb 2, 2025

Directly integrate with MPRIS DBus interface instead of using playerctl. With this, CPU time is significantly lower (simple check through top).

This PR also implements support for displaying more than 1 player at a time in the Media Player module menu.

The media player panel now also lists all media players. Styling is
still to be done.
It will only fetch MPRIS player data without refetching the list of
MPRIS players if there was no DBus signal that the list of players
changed.
Media Player module will now show the title if there's only 1 player,
mimicking the previous behaviour. It will show the note icon if there
is more than 1 player.
Copy link
Owner

@MalpenZibo MalpenZibo left a comment

Choose a reason for hiding this comment

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

First of all: Great Job!!

I left some comments.

An addition that could be nice is to check the PlaybackStatus from dbus to show the correct play or pause icon instead of a generic PlayPause icon

src/modules/media_player.rs Outdated Show resolved Hide resolved
src/modules/media_player.rs Show resolved Hide resolved
src/modules/media_player.rs Outdated Show resolved Hide resolved
iced::widget::horizontal_rule(2).into(),
container(
column![]
.push_maybe(d.song.clone().map(text))
Copy link
Owner

Choose a reason for hiding this comment

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

Would be nice to have a fallback value where we cannot find the song title. Something like NoTitle or boh I don't know some common default value in this case 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with "No Title" for now.

I think in the future it can rely on Track_Id as a fallback, though if it has custom formatting support its back to square one. I have not looked at other status bars/shells to see what they do, but would think that's probably the easiest way to figure out what's sensible.

src/services/mpris/mod.rs Outdated Show resolved Hide resolved
src/services/mpris/mod.rs Outdated Show resolved Hide resolved
src/services/mpris/mod.rs Outdated Show resolved Hide resolved
src/services/mpris/mod.rs Outdated Show resolved Hide resolved
@mazei513
Copy link
Contributor Author

mazei513 commented Feb 4, 2025

An addition that could be nice is to check the PlaybackStatus from dbus to show the correct play or pause icon instead of a generic PlayPause icon

Was trying to keep the changes to a minimum with at least feature parity with #94 and the multiple players support. Will look into it along with checking CanControl and etc. in a separate PR.

Simplified a filter + map into a filter_map
Simplified a future::ready with async move
MediaPlayer module no longer keeps its own state of the connected
players. It instead relies on the data in the MPRIS service itself.
@mazei513
Copy link
Contributor Author

mazei513 commented Feb 5, 2025

Have made the changes with the suggested fixes.

Copy link
Owner

@MalpenZibo MalpenZibo left a comment

Choose a reason for hiding this comment

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

🚀
Just two minor things. We also forgot to update the README config section with the new module name.

Comment on lines 63 to 67
s.deref()
.iter()
.flat_map(|d| {
let d = d.clone();
let title = text(Self::get_title(&d, config));
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
s.deref()
.iter()
.flat_map(|d| {
let d = d.clone();
let title = text(Self::get_title(&d, config));
s.iter()
.flat_map(|d| {
let title = text(Self::get_title(d, config));

Comment on lines 141 to 148
self.service.as_ref().and_then(|s| {
let data = s.deref();
match data.len() {
0 => None,
_ => Some((
row![
icon(Icons::MusicNote),
text(Self::get_title(&data[0], config))
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
self.service.as_ref().and_then(|s| {
let data = s.deref();
match data.len() {
0 => None,
_ => Some((
row![
icon(Icons::MusicNote),
text(Self::get_title(&data[0], config))
self.service.as_ref().and_then(|s| {
match s.len() {
0 => None,
_ => Some((
row![
icon(Icons::MusicNote),
text(Self::get_title(&s[0], config))

@mazei513
Copy link
Contributor Author

mazei513 commented Feb 10, 2025

We also forgot to update the README config section with the new module name.

It seems correct on my end and when looking at master though. Is my mind fried 😵‍💫???

# Media player module configuration
mediaPlayer:
  maxTitleLength: 100 # optional, default 100

Also pushed the changes.

@MalpenZibo
Copy link
Owner

We also forgot to update the README config section with the new module name.

It seems correct on my end and when looking at master though. Is my mind fried 😵‍💫???

# Media player module configuration
mediaPlayer:
  maxTitleLength: 100 # optional, default 100

Also pushed the changes.

Ahah no that section it's ok. Last time I forgot to check this other section

# Declare which modules should be used and in which position in the status bar.
# This is the list of all possible modules
#  - AppLauncher
#  - Updates
#  - Clipboard
#  - Workspaces
#  - WindowTitle
#  - SystemInfo
#  - KeyboardLayout
#  - KeyboardSubmap
#  - Tray
#  - Clock
#  - Privacy
#  - Settings
# optional, the following is the default configuration
modules:

Also, the CI shows an unused use std::ops::Deref;

@mazei513
Copy link
Contributor Author

Fixed the use declaration and updated the README.

@MalpenZibo MalpenZibo merged commit 7ebbf11 into MalpenZibo:main Feb 11, 2025
2 checks passed
@MalpenZibo
Copy link
Owner

Great job!

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