Allow editing an existing lyric

[omar-polo]

From time to time, I'd like to tweak the lyrics fetched from versuri. Maybe it's just a wrong line break, or a typo, or a strange quoting, something like that.

This PR adds a versuri-lyric-edit command that pops a buffer in text-mode where the user can edit the existing buffer. Even better: after save the lyric buffer gets automatically updated :D

It's still a draft, I just noticed that the temporary file used to store the lyric remains and clutters the /tmp.

I'd also like to add a versuri-add-lyric command to manually insert a lyric for those cases where versuri fails to fetch one. (use case: I don't seem to be able to fetch the lyric for "It's the end of the world as we know it (and I feel fine)", but still like to be able to read it using versuri)

Cheers

[omar-polo]

I've also added versuri-lyric-edit-meta to change the author and song title, and did a bit of refactoring of my versuri-lyric-edit-meta function to do directly the update in the db instead of adding/removing the row.

Some of this commits could be squashed together if you prefer.

Cheers :)

P.S.: this time I haven't forgot to update the readme :P

P.P.S.: I noticed that the readme don't seem to document the interactive functions, so I haven't documented the lyrics-edit and edit-meta functions, or should I? (no problem, just wondering)

[mihaiolteanu]

Awesome!
I haven't thought about this feature. Looks like a nice addition!

Wouldn't it be easier (or more intuitive) to change the lyrics mode to a non-read-only mode?
That way, you can edit the db lyrics directly from the lyrics buffer. True, you would have to change the keybindings (from r to C-r, for example).
I would imagine editing the first line would mean editing the song artist and song name. Currently, - is used to split the two visually, but we can use whatever we like (then you can parse the first line using this char to split the string into artist and song) One question would need to be answering. If the user edits the artist and/or song that already exists in the db, what happens? Add a new entry in the db or modify/replace the existing one? Or allow the user to choose between the two?

If you agree, then maybe you would just need the new interactive function versuri-add-lyric that would just open a normal, but blank lyrics buffer where the user can add whatever. Maybe add placeholders for where the artist, song name and lyrics would go in the new buffer,

[artist] - [song]

[lyrics]

[mihaiolteanu]

I noticed that the readme don't seem to document the interactive functions

I might have not noticed some of them?!
Of course, they should be there.

[omar-polo]

Mihai Olteanu ***@***.***> writes:

Awesome! I haven't thought about this feature. Looks like a nice addition!

thanks :)

Wouldn't it be easier (or more intuitive) to change the lyrics mode to a non-read-only mode? That way, you can edit the db lyrics directly from the lyrics buffer. True, you would have to change the keybindings (from r to C-r, for example).

the rationale was to disallow accidental changes to the lyrics. For example, I currently have some binding like `n` to forward-line and `p` to previous line in versuri-mode. True, we could just enable view-mode instead of read-only-mode and disable that when the user wants to edit the lyric.

I would imagine editing the first line would mean editing the song artist and song name. Currently, - is used to spit the two visually, but we can use whatever we like (then you can parse the first line using this char to split the string into artist and song) One question would need to be answering. If the user edits the artist and/or song that already exists in the db, what happens? Add a new entry in the db or modify/replace the existing one? Or allow the user to choose between the two?

This was my first thought, but then I thought that there's no reason the "-" (or even " - ") can't appear on the artist name or song title, making the recognition more difficult. The edit-meta command was to avoid this possible ambiguity. (it's a quite extreme case and to be fair I can't recall from the top of my head an artist with a "-" in the name.) This also weighted in favour of the decision to pop-up a new buffer.

If you agree, then maybe you would just need the new interactive function versuri-add-lyric that would just open a normal, but blank lyrics buffer where the user can add whatever. Maybe add placeholders for where the artist, song name and lyrics would go in the new buffer, [artist] - [song] [lyrics]

