ultrabug / py3status

py3status is an extensible i3status wrapper written in python
https://ultrabug.github.io/py3status/
BSD 3-Clause "New" or "Revised" License
883 stars 259 forks source link

Add playerctl module #2211

Closed jdholtz closed 9 months ago

jdholtz commented 11 months ago

Fixes #2205.

This PR adds support for the playerctl module. It uses Playerctl's Python binding library to monitor and extract information from players. Most of the configuration options are the same as the cmus module. There are three more added: button_shift, button_unshift, and ignored_players.

An important thing to note: If any text sent to i3bar contains an &, the text will not render. The & is common in videos and therefore creates a problem. I also see this issue in the cmus module, so it isn't directly an issue with this module, but should be solved to provide users with a better experience.

There are two ways I see solving this:

  1. Fix it in the playerctl module by doing some sort of str.replace("&", "and")
  2. Fix it in py3status, which would also fix this issue for other modules

I also have a few more configuration options in mind but would like to get others' opinions:

  1. A max_width option to render the text with an ... if it is too long. There are a lot of videos that span half the i3bar. However, I am not sure if this can be handled in Py3status's formatter or not.
  2. A player_priority option to initially select the active player. Currently, it selects the first one that playerctl selects. I don't think this option is necessary as I only see it being a factor during the post_config_hook (mostly only run when the user starts i3). After the initial start, active players will be cycled through by the most recent player to change status, metadata, or be opened. Additionally, configuration options are provided for the user to manually switch the active player (button_shift and button_unshift)

This might remove the need for the mpris module, but more information about a player is provided in that module so that be good to still keep. Additionally, the player_control module probably doesn't need to be kept as this module controls the same players.

Any feedback is appreciated. I will be willing to maintain this module (fixing bugs, adding new features, refactoring, etc.).

lasers commented 11 months ago

Fix it in py3status, which would also fix this issue for other modules

Does {placeholder:escape} works?

A max_width option to render the text with an ... if it is too long. There are a lot of videos that span half the i3bar. However, I am not sure if this can be handled in Py3status's formatter or not.

I think it was brought up in the past and possibly coded, but not implemented.

A player_priority option to initially select the active player. Currently, it selects the first one that playerctl selects. I don't think this option is necessary as...

Actually, I wanted to treat this as a loop of players, so all players that comes and go would appear on the bar along with its own buttons.

jdholtz commented 11 months ago

Does {placeholder:escape} works?

Yes, that works perfectly!

I think it [max_width] was brought up in the past and possibly coded, but not implemented.

If desired, I can make a PR implementing this config option for every module. It would be similar to the min_width option and show an ... if it cuts off extra output. It could be helpful for other modules, especially music player modules.

Actually, I wanted to treat this as a loop of players, so all players that comes and go would appear on the bar along with its own buttons.

Sounds good. I'll make the change. How would buttons be done? None of the modules you shared had support for on_click. Is there a way in Py3status to figure out which of the 2+ players that would be clicked? Also, what if a click is run from py3-cmd?

lasers commented 11 months ago

If desired, I can make a PR implementing this config option for every module...

If you desire it, then go and make it. FYI, I haven't used py3status for a long while, but can try to help.

... How would buttons be done? Is there a way in Py3status to figure out which of the 2+ players that would be clicked? Also, what if a click is run from py3-cmd?

Note: I made many things and some unreleased code / unmerged PRs may contain this. My memory is hazy and is probably 94.44% correct.

IIRC, essentially, it's done by making a composite first with (py3.safe_format) then you update that composite with index -- using py3.composite_update(existing_composite, new_index_dict). When you clicked something, on_click event will be returned -- and your index should be found in there too -- your index should be unique string too to make things simple / identifiable.

In main method, you should create unique usable index string such as firefox-1235. I picked pid num as an example since you could open multiple firefox instances). The idea here is to use something unique from playerctl data if it have one for each instance.

Inside on_click, you'll break up that index string into usable configs. I think you may have to do things like assigning firefox-1235/play to play button composite... then split that index to firefox-1235 (instance id) and if index == "play" (clicked).

Get a loop of players to work first, then it's a minimal fix to add index string.

