Add support for multiple tags for multiple values (fixes #44)

[splintersuidman]

This PR fixes #44.

Description

MPD supports multiple tags for some values, such as the artist, the composer, and the performer. (See https://mpd.readthedocs.io/en/latest/protocol.html#tags.) There might be multiple artists for a track, in which case mopidy-mpd would concatenate their names into one value, separated by a semicolon. For some users and use cases, this can be suboptimal; when tracking the tracks you listen to on a platform such as Listenbrainz, tracks of multiple artists do not contribute to the total number of listens of one of the artists.

This pull request changes this behaviour when the multiple_tags configuration variable is set to true, to use multiple tags for values when applicable instead of concatenating the values.

Design choices

• I chose multiple_tags for the configuration variable that toggles the implemented behaviour. I don’t think the name is very clear, but I can’t think of a good name.
• I added the value of the multiple_tags configuration variable as an attribute to the MpdContext class. I don’t whether there is a better place to store this value to make it available to the commands.
• I implemented the change for the artist, albumartist, composer and performer tags.

[djmattyg007]

Please update the configuration documentation in the readme to mention the new setting.

[djmattyg007]

Everything looks good 👍 I'm going to leave it open for a couple of days to give others a chance to review, but I don't see any problems with it at this stage.

[djmattyg007]

Actually, would you be able to squash your branch down to a single commit before we merge it? The many fixup! things in all the messages are probably unnecessary.

[splintersuidman]

Actually, would you be able to squash your branch down to a single commit before we merge it? The many fixup! things in all the messages are probably unnecessary.

Done! :)

[girst]

maybe like this? (field_name, field_value) = result set(field_value.split(";")) == {"an artist", "yet another artist"}

[kingosticks]

Sorry if I missed this discussion already but if mpd always(?) splits the artists into separate tags, why does Mopidy need to have it configurable? Is there a specific case where we must continue supporting the old non-conforming behaviour?

I'm asking in the spirit of avoiding unnecessary user config options and changes to lots of places just so we can pass the switch through.

[splintersuidman]

@kingosticks

Sorry if I missed this discussion already but if mpd always(?) splits the artists into separate tags, why does Mopidy need to have it configurable? Is there a specific case where we must continue supporting the old non-conforming behaviour?

See this reply on the issue: #44 (comment). There could be MPD clients that do not support multiple artist tags, for example. It could also be that some users might prefer the concatenation behaviour.

However, the MPD documentation states that ‘there can be multiple values for some of these tags.’ I don’t know whether there exists an exhaustive list of tags for which there can be multiple values, but if there is and artist, albumartist, composer and performer are elements of that list, I think it would be fair to say that not handling multiple values is the fault of the MPD client since it doesn’t support the full MPD protocol.

I think it is more safe to make this behaviour configurable, but I personally don’t (yet) see any reason to keep using the old behaviour.

[djmattyg007]

I suggested the behaviour should be configurable as a “just in case”. I’m not very familiar with MPD so wasn’t sure how big of a change it may be considered to be. I appreciate now that the decision to have a flag has significantly increased the magnitude of changes required.

[kingosticks]

Personally I'd not have it configurable, clients should support this.

[splintersuidman]

I also think it should be fine to not make it configurable, given there has not yet been an objection by anyone who depends on the old behaviour. (Of course not every user is actively reading the issues, so we cannot be sure this is fine for everyone.) Furthermore, this behaviour should be supported by MPD clients. (That does not mean in that it interacts well with every user’s scrobbling setup and the like.)

Shall I change this PR to remove the configuration option?

[djmattyg007]

I think it makes sense to proceed with removing the configuration option, yeah. Please accept my apologies for creating so much unnecessary work.

[splintersuidman]

I have now removed the configuration option. I have kept a back-up branch where the option is configurable here.

Please accept my apologies for creating so much unnecessary work.

No problem!

[djmattyg007]

@splintah Could you please try pushing your branch again? For some reason we don't appear to be able to approve running the CI workflow for this PR.

[jodal]

This can be inlined, so we keep the same order of the out as before:

    result = [
        ("file", track.uri),
        ("Time", track.length and (track.length // 1000) or 0),
        *multi_tag_list(track.artists, "name", "Artist"),
        ("Album", track.album and track.album.name or ""),
    ]

Example illustrating how this works:

>>> [1, 2, *[3, 4, 5], 6, 7]
[1, 2, 3, 4, 5, 6, 7]

[jodal]

I'd maybe rename models to objects. I'm thinking of "models" as the types themselves, while the instances of the model types are simply instances or objects.

[jodal]

@splintah Could you please try pushing your branch again? For some reason we don't appear to be able to approve running the CI workflow for this PR.

I reviewed this and pushed fixes to my own comments to the branch to have something to force a build. I'm 👍 to merging this!

Thanks for your contribution, @splintah!

[jodal]

This has now been released to PyPI as part of Mopidy-MPD v3.2.0.