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.

Sign up for GitHub

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

Jump to bottom

command/file: add command "readlyrics" #653

Closed
wants to merge 2 commits into from
Closed

command/file: add command "readlyrics" #653

wants to merge 2 commits into from

Conversation

NBonaparte
Copy link

Implements unsynced lyrics (ID3 "USLT") as a tag.

Not sure if it should be disabled by default (like TAG_COMMENT); any feedback/opinions would be appreciated.

@MaxKellermann
Copy link
Member

How does your change deal with multi-line tag values? Lyrics tags are usually multi-line, aren't they?

@prat0088
Copy link

What's needed before this can be merged?

@NBonaparte
Copy link
Author

NBonaparte commented Nov 25, 2019 •
edited
Loading

@prat0088 This version doesn't really work with tags with newlines.

I reimplemented lyrics support as a readlyrics command (similar to readpicture, etc.) which should properly handle newlines nearly two months ago (AFAIR) but forgot to push to this branch.

@NBonaparte NBonaparte changed the title tag/Type: add TAG_LYRICS command/file: add command "readlyrics" Nov 25, 2019
@jcorporation jcorporation mentioned this pull request Nov 27, 2019
Closed
3 tasks
@MaxKellermann
Copy link
Member

You added this as a regular MPD tag. This will still break the protocol, because now we have a regular tag with newlines.

@NBonaparte
Copy link
Author

I'm unable to reproduce this. Without the lyrics tag in the metadata_to_use setting, the lyrics tag cannot be queried (obviously) and the readlyrics command works as expected. With metadata_to_use = +lyrics, the lyrics show up as tags, but with newlines stripped so everything is in one line.

@MaxKellermann
Copy link
Member

With a long enough line, libmpdclient will close the connection.
And it doesn't even make sense to have lyrics as a regular MPD tag. Just like the album cover wasn't implemented as a regular tag. Look at the album cover code.

MaxKellermann
MaxKellermann requested changes Dec 17, 2019
View reviewed changes
NEWS
@@ -1947,7 +1948,7 @@ ver 0.12.0 (2006/9/22)
* New --kill argument for killing MPD if pid_file is specified
* Removed --update-db argument (use the update function in your client instead)
* New mpdconf.example
* New mpd.conf man page
* New mpd.conf man page
Copy link
Member

Choose a reason for hiding this comment

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

Whitespace change?

NEWS
@@ -1988,7 +1989,7 @@ the write() becomes successful)
7) If "user" is specified, then convert ~ in paths to the user's home path
specified by "user" config paramter (not the actual current user running mpd).

ver 0.11.2 (2004/7/5)
ver 0.11.2 (2004/7/5)
Copy link
Member

Choose a reason for hiding this comment

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

Whitespace change?

NEWS
@@ -2162,7 +2163,7 @@ non existing file. This causes parsing errors for clients.
ver 0.8.4 (2003/8/13)
1) Fix a bug where garbage is returned with errors in "list" command

ver 0.8.3 (2003/8/12)
ver 0.8.3 (2003/8/12)
Copy link
Member

Choose a reason for hiding this comment

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

Whitespace change?

doc/protocol.rst
:command:`readlyrics {URI}`
Read lyrics from the file specified by "URI". This "URI"
can be a path relative to the music directory or an
absolute path.
Copy link
Member

Choose a reason for hiding this comment

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

How does the response look like?

doc/protocol.rst
@@ -1275,7 +1280,7 @@ Audio output devices

:command:`outputs`
Shows information about all outputs.

Copy link
Member

Choose a reason for hiding this comment

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

Whitespace change?

src/command/FileCommands.cxx

void OnLyrics(StringView value) noexcept override {
response.Format("%.*s\n",
int(value.size), value.data);
Copy link
Member

Choose a reason for hiding this comment

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

So this writes the lyrics right into the stream, raw. How does the client know when the lyrics are finished?
What if there are multiple lyrics tags?

@jcorporation jcorporation mentioned this pull request Feb 12, 2020
Closed
4 tasks
@MaxKellermann
Copy link
Member

Let's reopen this as soon as you have solved the fundamental problems.

@ddonindia
Copy link

waiting for this for long time :(

@jcorporation
Copy link
Member

jcorporation commented Apr 19, 2020 •
edited
Loading

How does the client know when the lyrics are finished?

One possibility could be to print the size of lyrics as a pair before the lyrics. The client can then use the new mpd_recv_raw function to get the lyrics.

@MaxKellermann MaxKellermann mentioned this pull request Sep 30, 2020
Open
@SoongNoonien SoongNoonien mentioned this pull request Apr 7, 2021
Open
@mxnemu
Copy link

mxnemu commented Jul 31, 2023

I'm going to try pick this back up, and will probably make a new Merge Request this weekend.

I changed it to have the same command API and binary response as readpicture / albumart and ignore the second lyrics tag if there is more than one, like readpicture does.

I just need to rebase this and clean up the commit first. WIP branch: https://github.com/mxnemu/MPD/tree/unsynced-lyrics

@mxnemu mxnemu mentioned this pull request Aug 5, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxKellermann MaxKellermann MaxKellermann requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@NBonaparte @MaxKellermann @prat0088 @ddonindia @jcorporation @mxnemu