search

boysetsfrog / vimpc

Official repository for vimpc a vi/vim inspired client for the Music Player Daemon (mpd). Pull requests are welcome.
GNU General Public License v3.0
269 stars 34 forks source link

Modify save playlist to automatically overwrite #40

Closed 0xtobit closed 9 years ago

0xtobit commented 10 years ago

Issue #10, #11 and #12 describe the issue and fix for updating/saving a playlist that already exists. It appears that this was fixed, in 7d064482ba4379555cc6a068e4846f3e70269c96. However when I cloned, this commit seemed to be nowhere in the history. And HEAD was missing the line to remove the playlist before saving it. So I added it. I compiled and tested and it lets me overwrite playlists with saving.

Hope this helps, apologies if I've misunderstood something.

connermcd commented 10 years ago

Related to #41.

connermcd commented 10 years ago

This isn't working for me as is. I can neither create a new playlist or overwrite an existing one.

connermcd commented 10 years ago

I take that back. I can overwrite an existing one; I just can't create a new playlist.

connermcd commented 10 years ago

Or rather, it does create the playlist but shows an error. It should check whether the playlist exists before trying to remove it.

0xtobit commented 10 years ago

Thank you, I'll try and make some time to look into this issue and fix it.

0xtobit commented 10 years ago

Hey @connermcd I added a fix but I'm not sure I'm happy with it. This will catch all exceptions, which isn't the best practice, so I can refine it if you'd like.

If you would like me to revise it, I was hoping you could give me a bit of a recommendation on what kind of exception I could specifically catch (I tried a std::string and an mpd_error to no avail. Alternatively if there's a function to check if a playlist already exists that I couldn't find, I think that would suit nicely.

connermcd commented 10 years ago

Hey @0xtobit, thanks for following up on this. Unfortunately the solution still throws an error when trying to create a new playlist on top of not being best practices. I think the best solution is to first check if the playlist exists, and if so then remove it.

0xtobit commented 10 years ago

Right, do you know of any function already existing to check if a playlist exists?

Also that seems a little bit weird, I did not get an error anymore...

connermcd commented 10 years ago

I only get the error when creating a playlist that does not already exist. It is trying to remove a playlist that is not there.

I don't know of an existing function.

0xtobit commented 10 years ago

Alright I'll keep digging, thanks!

0xtobit commented 9 years ago

I give up, I cannot figure out how to properly determine whether a playlist exists with mpd before removing it.

0xtobit commented 9 years ago

See #51 for my alternate solution.

0xtobit commented 9 years ago

As another note, I originally did these commits on master, but I didn't like that. I cleaned my master to reflect upstream and now this PR looks empty, for my code so far, please see the branch playlist_exists in my fork.