Skip to content

Conversation

@DanielFRico
Copy link
Contributor

Hi, team! I would you add next feature to onCurrentRecordingWaveformData listener. In my opinion its useful if user wants to know current recorded time. In our case to displayed how long time is being recorded in real time. Regards!

@kuldip-simform
Copy link
Contributor

@DanielFRico Thank you for your valueble contribution to this library. We will test this internally and let you know if this needs any modification or not.

Copy link
Contributor

@kuldip-simform kuldip-simform left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielFRico Can you please update Readme file to include progress value in callback ?

@kuldip-simform
Copy link
Contributor

@DanielFRico This functionality works properly on both iOS and Android.
I do have one recommendation that we should implement in order to ensure that this feature is beneficial to all consumers.

We should include a callback prop to transmit the updated time duration to the waveform component, similar to the process we use for static waveforms.

For static waveforms, we have a property named onCurrentProgressChange. Would you be able to look into that and include a similar prop (such as onRecordingDurationChange or onRecordingProgressChange)? This will enable users to independently display the time without having to rely on their implementation.

useEffect(() => {
    if (!isNil(onCurrentProgressChange)) {
      (onCurrentProgressChange as Function)(currentProgress, songDuration);
    }
  }, [currentProgress, songDuration, onCurrentProgressChange]);

