E-Kuerschner / useAudioPlayer

React hooks for controlling audio on the web
MIT License
330 stars 36 forks source link

useAudioPosition() position is not updated by calling seek() when the audio is not playing #38

Closed humbkr closed 4 years ago

humbkr commented 4 years ago

Describe the bug useAudioPosition() position is not updated by calling seek() when the audio is not playing, it's only updated when you start the playback again.

To Reproduce Steps to reproduce the behavior:

  1. Create an audio player with a clickable progress bar calling seek() on click
  2. Play an audio file and click the progress bar to seek to a new position: the "position" variable is updated across the components using useAudioPosition()
  3. Pause the playback
  4. Click the progress bar to seek to a new position: the "position" variable is not updated
  5. Start the playback again: the position variable is updated.

Expected behavior The "position" variable instantly updates when seek() is called, independently of the playback status.

Environment (please complete the following information):

E-Kuerschner commented 4 years ago

@humbkr the requested behavior is available in v1.0.0. Check it out and leave me some feedback

humbkr commented 4 years ago

Thanks for your work!

Unfortunately it now works but only for the current progress bar seek() is used from. To reproduce the issue, just add another progress bar to the basic example:

Use case: I use useAudioPosition() to display both a progress bar and a time counter " / "

Also in examples/src/VolumeControl.tsx line 7 I think you forgot a console.log()

humbkr commented 4 years ago

Looking at the code, the context provider contains only the Howl player and the load function, is there a reason for that?

In my opinion, if you want your player to be able to be used in a complex app like in your spotify-ish example, all the playback state should be available in the context object. Currently I think it would be difficult to build a complex player with a library management where you need to have access to the player state in real time in different components everywhere in the components tree.

E-Kuerschner commented 4 years ago

@humbkr thanks for reviewing the changes. Let me try to address both your comments here.

I agree that all the state (from useAudioPlayer) should be lifted up to the Provider. The way it works now is actually kind of confusing and I unfortunately didn't notice that change in the user experience as I was changing things too rapidly. I have also been thinking of doing a rewrite where there is no Provider and everything is scoped to the useAudioPlayer hook. As I have heard more and more peoples' use cases I think it might be appropriate to allow users to setup their own context for sharing state as they see fit rather than being opinionated and forcing that type of setup. I still like the idea of having a set of hooks that lets you distribute control to many components in your tree, but maybe that can just be left up to the user to setup themselves. Curious to hear your thoughts on this.

Regarding the useAudioPosition behavior - the position state is stored in the hook and seek modifies it so it makes sense to me that diff useAudioPositions are not syncing up. Perhaps the answer to the first point I made above could lead to a solution here as well.

humbkr commented 4 years ago

My opinion on this is that while I really like to use the API of this package, I find it more difficult to use to implement a fully functional player compared to react-howler for example. To implement a quick player to play short audio files, useAudioPlayer is easier, but as soon as you want to implement something a bit more complicated, it becomes way more difficult.

For example I spent a bit of time to figure out how to use the "ended" variable correctly when implementing a playlist feature, because I had to trigger a custom event to change track when a song ended, and I had to take in account my component re-renders flow so my events order was: track ended, ended variables is true => load new track with load() => ended variable is false, while using react-howler I could just subscribe to the onEnd event and not have any weird cases.

You can find a working work in progress code here: https://github.com/humbkr/react-howler-audio-player if you're interested.

Anyway, to answer your question I think:

In any case that's not a simple problem to resolved, having to find the right balance between ease of use and allowing to cover all use cases.

E-Kuerschner commented 4 years ago

@humbkr, not a simple problem indeed, but I'm down to tackle it! I think we can break these issues up into two separate issues/feature requests:

Anything else you can think of to get started? Also, more than happy to accept contributors if you would like to help and we can agree on the direction.

E-Kuerschner commented 4 years ago

Closing. Opened #39 & #40