ccgauche / ytermusic

An in terminal youtube music client with focus on privacy, simplicity and performance
Apache License 2.0
378 stars 19 forks source link

Going back to a playing playlist shouldn't change the play state #35

Closed Alphare closed 1 year ago

Alphare commented 1 year ago

Currently if you start a playlist, then go back to the playlist list view, then back inside the playlist, the first song is started again from the start. IMO it shouldn't do anything.

I guess this ties into the (maybe configurable) idea of not auto-playing when going into a playlist, which would allow for browsing, helping #26 be even more useful.

ccgauche commented 1 year ago

I see your point options that could be done:

ccgauche commented 1 year ago

@Alphare What option would be the best ?

Alphare commented 1 year ago

I'm not really familiar with the code architecture, but my naive approach is to make it so opening a playlist doesn't do anything (and you need to press enter or click on a song to start it), overriding the currently playing music as well as the queue (allowing you to go back to the previous songs, but forward logically through the new playlist).

Does that make sense?

ccgauche commented 1 year ago

@Alphare I implemented your suggestion. Can you tell me if everything is working as you expected? I also changed how I handle list so they are more reliable in the long run. Could you tell me if they still work for you.

Alphare commented 1 year ago

Thanks for working so fast!

I've tested it and there is a bug: open a playlist (which should start the replay, even though we could argue this is not perfect, but I like it), then press ESC, open another playlist and hit enter/click any song on that playlist and you will be brought back to the first playlist with no changes in playback.

Sidenote: pressing ESC twice brings you back to the playlist view again. I'm not sure if this is old behavior, but I find it somewhat confusing. IMO this behavior could stay if the "scroll view" and music player view had an obvious title or something.

ccgauche commented 1 year ago

Thank you for your feedback.

Alphare commented 1 year ago
* What should be the behavior when selecting a playlist when the player is empty?

IMO the current behavior is good for "least surprise", in that it defaults to playing music when you're starting to use the app. Can you confirm this is the currently intended behavior?

Maybe there could be a config option to do nothing until a song is explicitly chosen by the user even if the player is empty, I'd argue some people might want that.

* I wasn't able to reproduce your issue, may you send screenshoots describing what you did and what should have happened. I probably misunderstood something you said

That last step I would expect to move me to the player screen with the clicked song playing, with the rest of the playlist being replaced by the old one. If that still isn't clear I'll try creating a screencast of sorts.

* Pressing ESC brings to the player in the "Playlist inspector" and in the player brings to the "Playlist selector" so pressing twice does INSPECTOR -> PLAYER -> SELECTOR
  Can you clarify the expected behavior?

IMO pressing ESC should be "go back to the previous UI state" kind of shortcut, otherwise you always need to remember the context in which you're in to know where ESC will take you next. So I would expect it to do either INSPECTOR -> PLAYER -> INSPECTOR or INSPECTOR -> PLAYER -> (nothing).

  Thanks again you've definitely the one that made this project able to go from a side-project to a usable app.

Thanks for all the work, I'm glad to help, you're making the player I've been too busy (or lazy) to make for a long time. :)

ccgauche commented 1 year ago

IMO the current behavior is good for "least surprise", in that it defaults to playing music when you're starting to use the app. Can you confirm this is the currently intended behavior? Maybe there could be a config option to do nothing until a song is explicitly chosen by the user even if the player is empty, I'd argue some people might want that.

It is the intended behavior but I can add a config option.

That last step I would expect to move me to the player screen with the clicked song playing, with the rest of the playlist being replaced by the old one. If that still isn't clear I'll try creating a screencast of sorts.

I fixed that behavior. I did the system so it's coherent with the search (You select a music and it doesn't start playing but is pushed in the queue)

IMO pressing ESC should be "go back to the previous UI state" kind of shortcut, otherwise you always need to remember the context in which you're in to know where ESC will take you next. So I would expect it to do either INSPECTOR -> PLAYER -> INSPECTOR or INSPECTOR -> PLAYER -> (nothing).

This is a little more complicated has the current system doesn't use pagination (Since historically they were only two menus there was no need for it. But now I'll maybe reconsider implementing a proper pagination system (At least for the inspector).

I also updated the design of the inspector view to be more coherent and nice.

ccgauche commented 1 year ago

ed2ee3487e56b767aeb59d063e1b6a6f0283726c 18a825b5ea1857d7f4cca085c4fb3375213288f5

Alphare commented 1 year ago

It is the intended behavior but I can add a config option.

Cool, sure. :)

I fixed that behavior. I did the system so it's coherent with the search (You select a music and it doesn't start playing but is pushed in the queue)

The fixed behavior seems correct to me! I'm not sure what to believe since your sentence in parentheses means the opposite of the behavior I'm observing? It does start playing when you click it. Anyway thanks for being so fast.

This is a little more complicated has the current system doesn't use pagination (Since historically they were only two menus there was no need for it. But now I'll maybe reconsider implementing a proper pagination system (At least for the inspector).

Sure, no problem. Something that should be easy to implement is having a navigation shortcut to go each screen. I'm not exactly sure if the main shortcut namespace is too cluttered already or if having a chord (modifiers like ctrl, etc.) or even a vim-like modal system would be the best. Probably easy to try and see how it feels!

I also updated the design of the inspector view to be more coherent and nice.

Thanks! I've actually noticed that the inspector view and the player view don't display song names in the same manner, making the switchover quite jarring/confusing: the music player displays in the form of <artist> | <song> whereas the inspector view displays <song> | <artist> (album if available).

Alphare commented 1 year ago

Oh, here's a fun bug:

ccgauche commented 1 year ago

Oh, here's a fun bug:

Oh yeah I totally forgot to fix this one. (I already noticed it but never took the time to fix it) I'll do that as soon as possible (due to the screens being lazy to use less resources and the player not getting updated while something else is open.

The fixed behavior seems correct to me! I'm not sure what to believe since your sentence in parentheses means the opposite of the behavior I'm observing? It does start playing when you click it. Anyway thanks for being so fast.

The parenthesis was the behavior happening in the search tab what I was saying is that I first made the playlist inspector follow this rule to be coherent but then updated to follow the behavior you expected.

Sure, no problem. Something that should be easy to implement is having a navigation shortcut to go each screen. I'm not exactly sure if the main shortcut namespace is too cluttered already or if having a chord (modifiers like ctrl, etc.) or even a vim-like modal system would be the best. Probably easy to try and see how it feels!

I have ideas to fix this issue: The shortcut namespace has plenty of free space so no issue, Implementing a proper navigation, and / or adding a sort of quick action bar (Like finder on MacOS or the Launcher on gnome) with auto-completion ie typing: open playlists play This song title search playlist Halloween songs queue Another song clear queue ...

Thanks! I've actually noticed that the inspector view and the player view don't display song names in the same manner, making the switchover quite jarring/confusing: the music player displays in the form of | whereas the inspector view displays | (album if available).

I'll fix that!

ccgauche commented 1 year ago

@Alphare Real navigation is now in YTerMusic + Search for playlist (Can you tell me if everything works as expected) Maybe I should create a discord it would be easier...