Use startingTime from playlist tracks & onTimeUpdate prop #369

danielr18 · 2019-02-16T05:09:24Z

Solves #365

danielr18 · 2019-02-16T05:10:59Z

@benwiley4000 Do you want to help me with the documentation?

benwiley4000 · 2019-02-16T05:23:19Z

packages/core/src/PlayerContextProvider.js

    if (this.state.trackLoading) {
      // correct currentTime to preset, if applicable, during load
      this.media.currentTime = this.state.currentTime;
      return;
    }
+    if (onTimeUpdate) {
+      onTimeUpdate(currentTime, playlist[activeTrackIndex], activeTrackIndex);


Can we move this one to componentDidUpdate? Sort of subjective but I'm trying to stick handlers there when possible.

Like this?

if (prevState.currentTime !== this.state.currentTime) { const { onTimeUpdate, playlist } = this.props; const { activeTrackIndex, currentTime } = this.state; if (onTimeUpdate) { onTimeUpdate(currentTime, playlist[activeTrackIndex], activeTrackIndex); } }

Ok now that I see how complicated that is I changed my mind 😄.

Could you just leave it here but move it after the call to setState so the child re-render before the parent? (Not in the setState callback, just the next line)

benwiley4000 · 2019-02-16T05:27:38Z

packages/core/src/PlayerContextProvider.js

@@ -1055,7 +1073,6 @@ PlayerContextProvider.propTypes = {
  defaultRepeatStrategy: PlayerPropTypes.repeatStrategy.isRequired,
  defaultShuffle: PropTypes.bool,
  defaultPlaybackRate: PropTypes.number.isRequired,
-  startingTime: PropTypes.number.isRequired,


benwiley4000 · 2019-02-16T05:45:02Z

packages/core/src/PlayerContextProvider.js

  shouldPlay = true,
  shouldForceLoad = false
 }) {
  const isNewTrack = prevState.activeTrackIndex !== index;
  return {
    activeTrackIndex: index,
-    trackLoading: isNewTrack,
+    trackLoading: Boolean(isNewTrack || shouldForceLoad),


Is that a bug fix we should document in the next release?

I just thought that it would make sense to set it as true if we are going to call load()

I agree - I think it was maybe a bug before. Makes sense to me! 👍

benwiley4000 · 2019-02-16T05:48:07Z

packages/core/src/PlayerContextProvider.js

@@ -685,13 +703,10 @@ export class PlayerContextProvider extends Component {
    });
  }

-  handleMediaLoadedmetadata() {
+  handleMediaLoadedData() {


Just to follow the convention with the rest of the handlers could data (in handleMediaLoadeddata) be all lowercase?

benwiley4000 · 2019-02-16T06:47:57Z

packages/core/src/PlayerContextProvider.js

    if (this.media.currentTime !== this.state.currentTime) {
      this.media.currentTime = this.state.currentTime;
    }
-    this.setState(
-      state => (state.trackLoading === false ? null : { trackLoading: false })
-    );


A few thoughts here:

Remember that state.trackLoading is being used to enforce currentTime while the track is loading in handleMediaTimeupdate

Ok but speaking of that... I am looking at the diff where I introduced that and I honestly have no idea why I added this (I wish I had left a comment!). Is it even possible for the currentTime to change to something inconsistent with the current state before the metadata has loaded? Maybe we should totally remove that check and use the trackLoading state just for API reasons.

I think canplaythrough is too late for setting trackingLoading: false. If you want to change it to canplay, I could agree with that.

1, 2 - I think trackLoading is really useful for users UI. If you want to have something like trackLoadingState, where 0 is not loading, 1 metadata, 2 canplay, it would be more flexible for users to decide when they want to stop displaying their loading UI, and you could check for loadingState 1 in timeupdate, so that logic stays the same. I can tell you however that I've tried these PR changes, and I didn't find any issue.
3 - Sure

I hadn't considered exposing something like you're suggesting that essentially maps to the readyState. However that sounds a bit more advanced, so for now I'd say just go ahead with continuing to expose trackLoading and make it true when canplay fires. Thanks!!

benwiley4000 · 2019-02-16T06:52:38Z

packages/core/src/PlayerContextProvider.js

+        const currentTime = playlist[0].startingTime || 0;
+        this.goToTrack({
+          index: 0,
+          currentTime,


This seems weird to me. It brings me back to wondering if playlist tracks are the best place to store the starting time... it seems like temporal information, not something you would want to store in a playlist. I don't think you would want to reload a track that was already played at the starting time from before.. would you?

This isn't a rhetorical question, I'm really not sure.

Yes, you wouldn't want to reload a track at a startingTime after it ends, you could remove it from the track after it changes. However, I'm open to suggestions with regards to the API.

I didn't consider the case where you have more than 1 source per track.

I'm not sure what you're saying would happen with multiple sources.. I'm mostly just thinking about when you cycle back to a track that has already played. I don't want to create a bunch of extra work for the user in order to use the startingTime API correctly.

Yeah, multiple sources comment isn't related to what you mentioned, it was just a thought that current API proposal doesn't support different starting tracks per source.

One thought I had to is to have something called intialStartingTime (instead startingTime) and we would keep it either in the track object or a separate prop. And it would only be respected the first time we play the track.. after that, it starts at the beginning as normal.

The problem with this is that if we save a currentTime for a skipped track then come back to it without unmounting the component, we won't have a way to tell that track to resume at the previous currentTime.

So just for a moment, considering my previous idea - where we save the currentTime for all tracks in the state snapshot (and assuming we implement the feature where you can swap in a new state snapshot), what about that doesn't work for you? I'm trying to understand the drawbacks of that approach.

I think a glaring problem about that approach is inconsistencies if you have a user listening on multiple devices concurrently, but besides that.

You should be able to switch between tracks multiple times and resume where you left (first time or not) and for that to work, I guess we would have to pass that initialStartingTime prop after every snapshot, it would be the same as updating the playlist all the time.

I guess we could do something like that, but it wasn't my first option to keep the library simpler, since as you said, it's an advanced feature.

I think if we were to go with the snapshot we would also override the startingTimes internally without worrying about a prop. But like I said below, I think we should go with what you've already implemented.

benwiley4000 · 2019-02-16T06:57:22Z

packages/core/src/PlayerContextProvider.js

      mediaCannotPlay: false,
-      awaitingPlayAfterTrackLoad: false
+      awaitingPlayAfterTrackLoad: false,
+      trackLoading: true


Seems like trackLoading: true is already there from the call to getGoToTrackState. Same for awaitingPlayAfterTrackLoad actually.

Not unless I pass shouldForceLoad: true, since trackLoading is set to true if the activeTrackIndex changes, and I can modify the playlist to remove the first track, and activeTrackIndex wouldn't change.

Good point.. maybe we should just set force load to true?

Sure, that makes sense, it will lead to the same behaviour in componentDidUpdate. Done.

benwiley4000 · 2019-02-16T21:59:23Z

So it seems like this could actually be two different pull requests:

Using canplay to set trackLoading: false
The onTimeUpdate callback prop and the startingTime API

I think 1 is basically ready to merge as soon as my comment about force load is addressed.

I'm not totally sure about 2 yet, needs more more thought.. so maybe we can pull out 1 and merge it now?

Also to answer your earlier question: yes I would always love help with documentation!! ☺️

danielr18 · 2019-02-16T22:02:56Z

Makes sense, let me get 1 prepared.

benwiley4000 · 2019-02-16T22:44:50Z

Ok honestly.. I've spent some time thinking through alternatives again and I think the solution you have implemented makes the most sense. This is an advanced feature and anyone using it will probably be dynamically updating the playlist anyway, so having to update the playlist contents with an updated startingTime should be no big deal.

benwiley4000

Thanks! And yes any help with documentation would be awesome.

BTW, if you think onActiveTrackUpdate should be passed info on the previous track and the currentTime to make things easier.. I'd be open to having that in a separate PR.

danielr18 · 2019-02-17T00:13:27Z

Is there any file to document startingTime? If not, we only have to add the onTimeUpdate info, and it would be ready to merge.

benwiley4000 · 2019-02-17T00:16:04Z

Everything about the playlist is documented currently here (might be a bit out of date?). I'd stick the info there. I need to integrate that somehow into the docs website eventually. If you have any ideas, go for it!

danielr18 · 2019-02-17T00:18:19Z

Yeah, I saw that one, but it doesn't have anything related to playlist tracks structure/fields

benwiley4000 · 2019-02-17T00:24:59Z

Sorry you're right. I got confused.

My plan was to eventually document some of the more complicated PropTypes for PlayerContextProvider, maybe in its markdown file? The info would be similar to what's currently in the README.md but updated

danielr18 · 2019-02-17T00:29:25Z

Maybe we can work on it in another PR, to document all the playlisy track fields.

…

On Sat, Feb 16, 2019, 9:24 PM Ben Wiley ***@***.*** wrote: Sorry you're right. I got confused. My plan was to eventually document some of the more complicated PropTypes for PlayerContextProvider, maybe in its markdown file? The info would be similar to what's currently in the README.md but updated for version 2 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#369 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIoKWL5aq0YIWemEjjy1JN2NhFx93eg_ks5vOKFbgaJpZM4a-yht> .

benwiley4000 · 2019-02-17T00:30:31Z

That sounds good to me!

benwiley4000 · 2019-02-17T00:31:17Z

And to leave comments for all the PropTypes so they show up in the docs

benwiley4000 · 2019-02-17T15:59:08Z

Thanks!

benwiley4000 · 2019-02-18T07:17:58Z

Released in v2.0.0-alpha.27 (has breaking changes, see the release notes)

benwiley4000 · 2019-04-08T06:31:54Z

BTW, if you think onActiveTrackUpdate should be passed info on the previous track and the currentTime to make things easier.. I'd be open to having that in a separate PR.

I ended up doing this in the latest release. Not there are some small breaking changes in the API. https://github.com/benwiley4000/cassette/releases/tag/v2.0.0-alpha.32

danielr18 added 2 commits February 16, 2019 01:03

Use startingTime from playlist tracks

8bf5a19

Add onTimeUpdate prop

be67601

benwiley4000 reviewed Feb 16, 2019

View reviewed changes

danielr18 added 3 commits February 16, 2019 10:10

set trackLoading: false after canplay

62dfc3f

Call onTimeUpdate on timeupdate event

a7fc367

Use shouldForceLoad if active track changed after playlist change

6a9c176

danielr18 mentioned this pull request Feb 16, 2019

Set trackLoading false after canplay #370

Merged

benwiley4000 approved these changes Feb 16, 2019

View reviewed changes

danielr18 marked this pull request as ready for review February 17, 2019 05:02

danielr18 added 2 commits February 17, 2019 10:24

add comment about onTimeUpdate

da851af

Merge remote-tracking branch 'upstream/next' into fix-365

3958e51

benwiley4000 merged commit 874dbea into benwiley4000:next Feb 17, 2019

Use startingTime from playlist tracks & onTimeUpdate prop #369

Use startingTime from playlist tracks & onTimeUpdate prop #369

Conversation

danielr18 commented Feb 16, 2019

danielr18 commented Feb 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielr18 Feb 16, 2019 • edited Loading

Choose a reason for hiding this comment

benwiley4000 commented Feb 16, 2019 • edited Loading

danielr18 commented Feb 16, 2019

benwiley4000 commented Feb 16, 2019

benwiley4000 left a comment

Choose a reason for hiding this comment

danielr18 commented Feb 17, 2019

benwiley4000 commented Feb 17, 2019

danielr18 commented Feb 17, 2019

benwiley4000 commented Feb 17, 2019 • edited Loading

danielr18 commented Feb 17, 2019 via email

benwiley4000 commented Feb 17, 2019

benwiley4000 commented Feb 17, 2019

benwiley4000 commented Feb 17, 2019

benwiley4000 commented Feb 18, 2019

benwiley4000 commented Apr 8, 2019

danielr18 Feb 16, 2019 •

edited

Loading

benwiley4000 commented Feb 16, 2019 •

edited

Loading

benwiley4000 commented Feb 17, 2019 •

edited

Loading