volumio / Volumio2

Volumio 2 - Audiophile Music Player
http://volumio.org
Other
1.37k stars 317 forks source link

Webradio station names are lost when saving to playlist V-2.348 #1476

Open Cuby2 opened 6 years ago

Cuby2 commented 6 years ago

When the queue contains webradios and they are saved to a playlist, the station name is not saved. Thus, on reloading the playlist, the queue shows "undefined" instead of names.

/data/queue in original state

{ "uri": "https://vintageradio.ice.infomaniak.ch/vintageradio-high.mp3", "title": "Vintage Radio", "service": "webradio", "name": "Vintage Radio", "albumart": "/albumart", "samplerate": "", "bitdepth": "", "channels": 0, "trackType": "webradio" },

Saved the queue as playlist "RADIO" and the field "name": "Vintage Radio", gets lost. /data/playlist/RADIO

{ "service": "webradio", "uri": "https://vintageradio.ice.infomaniak.ch/vintageradio-high.mp3", "title": "Vintage Radio", "albumart": "/albumart" },

Cuby2 commented 6 years ago

Closed just by mistake

Cuby2 commented 6 years ago

Corrected RADIO-record

Cuby2 commented 6 years ago

Actually, the webradio-names (and albumart, where they exist) get lost, when the playlist is loaded back. The bug is still there in V.2.389 I would like to suggest the following solution:

Patch 3 lines in /volumio/app/playlistManager.js I have not experienced any negative side effects.

For V.2.389 change the following lines:

650 original: var service; patched: var service; var title; var albumart;

654 original: else service=data[i].service; patched: else {service=data[i].service; title=data[i].title; albumart=data[i].albumart;}

656 original: uris.push({uri:uri,service:service}); patched: uris.push({uri:uri,service:service, title:title, albumart:albumart});

xipmix commented 6 years ago

I'd like to test this out. Can you post a proper diff here? It makes patching much faster and more accurate.

Just in case you need a little help -

Even better would be to set this up as a pull request, but that's a bit more learning curve. In case you want to try, these pages are helpful

Cuby2 commented 6 years ago

patch.txt

xipmix commented 6 years ago

I can confirm the behaviour that the name field gets lost when the playlist is saved.

Also when I go to the playlist file and 'add to queue', the /data/queue file lacks the name and title fields (while the playlist file contains them). The albumart fields seems to be populated however.

xipmix commented 6 years ago

I applied the patch and restarted volumio (vrestart, reloaded browser) but it didn't seem to make any difference. I even tried adding the name field to the object pushed onto the uris array.

I may have tested with an older image, I will flash a fresh 2.389 and try again later.

xipmix commented 6 years ago

Tried again with a fresh image, not seeing any behaviour change.

Cuby2 commented 6 years ago

Update

The previously suggested fix is incomplete. Sorry for that. "Clear and play" works, "Add to queue" does not.

Therefore I post this updated procedure which adds only one more line.

Patch 3 lines in /volumio/app/playlistManager.js and insert 1 additional line. For V.2.389 change the following lines:

650 original: var service; patched: var service; var title; var albumart;

654 original: else service=data[i].service; patched: else {service=data[i].service; title=data[i].title; albumart=data[i].albumart;}

656 original: uris.push({uri:uri,service:service}); patched: uris.push({uri:uri,service:service, title:title, albumart:albumart});

Finally: 176 contains: name: data[i].title, after this line insert: title: data[i].title,

A git-diff is attached. patch_v2.txt

xipmix commented 6 years ago

Ah that's better, it fixes the problem.

But I have a question - when I add a debug print statement to the PlaylistManager.prototype.commonPlayPlaylist function, I never see it being called, which makes me wonder about the second group of changes, lines 650-656. How did you determine those were needed?

xipmix commented 6 years ago

Nevermind - I see the calls now. Not sure what I was doing wrong before.

diehardsk commented 6 years ago

Hi guys, I was working on the very same thing on yesterday, changed code in similiar style as Cuby2 and made a pull request #1526. I refrenced it to issue #1270, because I found that one at that moment. It's the same issue.

xipmix commented 6 years ago

Cool, thanks for that. One thing I have been wondering about is the name vs the title field. Why is name even there and why is it so often left out in most playlistManager.js functions?

Cuby2 commented 6 years ago

It seems to me as if a single information item appears under 2 variable names.

When a song is loaded, the title appears in /data/queue as . When a webradio is loaded, the station-name in/data/queue is also . (plus a identical copy goes under , which is redundant)

When the queue is saved to a playlist, is saved as . When the playlist is loaded back into the queue becomes again. (At least with the fix) So I wonder, why not using a single variable name.

Using 2 names seems to be error prone. In case of "Add to queue" for a webradio-playlist the following happens: playlistManager.js loads the record(s) from file and copies to <name>, dropping <title>. The record is passed to playqueue.js playqueue.js then also attemts to copy <title> to <name>, but there is no <title> anymore and <name> becomes "undefined".</p> <p>That said - I am no programmer at all and my skills are pretty much limited. So there may be good reasons for the approach as is.</p> </div> </div> <div class="page-bar-simple"> </div> <div class="footer"> <ul class="body"> <li>© <script> document.write(new Date().getFullYear()) </script> Githubissues.</li> <li>Githubissues is a development platform for aggregating issues.</li> </ul> </div> <script src="https://cdn.jsdelivr.net/npm/jquery@3.5.1/dist/jquery.min.js"></script> <script src="/githubissues/assets/js.js"></script> <script src="/githubissues/assets/markdown.js"></script> <script src="https://cdn.jsdelivr.net/gh/highlightjs/cdn-release@11.4.0/build/highlight.min.js"></script> <script src="https://cdn.jsdelivr.net/gh/highlightjs/cdn-release@11.4.0/build/languages/go.min.js"></script> <script> hljs.highlightAll(); </script> </body> </html>