Playlist Manipulation: Adding tracks to playlists#198
Playlist Manipulation: Adding tracks to playlists#198martingoe wants to merge 7 commits intojpochyla:masterfrom
Conversation
While it is possible to add multiple tracks, I decided to use the simple case of adding one track in this case. If one wanted to change this to support multiple track_uris, they can simply be passed as CSVs as described in the documentation.
The PublicUser data is sent when returning the current playlists and must be used to identify the owned playlists.
There are two things I see that could be changed: - The current command executor is in the playlists list_widget function. This might not be the optimal position. - If the request failed, the user should be notified. I believe this could be done with an alert box, but I did not currently find a way to implement this.
This function implements a way to find the playlists that the user owns and hence can change. There is one thing I see that could be changed: - The current function returns a `Vec` while the `Vector` is often used in this application. I was unsure if the vec should be cast to a vector but decided against it. However, the change is relatively simple.
If possible, this should be changed to get the current user's id without an api call.
|
I agree with @martingoe that an alert box would be essential - not just for this PR but for more transparency in general with failed API calls. I imagine it would be a bar that appears above the seek bar. Does this PR include a method of removing tracks as well? |
|
No, this PR does not include a way to remove a track from a playlist. With that being said, I did experiment with it and do have an implementation. I did not include it in this PR because I wanted it to focus on one feature. |
|
Hi @martingoe, thanks a lot for your interest and for the PR! I finally have some more time so we can finish this. I'll push some more commits into the branch of this PR if that's fine with you. |
|
Of course, go ahead. |
|
I've added support for very simple error alerts in 731dadd (in master). |
|
OK, so I've made some changes in a separate branch and merged to master, so far without the error alerts. Thanks a lot @martingoe and @jacksongoode. |
3 participants
As described in the commits, this PR is not quite ready to merge with the main project.
I believe the following problems should be discussed:
Commit 4294be7:
list_widgetfunction. This might not be the optimal position.Commit 8efb405:
Commit a43611e uses
Vecs instead of the more commonly usedVector.edit: I was unaware of the draft feature github has but I believe this works just as well.