Bugzilla – Bug 17199
Add documented version of 'jiveplaytrackalbum" command
Last modified: 2011-12-22 01:22:05 UTC
The CLI is missing a command to play an album from a certain track on. In iPeng I have to mimick this by using a sequence of commands (clear playlist, add album, jump to index) but this can lead to issues with synchronization in some environments. SqueezePlay uses the "jiveplaytrackalbum" command which does exactly what would be needed but is not documented so it can change at an time. So my suggestion would be to add an "official" command plus documentation, maybe called "playtrackalbum" No functionality would need to be developed since "jiveplaytrackalbum" is already there and does the job.
Ben - can we consider this command "official" enough?
Not quite official enough commands that start with jive* are targetted for SP only, so what I'll need to do is: move this from Slim::Control::Jive to Slim::Control::Commands rename the command playtrackalbum create an aliased command jiveplaytrackalbum to the same sub for backwards compatibility document it in the CLI docs shouldn't be hard, I'll take this for 7.6
Thanks!
any objections if I call this cli command playlist playtrackalbum rather than just playtrackalbum ? I'm trying to be consistent with what is already in the CLI, and this is really a playlist manipulation command, which follows this paradigm. FYI, in working on this I also found jiveplaytrackplaylist, which I'm going to also pull out of SP-specific undocumented code space and make "official" so, two new and documented commands to be completed playlist playtrackalbum playlist playtrackplaylist jiveplaytrackalbum will do the same thing as playlist playtrackalbum for the unlikely need of backward compatibility. same goes for jiveplaytrackplaylist and playlist playtrackplaylist.
FYI, I'm going to go ahead with the command naming style mentioned in the previous comment. Checkin coming shortly.
== Auto-comment from SVN commit #32394 to the slim repo by bklaas == == http://svn.slimdevices.com/slim?view=revision&revision=32394 == Fixed Bug: 17199 Description: add cli commands 'playlist playtrackalbum' and 'playlist playtrackplaylist' add sections for both commands to cli-api documentation previously undocumented commands jiveplaytrackalbum and jiveplaytrackplaylist now alias to the above commands change Queries.pm calls to old jive* commands to new "official" commands remove code logic from playtrackplaylist and instead pass on request to playtrackalbum, which already performed the identical command when given a playlist_id
Thanks. playlist playtrackalbum is perfectly fine for me.
I worked through a merge conflict today bringing this change up to the onebrowser branch and discovered that Alan has already effectively replaced any need for these two commands with the playlistcontrol cli command. (my fault for not having better knowledge of onebrowser changes) For example, to start playlist 1231 from the 4th track, you issue this cli command { "playlistcontrol", "cmd:load", "menu:1", "playlist_id:1231", "play_index:3", "useContextMenu:1", } or to start album 62 from the 2nd track, you issue this cli command { "playlistcontrol", "cmd:load", "album_id:62", "menu:1", "play_index:1", "sort:albumtrack", "useContextMenu:1", } After reviewing this and confirming that onebrowser has no need for `playlist playtrackalbum` and `playlist playtrackplaylist`, my feeling is that I should strip these new commands out of the onebrowser branch entirely, but leave them in 7.6/trunk. The issue with that until 7.6 release is that trunk and onebrowser will have different commands to perform these tasks. Sorry, I realize that kind of sucks, but until onebrowser becomes trunk, that's probably the way it should be.
Well, since I know it in advance I think I can handle it... But... Is there documentation for it? I hope I don't have to use the menu stuff, right? And one more thing: does it work with music folders?
But... Is there documentation for it? yes, it's documented in the cli-api doc, on the onebrowser branch I hope I don't have to use the menu stuff, right? that's correct And one more thing: does it work with music folders? it appears not, but I'm assuming that's a bug to be fixed (?) documentation for the play_index param reads: play_index If this parameter is provided along with "cmd:load" then playback will start with the indicated track. Does not work with a "folder_id" parameter.
OK, music folders would definitely need fixing, that's actually where I need this most, do I need to open a separate bug?
Ben, after looking into this again I find this to not be a good solution, would it be possible to alias the "playlistcontrol cmd:load play_index:x" command to the "playlist playtrackalbum" command? The reason is, that you can't test for the availability of the playlistcontrol command since it's always available. What I do in iPeng is to use the "can ..." command to test for the availability of a command instead of making assumptions based on the version and I think this is the right way to go. Doesn't work in this case, though.
Alan, speaking specifically to the fact that playlistcontrol cmd:load doesn't support folder_id together with play_index. I made this very simple patch and now the command works for me. I must be missing something though, surely it couldn't be this easy? === Commands.pm ================================================================== --- Commands.pm (revision 50272) +++ Commands.pm (local) @@ -1895,6 +1895,11 @@ Slim::Control::Request::executeRequest( $client, ['playlist', $cmd, $folder->url()] ); + # jump to the correct index + if ($jumpIndex) { + $client->execute( ["playlist", "jump", $jumpIndex] ); + } + $request->addResult('count', 1); $request->setStatusDone(); return;
Ben, you are right, you are missing something. The command now plays the folder and all subfolder content which is not what the jiveplaytrackalbum command did and which is not what it should do. BTW, a command to ADD all content of a folder (without subfolders) would also be nice...
Hm, not 100% sure I'm seeing the right thing. Is this in trunk or onebrowser or am I getting something wrong? For me, the play_index parameter does nothing with a folder_id, the folder always plays from track one (both in trunk and onebrowser). Plus the subfolder issue I mentioned.
> The command now plays the folder and all subfolder content okay, that's confirmed, athough it has nothing to do with the play_index flag. On onebrowser branch, the playlistcontrol command is broken for this BMF use case (playing all tracks within a folder without playing all of the subfolder's tracks). Today I'm going to see about pulling code from what I did for BMF support in playlist playtrackalbum (trunk) and apply it to playlistcontrol (onebrowser) Joerg, just to be clear, the conclusion Alan and I came to with regards to supporting this command in onebrowser was that keeping `playlist playtrackalbum` and `playlist playtrackplaylist` around, even as an alias, when the new playlistcontrol command should provide that function, was bad practice. One of the important goals of onebrowser is code reduction to improve maintainability, and aliasing commands goes against that goal. What this means is that if I'm successful in getting Browse Music Folder to work as desired with the playlistcontrol command, it will break your ability to use the "can.." command. For this command in onebrowser, you'll have to do a server version check. Just to make things a little more complicated, it's important to note that onebrowser is still not merged to 7.6/trunk. Until that happens IMHO it's premature to say that it's a foregone conclusion that it will be. If onebrowser never folds into trunk, that will mean that we're back to the `playlist playtrackablum` and `playlist playtrackplaylist` commands, which I consider feature complete on that branch, including for Browse Music Folder.
OK, do you really think it's better practise to have no ability to determine the capability of the API compared to keeping aliases? OK, can I at least assume that if a build is version 7.6 or newer it will either support playlist playtrackalbum OR playlistcontrol with play_index? or should I just use jiveplaytrackalbum which is not documented but at least it's there...
Oh, btw, to avoid confusion: I'm aware that the fact that cmd:load on playlistcontrol loads all subfolder has nothing to do with the new behavior, it's always been that way, some API users might even EXPECT that behavior! But that doesn't change the fact that play_index with a folder_id currently does nothing.
== Auto-comment from SVN commit #32425 to the slim repo by bklaas == == http://svn.slimdevices.com/slim?view=revision&revision=32425 == Fixed Bug: 17199 Description: onebrowser specific changes for supporting Browse Music Folder and play_index The following commands are completely removed from onebrowser branch: playlist playtrackalbum playlist playtrackplaylist jiveplaytrackalbum jiveplaytrackplaylist All of the features in these commands should be available via the playlistcontrol command now. For example, this playlist playtrackalbum folder_id:1247 list_index:3 or this jiveplaytrackalbum folder_id:1247 list_index:3 is now playlistcontrol cmd:load folder_id:1247 play_index:3 this playlist playtrackplaylist playlist_id:123 list_index:6 or this jiveplaytrackplaylist playlist_id:123 list:index:6 is now supported by playlistcontrol cmd:load playlist_id:123 play_index:6
one additional point to make-- after discussing 7.6/branches/onebrowser vs. 7.6/trunk today with Felix and Michael, I am now of the opinion that it would be very surprising if onebrowser was not the official 7.6.0 release of SBS. * onebrowser is what PQA has been testing with for 7.6 release * onebrowser fixes a lot of existing bugs, which would all have to reopen if we didn't * onebrowser represents a significant reduction in code complexity on the server side So, I think it's reasonable to expect that onebrowser will be the 7.6 release. By extension, that means using a version check for 7.6 and the playlistcontrol cmd:load command to achieve the goal of having a CLI command to play an album from a certain track on. This includes browse music folder, which should work after my last checkin
OK, thanks. What I do now is a two-step approach: I assume that when the version is 7.6, the functionality is available, either through cmd:load or through playtrackalbum. The I check for playtrackalbum, if it's there, I use it, if not, I use cmd:load. Please document the changed behavior of cmd:load compared to previous versions of SBS!! One more thing: Is the behavior of cmd:add changing, too, so will adding a folder only add the content of the folder and not of subfolders? While being another change it would be quite useful...
> Please document the changed behavior of cmd:load compared to previous versions > of SBS!! It is documented. cli-api docs, under the command playlistcontrol. jiveplaytrackalbum was never documented under any version of SBS, so there's nothing to compare it to. I did make a transition from jiveplaytrackalbum to playlist playtrackalbum, which is documented in 7.6/trunk, but that code has been in 7.6/trunk for about 3 or 4 days total, and as 7.6/trunk is probably never going to be released (deferring to onebrowser for the release), again there is nothing to do. If 7.6/trunk would be released in favor of onebrowser, I've fully documented those commands, which would be brand new. > One more thing: Is the behavior of cmd:add changing, too, so will adding a > folder only add the content of the folder and not of subfolders? While being > another change it would be quite useful... Nothing I checked in for playlistcontrol and browse music folder support is relevant to anything but cmd:load.
Sorry I was unclear: I meant the changed behavior of cmd:load "playlistcontrol cmd:load" alsways has been loading the content of a folder plus all subfolders in previous SBS versions. Now (I assume, if it's replacing jiveplaytrackalbum) it should only load the content of a folder without subfolders.
== Auto-comment from SVN commit #32426 to the slim repo by bklaas == == http://svn.slimdevices.com/slim?view=revision&revision=32426 == Bug: 17199 Description: document change in behavior of playlistcontrol cmd:load command with reference to browse music folder
Sorry, this is broken again. With Music Folders the "play_index" doesn't work in 7.6.1
Joerg, are you sure that it did work in versions prior to 7.6.1? I haven't touched any code remotely related to this since May. Maybe this broke when onebrowser merged to trunk?
No. Id did work ONLY after onebrowser was merged to trunk. I believe Alan fixed this. It worked in 7.6.0 but the problem there was that this has a side effect (whic I already mentioned after the original fix but which was then decided to be the intended behavior), it did not allow playing a folder WITH subfolders anymore. Some users complained so that was changes again. I originally thought that the play_index feature still worked (I believe I _did_ test it when the original change was done) but maybe later something was harmonized or whatever and it stopped to work again. The other bug regarding this is this one: https://bugs-archive.lyrion.org/show_bug.cgi?id=17347
*** Bug 17576 has been marked as a duplicate of this bug. ***
Do you plan to ever fix this bug or do I have to revert to the undocumented feature? This is extremely annoying since it's hard to use music folders with current server versions. This is NOT an enhancement request but a bug because the functionality doesn't work as documented in the CLI spec.
Jörg - I'd suggest you close this bug and start a new bug report with a clear description what wasn't working. This bug has gone back and forth so many times, even after reading all of it I'm not clear what's fixed, what's not, and what used to be fixed before it was broken again...
I agree with Michael's comment. The original problem has been resolved in that documented functionality has been added to the CLI to enable playing an album starting at a specific track. If subsequent bugs have arisen the please open a new bug.
Well, I would not have called "added just to be removed again" a fix which is why I re-opened the bug but here's a new one: https://bugs-archive.lyrion.org/show_bug.cgi?id=17835