BlingCorp / bling

Utilities for the awesome window manager
https://blingcorp.github.io/bling/
MIT License
852 stars 49 forks source link

Cache album art for playerctl module #179

Open Apeiros-46B opened 2 years ago

Apeiros-46B commented 2 years ago

Summary

This PR

  1. Replaces the usage of os.tmpname() with a URL-based path in the playerctl module and prevents album art from being downloaded multiple times, and
    • The path is determined based on the following logic: all occurences of https:// and http:// are first removed from the URL, then /tmp/bling_album_art .. trimmed_url is used as the path
  2. Adds new params for save_image_async_curl function in helpers/filesystem.lua (required for 1.):
    • boolean redownload: if this is false and the file already exists, curl isn't called
    • boolean create_dirs: if this is true the --create-dirs flag is added to the curl call

This means that

  1. Disk space is saved by not unnecessarily re-downloading album art
  2. Already downloaded album art will work when you don't have an internet connection
  3. For people with slow internet speeds, getting the album art should be faster if it isn't the first time

Notes

Haven't tested if this works when passing some other values for redownload and create_dirs to the save image function, but it theoretically should still work just fine

Apeiros-46B commented 2 years ago

Another thing I wanted to point out: on some systems the /tmp dir is cleared on boot; maybe it would be better to store album art in ~/.cache instead? Or make the dir configurable

nawuko commented 2 years ago

Consider that some clients cache there own artwork and generate a temporary filename, with this implementation every time you play a youtube video (even if its the same video!) for example from firefox a new file is created, which, with this solution, will never be deleted. I think a task to clean old files is outside of the scope, but this new behavior should be considered with this change

Edit: Since those files are local (they begin with a file:// uri) maybe they could be filtered and set directly cause there is no need to run curl for them anyways. (And according to freedesktop spec this should be always the case - "URIs should be sent as (UTF-8) strings. Local files should use the "file://" schema.")

Example metadata from firefox:

{
  ["mpris:artUrl"] = "file:///home/nawuko/.mozilla/firefox/firefox-mpris/4688_6.png",
  ["xesam:album"] = "",
  ["xesam:artist"] = { "Practical Engineering" },
  ["xesam:title"] = "What Happens When a Reservoir Goes Dry?"
}
Nooo37 commented 2 years ago

Don't know much about the playerctl module. @Kasper24 is the contact person for that. Nawuko seems to have valid concerns too

Apeiros-46B commented 2 years ago

Would checking if the art url starts with file:// and then use that path instead of downloading it work for this? Or are there also other clients that don't report the file path correctly

javacafe01 commented 2 years ago

@Kasper24

Kasper24 commented 1 year ago

@JavaCafe01 The issue with storing it in cache is it won't get deleted, that's why I saved it to tmp instead. It also uses a blocking function to query if the file exists or not, so that would need to be converted to async. Another issue I saw mentioned in the discord is that when no network connection is available, trying to download the artwork will cause the metadata signal to not be emitted, so it might be worth to split them into 2 signals ("metadata" and "artwork") while we're at it