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

Playlist item with infinite duration #4455

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

joosthoi1
Copy link

Infinite duration for playlist presets, so playlists can be controlled by just calling the next playlist api call.

The ui isn't great.

@softhack007
Copy link
Collaborator

@joosthoi1 the existing checkbox "Repeat indefinitely" did not work for you?

@DedeHai
Copy link
Collaborator

DedeHai commented Jan 4, 2025

@softhack007 this change would allow for infinite play per entry, not the whole list. its a "hack" to allow preset grouping. I did not have time yet to look at the nested playlist commit from @blazoncek, that may also do something similar from what I was able to pick up on discord.
I like the general approach taken here but need to check out nested playlists first.

@blazoncek
Copy link
Collaborator

There isan issue with this (and the reason I haven't published my solution 2 days ago).
If you accidentally enter 0 for one of the entries playlist will stop at that point and may cause user frustration. My prefered method would be to add a selection checkbox stating that this is a manual advance playlist and hide duration entry field.

There's also a nonsense in this code. I think you will easily spot it. 😉

@joosthoi1
Copy link
Author

joosthoi1 commented Jan 4, 2025

If you accidentally enter 0 for one of the entries playlist will stop at that point and may cause user frustration. My prefered method would be to add a selection checkbox stating that this is a manual advance playlist and hide duration entry field.

Do you think it should still be implented so that duration = 0 is manual advance, or should there be a separate variable to stop automatic advancing?

I think duration 0 could still work and we'd just update the UI to abstract this away. I completely agree with you that this ui at the moment isn't great and a checkbox would be better.

I haven't published my solution 2 days ago)

The only reason I uploaded this was because I couldn't find your commit and wanted to use it on my personal machine. Feel free to close this pr if your implementation is better. I already got what I need :P

There's also a nonsense in this code. I think you will easily spot it. 😉

I'm not sure of the top of my head what you mean, if you mean dur > 1 or dur == 0, that can definitely be simplified to dur != 1 lol. I made this code late last night, so it's entirely stupid.

@joosthoi1
Copy link
Author

joosthoi1 commented Jan 4, 2025

Ah, I also saw that I didn't change how I checked if dur == 0 in handleplaylist, so that implementation is stupid aswell

@blazoncek
Copy link
Collaborator

Screenshot 2025-01-04 at 11 18 43
That's better. Will be available shortly.

@blazoncek
Copy link
Collaborator

67c4dbf
builds upon nested playlists

@joosthoi1
Copy link
Author

67c4dbf builds upon nested playlists

I haven't tested this yet, but doesn't this allow for duration = 1 (0,10 seconds)? which is specifically not allowed as explained in #4164 (comment)

Other than that, looks good

@softhack007 softhack007 changed the title Playlist infinite duration Playlist item with infinite duration Jan 4, 2025
@blazoncek
Copy link
Collaborator

100ms is about 4 frames, plenty of time for ESP8266 to switch preset.
Will it crash? Not because switching presets from a playlist. Using UI to switch presets as quickly is completely different story.
Why 200ms originally? Because you would have 100ms transition time and 100ms effect duration making effect never properly show.
Will this matter? Most likely not as additional code will make sure you cannot have transition longer than effect duration.

@joosthoi1
Copy link
Author

Okay, that's fine then

@joosthoi1
Copy link
Author

So how do we proceed from here? Do you make a seperate pull request, or add the changes to my branch?

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.

5 participants