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

feat: adds creation, saving and playback of audio #762

Merged
merged 48 commits into from
Oct 10, 2024

Conversation

cimigree
Copy link
Contributor

@cimigree cimigree commented Sep 26, 2024

closes #360

Description

Adds the ability to create an audio file from an observation
Adds the necessary files and logic so that when you hit "stop" on an audio recording, you are given the Playback option to be able to hear what you just recorded.
Continues to address issue #360
Used this PR that Andrew created
Here are the designs
Uses some code from SWM

Instructions
Make sure to add this to your env file: EXPO_PUBLIC_FEATURE_AUDIO=true
Go to create an observation
Hit the microphone
Grant permission to record audio
You will be taken to the create audio recording screen
Watch the screen change color and the time pass and the countdown change.
It will stop automatically at 5 minutes
Hit the stop button to stop recording.
You can play, stop, play, stop, and then when you get to the end it resets to the beginning. Hope that is right. Designs weren't clear.
At that point, the audio is added to state.
You can delete it with the trashcan.
If you hit the X in the top left, the audio is added to the draft observation, the success modal opens, and you are given the option to return to the editor or record another audio, which I wired up to work.
Once you save the Observation, the audio file or files is added to the observation.
You can't see these yet... That will be another PR!

@cimigree cimigree changed the title feat: adds saving and playback of audio [WIP] feat: adds saving and playback of audio Sep 26, 2024
@cimigree cimigree changed the title [WIP] feat: adds saving and playback of audio feat: adds saving and playback of audio Sep 30, 2024
@cimigree
Copy link
Contributor Author

cimigree commented Oct 1, 2024

@ErikSin Ready for continuation of review. I changed the multi ternary to an if statement. I find it a little more readable. I moved the createdAt to happen when the recording is saved. I had to keep the header in RecordingIdle because it is needed when you go to record another audio recording after you have recorded the first one. Got rid of the unlink function because resetting seems to do the trick upon deletion as you noticed.

Copy link
Contributor

@ErikSin ErikSin left a comment

Choose a reason for hiding this comment

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

Some adjustment but the overall architecture seems sound. I am a little worried though about the fact that there is no error handling, so I think we should address that in a follow up PR.

src/frontend/screens/Audio/ContentWithControls.tsx Outdated Show resolved Hide resolved
src/frontend/screens/Audio/Playback.tsx Outdated Show resolved Hide resolved
src/frontend/screens/Audio/constants.ts Outdated Show resolved Hide resolved
src/frontend/screens/Audio/useAudioPlayback.ts Outdated Show resolved Hide resolved
Comment on lines +28 to +32
.then(({sound, status}) => {
if ('error' in status && status.error) {
console.error('Error while creating audio playback', status.error);
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

as a follow up pr, i think we should handle audio errors (because we do not handle them in the UI at all)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

…cation. Adjusts playback location. Removes isready in state.
@cimigree cimigree requested a review from ErikSin October 2, 2024 17:08
Copy link
Contributor

@ErikSin ErikSin left a comment

Choose a reason for hiding this comment

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

when i delete a recording, the bottom sheet is not closing

@cimigree
Copy link
Contributor Author

cimigree commented Oct 2, 2024

when i delete a recording, the bottom sheet is not closing

Hm. I can't figure out why that would be, can't replicate.

Here is what it looks like on my Pixel. Is this what you are talking about?

deletionvideo.mp4

@cimigree cimigree requested a review from ErikSin October 2, 2024 20:08
@ErikSin
Copy link
Contributor

ErikSin commented Oct 2, 2024

when i delete a recording, the bottom sheet is not closing

Hm. I can't figure out why that would be, can't replicate.

Here is what it looks like on my Pixel. Is this what you are talking about?
deletionvideo.mp4

yeah, i can recreate it sometimes....im just trying to figure out why its only happening sometimes

@cimigree cimigree changed the title feat: adds saving and playback of audio feat: adds creation, saving and playback of audio Oct 7, 2024
@cimigree cimigree requested a review from ErikSin October 8, 2024 13:35
Copy link
Contributor

@ErikSin ErikSin left a comment

Choose a reason for hiding this comment

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

Sorry, i just noticed the use of statusBar. the SWM team was using them, and it was causing a flickering, so I think we should just remove them entirely

src/frontend/screens/Audio/CreateRecording/index.tsx Outdated Show resolved Hide resolved
@@ -6,6 +6,8 @@ import {CustomHeaderLeft} from '../../sharedComponents/CustomHeaderLeft';
import {StatusBar} from 'expo-status-bar';
Copy link
Contributor

Choose a reason for hiding this comment

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

this StatusBar is causing a flicker in the screen when you navigate back. Notice in the video how the screen jumps up and down when navigating between the screen. I think we can just get rid of it completely. I also think that Audio/CreateRecording/index should become the new Audio/index

Screen.Recording.2024-10-08.at.8.56.54.AM.mov

Copy link
Contributor Author

@cimigree cimigree Oct 8, 2024

Choose a reason for hiding this comment

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

@ErikSin, There is going to be something else in Audio/index in the next PR related to Playback from the Observation. Is it ok if I leave it as is? Not the Status Bar... Having the two files

src/frontend/sharedComponents/ActionRow.tsx Outdated Show resolved Hide resolved
@cimigree cimigree requested a review from ErikSin October 8, 2024 18:03
Copy link
Contributor

@ErikSin ErikSin left a comment

Choose a reason for hiding this comment

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

As a follow up, lets add the metadata to the blob

@cimigree cimigree merged commit ffb4809 into develop Oct 10, 2024
3 checks passed
@cimigree cimigree deleted the feat/add-audio-save-playback branch October 10, 2024 20:35
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.

New audio recording
2 participants