const tracePlaybackValue = onCurrentDuration(data => {

const tracePlaybackValue = onCurrentDuration(data => {
      if (data.playerKey === `PlayerFor${path}`) {
        const currentAudioDuration = Number(data.currentDuration);

        if (!isNaN(currentAudioDuration)) {
          setCurrentProgress(currentAudioDuration);
        } else {
          setCurrentProgress(0);
        }
      }
    });

Let me know your thoughts on this.

@kuldip-simform
Copy link
Contributor

@DanielFRico Upon further testing, we found that this approach would be difficult to manage for implementing resume and pause recording, as it currently only accounts for startRecording.

To handle this properly, we need to consider scenarios where users utilize pauseRecording and resumeRecording while maintaining the correct progress. If we go ahead with this, we would need to account for all these cases; otherwise, it might be better to allow users to handle progress tracking on their own.

Let us know your thoughts—should we proceed with refining this implementation, or would it be better to close this PR for now?

@DanielFRico
Copy link
Contributor Author

@DanielFRico This functionality works properly on both iOS and Android. I do have one recommendation that we should implement in order to ensure that this feature is beneficial to all consumers.

We should include a callback prop to transmit the updated time duration to the waveform component, similar to the process we use for static waveforms.

For static waveforms, we have a property named onCurrentProgressChange. Would you be able to look into that and include a similar prop (such as onRecordingDurationChange or onRecordingProgressChange)? This will enable users to independently display the time without having to rely on their implementation.

useEffect(() => {
    if (!isNil(onCurrentProgressChange)) {
      (onCurrentProgressChange as Function)(currentProgress, songDuration);
    }
  }, [currentProgress, songDuration, onCurrentProgressChange]);

const tracePlaybackValue = onCurrentDuration(data => {

const tracePlaybackValue = onCurrentDuration(data => {
      if (data.playerKey === `PlayerFor${path}`) {
        const currentAudioDuration = Number(data.currentDuration);

        if (!isNaN(currentAudioDuration)) {
          setCurrentProgress(currentAudioDuration);
        } else {
          setCurrentProgress(0);
        }
      }
    });

Let me know your thoughts on this.

@kuldip-simform You're completely right! I've added your suggestion in latest commit

@kuldip-simform
Copy link
Contributor

@DanielFRico Upon further testing, we found that this approach would be difficult to manage for implementing resume and pause recording, as it currently only accounts for startRecording.

To handle this properly, we need to consider scenarios where users utilize pauseRecording and resumeRecording while maintaining the correct progress. If we go ahead with this, we would need to account for all these cases; otherwise, it might be better to allow users to handle progress tracking on their own.

Let us know your thoughts—should we proceed with refining this implementation, or would it be better to close this PR for now?

@DanielFRico Can you please check this comment? also can you please take a pull or rebase your branch with the latest develop branch as we have merged some code?

@DanielFRico DanielFRico force-pushed the feature/add-progress-to-onCurrentRecordingWaveformData-listener branch from cecd01b to ac4e82a Compare February 24, 2025 16:19
@DanielFRico
Copy link
Contributor Author

@DanielFRico Upon further testing, we found that this approach would be difficult to manage for implementing resume and pause recording, as it currently only accounts for startRecording.
To handle this properly, we need to consider scenarios where users utilize pauseRecording and resumeRecording while maintaining the correct progress. If we go ahead with this, we would need to account for all these cases; otherwise, it might be better to allow users to handle progress tracking on their own.
Let us know your thoughts—should we proceed with refining this implementation, or would it be better to close this PR for now?

@DanielFRico Can you please check this comment? also can you please take a pull or rebase your branch with the latest develop branch as we have merged some code?

@kuldip-simform done!

@kuldip-simform
Copy link
Contributor

@DanielFRico Upon further testing, we found that this approach would be difficult to manage for implementing resume and pause recording, as it currently only accounts for startRecording.

To handle this properly, we need to consider scenarios where users utilize pauseRecording and resumeRecording while maintaining the correct progress. If we go ahead with this, we would need to account for all these cases; otherwise, it might be better to allow users to handle progress tracking on their own.

Let us know your thoughts—should we proceed with refining this implementation, or would it be better to close this PR for now?
@DanielFRico Can you verify this scenario as well where user can pause recording and resume again, I do not think that our current implementation can handle this edge case.

@DanielFRico
Copy link
Contributor Author

@DanielFRico Upon further testing, we found that this approach would be difficult to manage for implementing resume and pause recording, as it currently only accounts for startRecording.
To handle this properly, we need to consider scenarios where users utilize pauseRecording and resumeRecording while maintaining the correct progress. If we go ahead with this, we would need to account for all these cases; otherwise, it might be better to allow users to handle progress tracking on their own.
Let us know your thoughts—should we proceed with refining this implementation, or would it be better to close this PR for now?
@DanielFRico Can you verify this scenario as well where user can pause recording and resume again, I do not think that our current implementation can handle this edge case.

@kuldip-simform I've added some changes to support what you said. I made test both iOS and Android and progress value now is sync also with events like pause and resume

@kuldip-simform
Copy link
Contributor

@DanielFRico Thank you for updating the PR.

While testing the pause and resume functionality, I noticed a mismatch between the actual recording duration and the duration calculated in your implementation. Specifically, the time reported at stop differs from the duration retrieved using asset.duration.

Additionally, if pause and resume actions are performed in quick succession, the mismatch tends to increase significantly.

Could you please investigate this issue? You can compare the calculated duration at stop time with asset.duration to identify any discrepancies. I’ve also attached a video for further context.

Thanks!

Screen.Recording.2025-02-25.at.2.mp4

@DanielFRico
Copy link
Contributor Author

@DanielFRico Thank you for updating the PR.

While testing the pause and resume functionality, I noticed a mismatch between the actual recording duration and the duration calculated in your implementation. Specifically, the time reported at stop differs from the duration retrieved using asset.duration.

Additionally, if pause and resume actions are performed in quick succession, the mismatch tends to increase significantly.

Could you please investigate this issue? You can compare the calculated duration at stop time with asset.duration to identify any discrepancies. I’ve also attached a video for further context.

Thanks!

Screen.Recording.2025-02-25.at.2.mp4

@kuldip-simform my first idea was to get currentProgress inside timers. But I found some complications. What do you think? is it possible to get current audio recording duration in that point to get a more accurate value?

@kuldip-simform
Copy link
Contributor

@DanielFRico
I’ve tried multiple approaches, such as calculating the total pause duration and subtracting it from the total time to determine the actual recording duration. However, I still notice some mismatches, and I’m not sure if achieving a 100% accurate match is feasible. Now, I’m at a point where I’m unsure whether we should release this with the slight mismatch for regular users or find a way for them to handle it on their end.

@DanielFRico
Copy link
Contributor Author

@DanielFRico I’ve tried multiple approaches, such as calculating the total pause duration and subtracting it from the total time to determine the actual recording duration. However, I still notice some mismatches, and I’m not sure if achieving a 100% accurate match is feasible. Now, I’m at a point where I’m unsure whether we should release this with the slight mismatch for regular users or find a way for them to handle it on their end.

@kuldip-simform in my opinion I suggest to send this implementation is not perfect but it will add a value for users. Of course, changes and improvements will appear but al least we will have a base of code to make refactors.

@kuldip-simform
Copy link
Contributor

@DanielFRico
We sincerely appreciate your time and effort in reviewing and contributing to this PR. Your input is highly valued, and we’re grateful for your engagement.

Following internal discussions, we’ve determined that any feature we introduce should aim for a high degree of accuracy to ensure the best possible experience for end users. While this implementation certainly offers value, we feel that it does not yet meet the precision standard we aim for.

With that in mind, we’ve decided to keep this PR open for now. This will allow us to gather additional insights, explore potential refinements, and evaluate alternative solutions that could enhance the implementation. If we identify a more precise approach—either from further team discussions or new suggestions—we will revisit and improve this accordingly.

Once again, thank you for your contributions. We appreciate your dedication and look forward to refining this feature together.

@lovelytingy
Copy link

+1. Really need this. Currently I am using modified lib with patches to get the real-time duration.

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.

3 participants