search

jcorporation / myMPD

myMPD is a standalone and mobile friendly web mpd client with a tiny footprint and advanced features.
https://jcorporation.github.io/myMPD/
GNU General Public License v3.0
414 stars 65 forks source link

ListenBrainz Feedback trigger script errors with 10.0.0 #846

Closed aereaux closed 2 years ago

aereaux commented 2 years ago

myMPD version: [e.g. 8.1.0]

Describe the bug

With the upgrade to v10.0.0, my Listenbrainz feedback script now errors with:

Error executing script LB Feedback: Runtime error: /var/lib/mympd/scripts/LB Feedback.lua:7: attempt to sub a 'string' with a 'number'

It looks like the "vote" argument is not getting populated.

To Reproduce

Steps to reproduce the behavior:

  1. Install the Listenbrainz Feeback trigger and script as normal
  2. Click on the thumbs up button
  3. See error

Expected behavior

Script completes successfully and result appears in listenbrainz

Screenshots

Server plattform (please complete the following information):

Client plattform (please complete the following information):

Debug logs (please attach if it can be usefull)

Can attach these if needed.

Configuration (please attach if it can be usefull)

Can attach these if needed

Additional context

I looked at the code a bit, and it seems like the problem was probably caused with this change. It looks like it now ignores the script_arguments t_list, instead passing in &trigger_data->arguments, which probably doesn't include the vote (or uri). I was trying to understand the code here in order to write up a patch, but it's been a while since I've written a lot of C. Does some merging need to go on between the two lists, or can the script_arguments list just be passed in as is (like it was before)?

jcorporation commented 2 years ago

Thanks for reporting and analyzing this bug.

Does some merging need to go on between the two lists, or can the script_arguments list just be passed in as is (like it was before)?

Just like before.

jcorporation commented 2 years ago

Bug should be fixed with above commit, can you test?

Edit: there was an another bug in the script itself: replace MYMPD_API_DATABASE_SONGDETAILS with MYMPD_API_SONG_DETAILS

aereaux commented 2 years ago

Yeah, I noticed the bug in the script and fixed it earlier.

Great thanks, it works now with this change!