Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Options for double clicking a song in the playlist #4991

Merged
merged 6 commits into from
Aug 17, 2015

Conversation

redyoshi49q
Copy link
Contributor

I've hacked in a feature (and the options setting to adjust it) that allows for a variety of behavior when a song in the playlist is double clicked.

The default behavior is to just play the song (this is the prior behavior), so current users won't notice a behavior change if they don't change any options.

If the default is changed, the user can instead enqueue the song; if nothing is playing (and the option for playing is set), the first enqueued song (which will be the clicked one if there was previously no queue) will play.

Alternatively, the user can set the double clicked song to be the next to play. The current implementation does this by wiping the queue before enqueueing the next song; while a better implementation would instead append to the beginning of the queue, that doesn't appear to be as easily accomplished with the queue's API.

Lastly, there's an option to disable the execution of any action upon double clicking a song in the playlist, in case someone would want that.

@hatstand
Copy link
Contributor

Run make format

this will allow the make format to happen without causing a merge conflict
@redyoshi49q
Copy link
Contributor Author

I performed a make format and pushed the updated branch. I had to merge master into the branch first because the make format I did without merging first created merge conflicts (it seems the commit from master that I forked itself needed a make format, and I suspect that the redundant correction of that caused the conflict).

On a side note, it's cool to know that that tool exists. I spent quite a bit of time trying to manually emulate what I thought was the coding style (including backspacing over automatically generated tabs in order to replace them with spaces).

hatstand added a commit that referenced this pull request Aug 17, 2015
Options for double clicking a song in the playlist
@hatstand hatstand merged commit 15b33af into clementine-player:master Aug 17, 2015
@TheUbuntuGuy
Copy link
Contributor

This caused me a lot of confusion until I found this PR. I don't think it's a good idea to default to PlaylistPlayBehaviour_IfStopped because it's not what users will expect and they will think something is broken (like I initially did), especially if they upgrade from a previous version. I think it is best to set it to PlaylistPlayBehaviour_Always by default, emulating the old behaviour.

@redyoshi49q
Copy link
Contributor Author

It was my intention to emulate the old behavior by default, but having looked through the code, you're right; I goofed that up. I've added a commit to double_click_playlist (after fast forwarding the branch) to change the default to PlaylistPlayBehaviour_Always as suggested. I'm doing a test compile as I post this, though I don't expect there to be any problems with so small a change.

I'm also open to suggestions on how to present the UI options for this to the user. I admit that the options as they stand now are a bit clunky (though hopefully understandable), but I'm not entirely sure how to improve them. I've considered getting rid of PlaylistPlayBehaviour dropdown entirely, but the options there do provide some flexibility that would otherwise be lost, even though they're situationally redundant.

@ArnaudBienner
Copy link
Member

Sorry for the delay commenting this PR, but I'm ambivalent about this change.
I understand it might be useful in some cases, so maybe some options make sense here, but currently IMHO there are too many, and this confuses me a lot.
At least, just starting to play with it, things aren't intuitive at all I believe.

What is the purpose of the second column/combobox? Wasn't just one column enough?
"Play if there is nothing already playing"
"Never start playing"
"Always start playing"
If first is "Change currently playing song" but the second is "Play if there is nothing already playing" it will not do anything AFAICT. I'm not sure what to expect here, probably nothing indeed, but then "Do nothing" is what I should use then.
For "Add to the queue", it will indeed add a song to the queue and start playing right away (either the song just added to the queue if it was the first one, or another the first one we added previously). In addition of being a weird behavior IMO, I'm not sure how useful this can be.

So OK, why not giving the user more control about this, but I think we should stick to something simple.
For example just:

  • Change the currently playing song
  • Add to the queue

"Replacing the queue" looks strange to me (and it's not really a queue anymore if you keep replacing it with one song all the time)
"Doing nothing" sounds strange: maybe there is some corner case where you don't want to accidentally start a song by clicking on it, but in this case it's safer to simply lock your computer or whatever than having this option: now it's more likely that people might accidentally activate this option without noticing and then believe Clementine is broken (http://limi.net/checkboxes-that-kill/)

Well, don't take me wrong: what you did is great and this is just opinion :) But I'm pretty sure we can make Clementine more customizable while keeping it easy to use.

@redyoshi49q
Copy link
Contributor Author

@ArnaudBienner , I made a pull request based on your suggestion. I made my original list of options with the mentality of giving the user the greatest number of options possible; the utility of such options was a bit of an afterthought. The link you provided on the subject was an effective counterargument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants