benwiley4000 / cassette

📼 A flexible media player component library for React that requires no up-front config
https://benwiley4000.github.io/cassette/styleguide
MIT License
185 stars 28 forks source link

Update duration on track change/load #363

Closed danielr18 closed 5 years ago

danielr18 commented 5 years ago

Duration is only updated at the moment with the durationchange event, so this leads to a few seconds when you play a new track and it shows the previous duration (which possibly lets you seek to a position longer than the new track duration during that time frame).

The spec says that duration is set to NaN during the load algorithm, but doesn't trigger durationchange.

If I had to choose between previous duration and NaN (possibly displayed as 00:00), I would go with NaN, and this would keep it consistent with the media element.

However, I think it would be nice to be able to specify the duration in the playlist track object, and be able to use this when duration is NaN. I think Spotify might do something like that (or not), because I see that a track appears in the list with a duration of 5:22, and then when you play it, duration changes to 5:22 for a short amount of time and then it updates to 5:21.

benwiley4000 commented 5 years ago

Specifying duration as part of the track data is a good idea. We could even make it a required track property. But still have a fallback if it's not specified.

NaN is a pain to deal with. Maybe it would be better to store it as 0 than NaN when the duration isn't specified - currently we use 0 as the default, so we could set duration: track.duration || 0 when we choose a new track?

Finding the track time as a value in seconds is a bit impractical, so maybe we could allow specifying it in [hh:][mm:]ss form, and translate it to a number before we set it to state.

benwiley4000 commented 5 years ago

Did you want to do a PR for this?

danielr18 commented 5 years ago

I like duration: track.duration || 0.

I'm not too set whether to go with seconds or string, some users might have their duration in seconds, others as time strings, and one of those will have to convert them before adding the track.

I guess that if we are going to end up turning it into a number, it might be better to accept a number, preventing double conversions that way for users that have the durations already in number.

PD: OwlTail's API sends duration as time strings, so I'm not looking at it with a biased opinion.

benwiley4000 commented 5 years ago

Technically if you do the hh:mm:ss format you can convert a number to string and parse it and it will come out the way you want.

But my thought was to accept either one and just check if it's a string or a number so we don't bother converting numbers. I don't think this is going to be a performance bottleneck either way.

benwiley4000 commented 5 years ago

The main reason I'm leaning toward supporting timestamp format is that it's much easier to get the info in that format and I don't want creating a playlist to be an annoying task for a new user.

benwiley4000 commented 5 years ago

Any thoughts on going ahead with implementing this?

danielr18 commented 5 years ago

I'm wondering if it's better to just let the user show the duration he wants, kind of like we do with paused and awaitingPlayResume.

If we just set it back to 0 in the state, users can do something like: {props.duration || props.playlists[props.activeTrack].duration}.

The only change in core would be to make duration: 0 on load.

danielr18 commented 5 years ago

However, I think you might be interested on implementing this in cassette player/components, so you would have to define where you specify the duration in the playlist track, and what format.

benwiley4000 commented 5 years ago

Yeah.. I have mixed feelings here. On the one hand I think I understand your perspective of wanting consistency with the media element. On the other hand, I don't think it hurts anything to set the state duration as the user-supplied version if it exists. No one really wants to look at 0:00/0:00.

benwiley4000 commented 5 years ago

If I can reduce the amount of work for something that just makes sense, that seems better to me.

danielr18 commented 5 years ago

Makes sense, I agree. If for any reason, that behaviour doesn't suit someone, they could always check media.duration or just don't set the duration in the track, or give it another name.

benwiley4000 commented 5 years ago

My only hesitation would be if this would cause bugs (like with seeking) but I doubt it.. and we could fix anything that comes up.

benwiley4000 commented 5 years ago

Did you want to make a PR for this one?

danielr18 commented 5 years ago

Should we pass the track to getGoToTrackState? I think that would be the best place to set track.duration || 0. And we would also avoid doing this:

const currentTime = playlist[index].startingTime || 0;
this.goToTrack({ index, currentTime });
benwiley4000 commented 5 years ago

Sure, makes sense.

benwiley4000 commented 5 years ago

I just made a new release (v2.0.0-alpha.30), which includes a fix where we now respect the duration property when Cassette first loads. I think before it was just used when switching tracks.