This should work in py3-cmd too... see py3-cmd click as an example.... it would look something like py3-cmd click --button 1 --index "firefox-1234/pause" playerctl -- OK... You can look at timer module too... since it does contains index in the composite there.

jdholtz commented 11 months ago

@lasers, I finished your requested changes. The colors and indices work for each player separately. Should I document how to select the index? The index is the player name, not the player instance because playerctl does not track two of the same player (at least for browsers, so I assume every player).

jdholtz commented 11 months ago

I decided to automatically add :escape to title, based on your suggestions. Additionally, due to f-string formatting, I don't think the maximum length for a module is necessary because users have more precision over it using f-strings.

I also added support for fnmatch for tracked players. Is there a reason re.match is not used instead? I think it allows for much more exact configuration. For example, you can't negate just one word (ex. playerctld) with fnmatch without doing something like [!p]* which fails to match any player starting with p (at least, not to my knowledge).

Another important issue that I want to solve before this is officially added is the i3bar exiting unexpectedly when a browser instance is closed. I get the error Error: status_command process exited unexpectedly (exit 0). However, a 0 exit code would indicate success. What's stranger is that I don't get this error when running it from the command line (the logs don't show anything either), nor can I catch this supposed error when wrapping all of my code blocks in try/excepts. Do you know of any way to debug this further?

lasers commented 11 months ago

I decided to automatically add :escape to title

Nice on adding :escape.... Making it easier for users. :-)

Is there a reason re.match is not used instead?

No particular reason other than fnmatch is being used in many modules already (and possibly not needed). If you need to negate something, then we can roll back. Not familiar with playerctld. I assume users would do no more than to target their favorite players or just ignore that one player (ie wanting all information from playerctl).

...i3bar exiting unexpectedly...

