This repository was archived by the owner on Sep 8, 2021. It is now read-only.
Add 'add to play queue' buttons to the playlist page #1433
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Sorry I had intended to post a comment on this PR earlier and the reason it was rejected from the I think this PR should be reverted on this fork, or fixed. The central method in the PR is erroneous and the only reason it hasn't been spotted is either no one really reviewed the PR, or the UI is masking the actual error in the method because it just sends nulls as an argument. Anytime you send a non null argument into the method, the error will become obvious. |
Comment on lines
+256
to
+278
| public PlayQueueInfo addPlaylist(int id, Integer index) throws Exception { | ||
| HttpServletRequest request = WebContextFactory.get().getHttpServletRequest(); | ||
| HttpServletResponse response = WebContextFactory.get().getHttpServletResponse(); | ||
|
|
||
| String username = securityService.getCurrentUsername(request); | ||
| boolean queueFollowingSongs = settingsService.getUserSettings(username).isQueueFollowingSongs(); | ||
|
|
||
| List<MediaFile> files = playlistService.getFilesInPlaylist(id, true); | ||
| if (!files.isEmpty() && index != null) { | ||
| if (queueFollowingSongs) { | ||
| files = files.subList(index, files.size()); | ||
| } else { | ||
| files = Arrays.asList(files.get(index)); | ||
| } | ||
| } | ||
|
|
||
| // Remove non-present files | ||
| files.removeIf(file -> !file.isPresent()); | ||
|
|
||
| // Add to the play queue | ||
| int[] ids = files.stream().mapToInt(f -> f.getId()).toArray(); | ||
| return doAdd(request, response, ids, index); | ||
| } |
There was a problem hiding this comment.
This method seems incorrect. It's not doing what it should be doing. You haven't discovered the issue yet because you're not using the arguments from DWR properly (it always passes in a null for an index).
The issues are:
- I think you've confused
indexand used the same variable for two separate tasks- There are two indices in question here
- One passed into the method itself
addPlaylist(id, index): Thisindeximplies which part of the playlist to add. If you pass in 0, it should select the entire playlist to add. If you pass in 20, it'll add the songs in the playlist after index 20. And so on. You can see that from thesublist()bit you're doing - One passed into the method
doAdd(request, response, ids, index)which you call at the end to do the actual adding into the playqueue. Thisindexrepresents where in the playqueue you want to add the given files.
- One passed into the method itself
- You pass the same
indexto both places. Imagine if index is 10. You're telling the function to select the playlist's songs starting at index 10 and also add it into the playqueue at its own index 10. These (playqueue and playlist) are two separate entities. Each's index has nothing to do with the other and yet are being passed around interchangeably. NPE will be the most frequent error, though you'll run into plenty more if your terms are confused.
- There are two indices in question here
- I'm not sure why that whole bit about
queueFollowingis needed. Looks like copy-paste error from another function. You don't need it. You're appending the playlist in its entirety or its sublist. There isn't a question of whether to add followup songs. The playlist itself has all the songs. - The method signature implies that you can select sublists of the playlist. But your UI specifically says "Add All", and never sends any index (always a null), which means this method never actually is using the index. My suggestion is to just remove the index here. You can add the entire playlist or nothing at all. When you come up with another UI design to add sublists, you can expand it later (YAGNI)
There was a problem hiding this comment.
Good catch, I'll send a new PR for that!
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should fix #1226.