I don't have any strong opinion in this, I just forgot to mention those points in the OP, apologies. I can just easily change the code to allow editing the original buffer, and for the user to avoid accidental edits is just as easy as: (add-hook 'versuri-mode #'view-mode) which we may even make the default or mention in the documentation.

[mihaiolteanu]

the rationale was to disallow accidental changes to the lyrics

I understand your point.
First, the user would, somehow, have to not see his accidental changes.
Second, he would have to not know how to undo the buffer.
Third, he would actually have to C-x C-s to change the db, as well. Otherwise, he can answer no when closing an edited buffer.
Worst case, he can re-fetch the lyrics completely new from the web, so nothing is lost for good.

I currently have some binding like n to forward-line and p to previous line in versuri-mode.

Yes, I have that too.
First, how often do you need to move through the lyrics buffer.
Second, with a non-read-only buffer you will have to fall-back to C-n and C-p like in a regular buffer. Would that be (much) worse that simply using n and p.

there's no reason the "-" (or even " - ") can't appear on the artist name or song title

Correct! That is a real concern. If we don't know any artist with - in its name, it doesn't mean it doesn't exist. But, we're the presidents here, we can choose whatever we like :)) |, maybe. Or split the line in two,

[artist]
[song]

[lyrics]

which we may even make the default or mention in the documentation

It looks to me like the Emacs/Lisp/Unix way is to allow the user to do whatever he likes. You can even change the car to point to whatever you like, for example. It is not useful, but nobody is stopping you if you have some crazy ideas. So I would favor this approach. And yes, updating the documentation to let the user know what happens when he opens a lyrics buffer, that he can edit it, that he can modify it and if he doesn't like that he can disable it, is an excellent idea!

I'm not 100% convinced of anything yet. Just floating ideas around. :)

[omar-polo]

Mihai Olteanu ***@***.***> writes:

the rationale was to disallow accidental changes to the lyrics I understand your point. First, the user would, somehow, have to not see his accidental changes. Second, he would have to not know how to undo the buffer. Third, he would actually have to C-x C-s to change the db, as well. Otherwise, he can answer no when closing an edited buffer. Worst case, he can re-fetch the lyrics completely new from the web, so nothing is lost for good.

I completely agree, I was over-thinking the interaction.

I currently have some binding like n to forward-line and p to previous line in versuri-mode. Yes, I have that too. First, how often do you need to move through the lyrics buffer. Second, with a non-read-only buffer you will have to fall-back to C-n and C-p like in a regular buffer. Would that be (much) worse that simply using n and p. there's no reason the "-" (or even " - ") can't appear on the artist name or song title Correct! That is a real concern. If we don't know any artist with - in its name, it doesn't mean it doesn't exist. But, we're the presidents here, we can choose whatever we like :)) |, maybe. Or split the line in two, [artist] [song] [lyrics]

Two lines may be the best solution, only I'd put the song as first line and the artist as second. We could introduce different faces for the artist and the song title too!

which we may even make the default or mention in the documentation It looks to me like the Emacs/Lisp/Unix way is to allow the user to do whatever he likes. You can even change the car to point to whatever you like, for example. It is not useful, but nobody is stopping you if you have some crazy ideas. So I would favor this approach. And yes, updating the documentation to let the user know what happens when he opens a lyrics buffer, that he can edit it, that he can modify it and if he doesn't like that he can disable it, is an excellent idea! I'm not 100% convinced of anything yet. Just floating ideas around. :)

I really like your take on the idea. Using two lines for the title/artist and an editable buffer from the start seems to most powerful way, yet easy to implement and use/understand, to add this feature. I've only one doubt in the technical side and that is if we allow the title to be edited in the lyrics buffer how do we to ensure that the faces are not messed up. (i.e. the user copies something from the text into the title and now it has the normal face instead of the title one, or vice versa.) But we can solve this later when we have a demo of the new lyrics buffer. Thanks! (it'll take me a while to find the time to rework the patch thought, sorry)