I see your issue. I wonder if it had to do something with Glib/Thread loop. git grep -Iri glib py3status/modules/*.py shows slightly different ways to run thread loops. I wonder if it's the current implementation that's causing the crash on exit... I don't recall crashes with conky module too.

I toyed with your module some in virtual machine. For me, it is chromium that crashes it. Firefox does it okay. I don't think it's worth creating individual buttons (like mpris) since clicking full player does the job and is friendly enough.

This is going to be a nice module for many users... and we probably should remove player_control module too in favor of this one.... I don't even think that module works right now anyway. :-)

I think we need to look at cache_timeout / sleep_timeout again too... I'm not seeing your description on how it works when I use self.py3.CACHE_FOREVER in there. (ie it gets updated constantly)

format_player = ... [{title}]...

Imho, I would add [{title}|{player}] in your format_player to show player name too if no title (ie vlc).

jdholtz commented 11 months ago

I fixed the crash that was happening by adding a very small delay (time.sleep(0.01)) before updating the module after a player vanishes. This might not be the best way to do it, but it works perfectly for me and such a short delay would make no noticeable difference to the user.

I found out that the reason for this is because Playerctl is not given enough time to remove the player from the manager before this module retrieves tracked players. It would then error when getting player.props.metadata as the metadata no longer existed.

Additionally, I removed the _init_manager function and added a default {player} to the format_player when no title exists.

jdholtz commented 11 months ago

Hey @lasers, I believe I have addressed every one of your points. Please let me know if you have any other suggestions. Thanks very much for helping improve this module a lot!

jdholtz commented 11 months ago

Hey @lasers, I finished adding support for everything I think is possible with Playerctl. This includes displaying and changing the volume, shuffle mode, loop status, and seeking forward/backward with buttons.

While doing this, I noticed a few odd behaviors. Cmus changes the volume and loop status (not shuffle) using playerctl, but playerctl does not respond to updates from the player, so it effectively does not work. For example, to increase the volume, the current volume is added to a volume_change configuration value. However, the volume for the player will always be the same so it will only change once. This seems to only be an issue with Cmus though as Spotify works perfectly for all of these new controls.

I also looked into how to mute the players, but there is no specific functionality to mute using MPRIS. Setting the volume to 0 doesn't work because this module doesn't remember the previous volume (cmus also doesn't mute correctly natively). If you have any suggestions to do this, I would be happy to add them. However, it seems infeasible because it depends on the player's volume integrating well with playerctl and the volume before mute might need to persist even when py3status restarts.

Last, I added a better caching algorithm so the module only updates when necessary.

lasers commented 11 months ago

You should make a PR to remove player_control module too. 😉

jdholtz commented 11 months ago

I guess it couldn't be helped with excessive buttons... Sometimes I wonder having excessive buttons in the modules is one of annoying thing about modules... and thought about making buttons = {"volume_up": 4, "volume_down": 5} dict config to make it go from 12 down to 1. I think that could be not friendly for users too.

I wouldn't be opposed to adding a dictionary for the buttons. However, it doesn't follow the configuration of other modules so it might be more confusing to users than just having 12 button configuration values.

jdholtz commented 11 months ago

You should make a PR to remove player_control module too.

There are a few other modules that could possibly be removed as well.

  1. gpmdp -- playerctl supports Google Play Music Desktop Player (AFAIK from the repository issues)
  2. mpris -- this is basically the playerctl module using DBus instead of Playerctl (but with less functionality)
  3. clementine -- playerctl supports the Clementine music player
  4. spotify -- uses a DBus implementation to access Spotify

All of the above don't appear to provide any more data/functionality that this module can provide. I don't think cmus should be removed because it provides a lot more information than Playerctl can get.

Is there a process to removing modules, similar to deprecating configs?

lasers commented 11 months ago

Is there a process to removing modules, similar to deprecating configs?

Not linking to unrelated pulls.

nvidia_temp -- https://github.com/ultrabug/py3status/pull/1853 
bitcoin_price -- https://github.com/ultrabug/py3status/pull/2047
...et cetera
ultrabug commented 11 months ago

Hello @jdholtz ; thanks for this amazing work and first contribution to py3status (thanks to you too @lasers)

I had some work to do on this project which I hope has been covered yesterday with the 3.52 release.

I'll do my best to look at and test your module quickly.

Kindly

ultrabug commented 11 months ago

Meanwhile, I would be grateful if you could rebase your branch with current master (I changed quite a lot of things regarding to CI and build system).

jdholtz commented 11 months ago

Thanks @ultrabug. I rebased my branch, so let me know if anything else should be done.

jdholtz commented 10 months ago

This is more of a configuration question (I didn't want to open a new issue for it. Maybe opening GitHub discussions could help users with similar questions), but how can I use multiple format specifiers? I want to escape the title and limit it to 15 characters, but my current configuration does not work even though it does in Python's f-string formatting (which this appears to be based off of). Here is what I tried: {title:escape.15s}. {title:.15s} works, but then it gets rid of the default escape. Is this possible yet in Py3Status?

lasers commented 10 months ago

IIRC it's not from Python f-string formatting. Some things may behave like that.

As for length, you may want max_length.... See git grep length= tests -- so length is not limited to placeholders only, but the whole block... so we can add other things inside the brackets too.

jdholtz commented 10 months ago

Wrapping the title in a max_length block worked. Thanks!

jdholtz commented 9 months ago

@ultrabug any updates to this? I’ve been using it for over a month now and it has worked very well for me.

ultrabug commented 9 months ago

Thanks a lot @jdholtz and @lasers

I'm merging tho CI fails, I'll fix that myself before releasing a new version of py3status

jdholtz commented 9 months ago

Thanks @ultrabug. The code QA commit did wrongly move an import from line 75 to 73 in this commit. When running the module in test mode (python playerctl.py), it prints the message /home/jdholtz/.config/py3status/modules/playerctl.py:73: PyGIWarning: Playerctl was imported without specifying a version first. Use gi.require_version('Playerctl', '2.0') before import to ensure that the right version gets loaded. which is why I had the import after. However, it isn't a big deal since it cannot be seen in the log file and doesn't cause any issues.

Also, let me know if I should submit any PRs to deprecate modules that are similar to Playerctl's functionality but are old. I mentioned possible module deprecations here.

ultrabug commented 9 months ago

@jdholtz indeed, CI was not happy about it. So if that doesn't break anything let's leave it.

Also, let me know if I should submit any PRs to deprecate modules that are similar to Playerctl's functionality but are old. I mentioned possible module deprecations here.

It's hard to know which modules are used or not. I guess clementine and gpmdp are very old and unmaintained.

I'm sure that spotify and mpris have a strong user base so I would not touch them.