-
Notifications
You must be signed in to change notification settings - Fork 6
Improve metadata support #13
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
Conversation
DamienCassou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the awesome work! I really appreciate the effort. It feels like you really understand the architecture of mpdel.
Unfortunately, the version of libmpdel in this PR doesn't work with mpdel as it is today. At least for me. For example, when selecting an artist, I would expect to see its albums but I get an error now:
Wrong type argument: libmpdel-artist, "2010"
I'm using mpd 0.24.3.
I think it would be easier for me to get everything merged if your work was split into several independent PRs. I would then be able to review, test and merge each one separately.
Can you please make sure that linters are satisfied and that unit tests pass?
| (cl-defmethod libmpdel-list ((_entity (eql albums)) function) | ||
| "Call FUNCTION with all albums as parameter." | ||
| (libmpdel-send-command | ||
| ;; TODO: compute all albums by listing all songs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the purpose of this TODO? Are you planning to address it before we get the PR merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a tradeoff that still needs to be addressed: you cannot get a correct list of all albums along with the associated dates and albumartists using list album (it might be tempting to do something along the lines of list album group albumartist group date, but that duplicates albums with several albumartists).
As far as I know, if you want a list of all albums with metadata, you'd need to list all the songs in the entire database using find and filter them into albums, analogously to what libmpdel-list ((artist libmpdel-artist) function) does in this PR.
I think that this tradeoff is worth it for artists, since listing all the songs of a particular artist should be an acceptable computational expense - but listing and processing all the songs in the mpd database may however very well be prohibitively computationally expensive.
So the tradeoff is:
- either
mpdel-core-open-albumslists the albums without artist and date metadata, effectively merging any albums with the same name (this is the current situation in this PR), - or running
mpdel-core-open-albumsis quite expensive if you have a large MPD library
| (album nil :read-only t) | ||
| (genres nil :read-only t) | ||
| (performers nil :read-only t) | ||
| (artists nil :read-only t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is the value of "artists" different from the album's artists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MPD distinguishes between "AlbumArtist" and "Artist", and this reflects this distinction in libmpdel. Typical real world examples of when these differ are:
- artists that feature on an individual track of an album (these will not be in the AlbumArtist but will be in the Artist field), or
- compilation albums (in this case, the AlbumArtist is typically the compiler), or
- split albums (here, all tracks typically list both AlbumArtists but only one Artist)
|
I've updated the |
Thank you for putting mpdel together, for the kind words, and for the review :)
Huh, that is weird - it works with mpdel 2.1.0 and mpd 0.23.17 on my machine (though I've not tested the commits individually, only the end result). Could this be something to do with load/evaluation/compilation order, or "old" structs with fewer fields hanging around after a new libmpdel was loaded?
The commits build on each other, so they have to be merged in order and thus it makes more sense to me to have it all as a single PR - if you'd like, I can nonetheless create a PR per commit
I rebased, purcell's linter no longer finds any issues and all tests in |
most probably. I will try again.
yesterday I merged a PR and all tests were passing: https://github.com/mpdel/libmpdel/actions/runs/15376761289/job/43262403226
The first 3 commits look independent to me. I understand this is more work for you but decomposing into different PRs helps me ensure quality. Could you please open 3 PRs for the first 3 commits if you confirm they are independent? When they are merged, you will then be able to open the final PR with the last commits. Feel free to decompose differently if that makes more sense to you. |
I opened a fake PR to double-check and I can confirm tests pass there: #15. |
|
Can we close this PR and focus on smaller ones? |
This PR distinguishes between song artists and album artists, and adds support for
It should be backward compatible provided that no
--functions are usedI'll soon follow up with a PR to mpdel proper to reflect these changes in the UI