[New Feature] keep playlist visible after `key_playfile`

[verygoodlee]

The playlist automatically closes after using key_playfile in it, I want to keep it visible.

Adding a new value 3 to show_playlist_on_fileload can be easily solved. \ show_playlist_on_fileload=3 means if playlist is visible before fileload, keep it visible，if playlist is invisible before fileload, keep it invisible.

[jonniek]

Hi, thanks for the PR.

I think we could maybe add another option instead close_playlist_on_playfile = true,. And then in playfile we can do

if settings.close_playlist_on_playfile then
    remove_keybinds()
end

I think this makes it more simple to understand how it works.

Your behaviour would be:

show_playlist_on_fileload = 0,
close_playlist_on_playfile = false,

There is probably some edge case of playlist refreshing and stale data still to consider. There is some code like this that can refresh the UI (and timeout duration):

if playlist_visible then
    showplaylist()
end

[verygoodlee]

Adding another option was my initial idea，but it conflicted with the show_playlist_on_fileload option.\ in this case, there are two understandings, only one of the two options ultimately works.

show_playlist_on_fileload = 2
close_playlist_on_playfile = true

1. show_playlist_on_fileload=2 open playlist, then close_playlist_on_playfile=true close it
2. close_playlist_on_playfile=true close playlist, then show_playlist_on_fileload=2 open it

Adding a new value to show_playlist_on_fileload is to avoid this conflict.

[jonniek]

It's a bit confusing here 3 keep playlist visible(if it is visible before fileload), 2 shows playlist, 1 shows current file(filename strip applied), 0 shows nothing. The option 3 and 0 sound like they do a similar thing. But they are kind of coupled to the loadfile and playfile in an unobvious way.

I think the problem is that there are 2 different file loading from users point of view.

1. File changes automatically when reached EOF
2. File changes manually when selecting what to play

The user may want to have different behaviour on different events.

For example with your change if you want to display the title on passive file load, you can no longer have any control on if the playlist is closed on playfile or not.

As for the conflict I think it's fine to simply close the playlist, and then let the other option open it. It's a nonsensical configuration, but it is more flexible for the other usecases. As long as we provide a reasonable default I think there is no issue.

As long as the default for show_playlist_on_fileload is 0, then I think default for close_playlist_on_playfile could be true or false. I don't have any strong opinion on it, even though the current assumption in code is that it is should be closed. I don't think it's a big deal to close it manually. And if you are going through a lot of files quickly, like an image gallery it's quite nice if the playlist doesn't always close. So maybe your use-case should be the default.

As for the stale rendering issue, seems like the end file hook does a if playlist_visible then showplaylist() end that seems to avoid it. I feel like this should probably be in the load file function instead, but seems to still work.

Another possible option would be to separate "playlist" and "filename" display logic into separate configurations so they would be fully independent of eachother. Now that I think about it, I think it's maybe the best solution. Something like:

show_playlist_on_file_load = false,
close_playlist_on_playfile = false,
show_title_on_file_load = false,

It still allows for the conflicting config, but at least all behaviour is very explicit and clear. The reason filename and playlist have historically been in the same config, is that this script used to render both in the OSD message. Nowadays it renders them separately and both can be displayed at the same time. It makes sense to separate them.

[verygoodlee]

update as you said @jonniek

In fact, if playlist_visible then showplaylist() end conflicted with some script-message commands. For example: KEY script-message playlistmanager show playlist-nokeys 5\ show playlist-nokeys with 5 seconds duration, If reach the EOF within 5 seconds, playlist still visible, so showplaylist() will be called, then playlist changes to default state, with keys and no duration.

I don't have a good idea to solve it, still use it to refresh the UI. May require a new functionrefresh_UI(), do not touch the keybinds and keybindstimer. if playlist_visible then showplaylist() end can all be replaced by refresh_UI(), I haven't tested it yet.

function refresh_UI()
  if not playlist_visible then return end
  refresh_globals()
  if plen == 0 then return end
  draw_playlist()
end

[verygoodlee]

Adding new function refresh_UI() may not be too easy, showplaylist() has been used in many places, \ need to carefully read the source code to determine whether you need to register keys, and it's hard to test comprehensively.

[jonniek]

Yeah true, I can take a look at that. Thanks for the PR! It's a nice feature.