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

Implementing the Audio Playback/Download on the chat #7900

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

panoramix360
Copy link
Contributor

@panoramix360 panoramix360 commented Apr 11, 2024

Summary

This is a work-in-progress PR that implements using the react-native-video library, the playback of audio on the mobile application. It uses the audioOnly prop to load audio using the library.

Features implemented

  • Play/Pause of the media
  • Progress with Cursor
  • Update the progress/time when the audio is playing

Work to be done

  • Download the audio

Ticket Link

mattermost/mattermost#26290

Checklist

  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file updates
  • Have tested against the 5 core themes to ensure consistency between them.

Device Information

This PR was tested on:

  • For now, just Android will test on iOS shortly

Screenshots

Android

image

iOS

image

Release Note

Added the possibility to play and download audios on the chat

@mattermost-build
Copy link
Contributor

Hello @panoramix360,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@mickmister mickmister added the Build Apps for PR Build the mobile app for iOS and Android to test label Apr 15, 2024
@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@panoramix360
Copy link
Contributor Author

panoramix360 commented May 7, 2024

@larkox

Added the possibility for download/preview of the audio file.

But I'm not so sure if this is the right code for it, I don't know if it's already implemented, I did the same way that was implemented on the Documents File.

So if any reviewer can help me clean this code or make it better, I'm available to fix it.

I'll test on iOS and open the PR for review.

Thanks!

@larkox
Copy link
Contributor

larkox commented May 8, 2024

@panoramix360 I gave it a quick glance, and I don't see any big problems in reusing the code on the document file. In order to have things cleaner and get better reusability, I recommend you to create a custom hook.

Something like useFileDownload (or a better name, I am horrible at naming things XD) where you keep and manage all the different states and callbacks needed. That way you can use it in both components.

How does that sound? Is there any other question?

@panoramix360
Copy link
Contributor Author

@larkox makes sense!

I'll create it :D

@panoramix360
Copy link
Contributor Author

panoramix360 commented May 15, 2024

@larkox,

Hook created and migrated in the document_file and audio_file!

Thanks for the tip! Can you take a look now?

I'm having some trouble running on iOS, I don't know what it could be, I managed to do the npm install without problems and pod install as well.

note: Removed stale file '/Users/LucasReis/Library/Developer/Xcode/DerivedData/Mattermost-gbiqdcuxzxywxvfzujpwidajdzki/Build/Intermediates.noindex/Pods.build/Debug-iphonesimulator/JitsiWebRTC.build/Script-46EB2E0002EF80.sh'


--- xcodebuild: WARNING: Using the first of multiple matching destinations:
{ platform:iOS Simulator, id:1EA71F74-860F-4DC5-B8D6-3143E22A2366, OS:17.4, name:iPhone SE (3rd generation) }
{ platform:iOS Simulator, id:1EA71F74-860F-4DC5-B8D6-3143E22A2366, OS:17.4, name:iPhone SE (3rd generation) }
** BUILD FAILED **


The following build commands failed:
	CompileC /Users/LucasReis/Library/Developer/Xcode/DerivedData/Mattermost-gbiqdcuxzxywxvfzujpwidajdzki/Build/Intermediates.noindex/Pods.build/Debug-iphonesimulator/FlipperKit.build/Objects-normal/arm64/FlipperPlatformWebSocket.o /Users/LucasReis/projects/mattermost-mobile/ios/Pods/FlipperKit/iOS/FlipperKit/FlipperPlatformWebSocket.mm normal arm64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler (in target 'FlipperKit' from project 'Pods')
(1 failure)

Any ideas? Maybe updating my branch with main?

Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

Great work! Some changes around the hook.

app/hooks/files.ts Outdated Show resolved Hide resolved
@larkox
Copy link
Contributor

larkox commented May 16, 2024

Any ideas? Maybe updating my branch with main?

Yes. Update your branch with main, run npm clean, and npm i again. That should do it.

@panoramix360
Copy link
Contributor Author

panoramix360 commented May 22, 2024

hey @larkox just updated with the main branch and now it builds the application but after trying to run it says:

3CA4978B6CC")
success Successfully built the app
2024-05-21 22:48:16.304 xcodebuild[32994:8107447]  DVTPlugInQuery: Requested but did not find extension point with identifier 'Xcode.InterfaceBuilderBuildSupport.PlatformDefinition'. This is programmer error; code should only request extension points that are defined by itself or its dependencies.
error Failed to get the target build directory.

Weird, I'm trying to investigate it, thanks!

EDIT: I was able to run it using Xcode directly, I don't know what may be wrong with the npm run ios but ok, hehe

@panoramix360
Copy link
Contributor Author

I found a bug on the iOS version, try to fix it now.

The player is not behaving like in Android, strange, after playing the audio it doesn't pause it anymore.

@panoramix360
Copy link
Contributor Author

Fixed, I need to fix it in a not-so-great way.

Let me know what you think @larkox, it seems to be a known issue that was solved in new versions:
TheWidlarzGroup/react-native-video#3644

@panoramix360 panoramix360 marked this pull request as ready for review May 28, 2024 23:36
@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@larkox
Copy link
Contributor

larkox commented Jun 10, 2024

@panoramix360 There is a thing failing in the tests, related to the en.json file missing the new line at the end of the file. Can you review it?

Once we have the CI all green, we can create a build and send to UX to do the first review on the "front-end" side.

@panoramix360
Copy link
Contributor Author

@larkox done!

Thank you!

@larkox larkox added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Jun 11, 2024
@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@larkox
Copy link
Contributor

larkox commented Aug 19, 2024

@panoramix360 pinging you just because the automation added the stale badge. You are still working on this, right?

@panoramix360
Copy link
Contributor Author

panoramix360 commented Aug 19, 2024

hey!

Yep, I was making some code to the Seek method last week so we can deliver that in this PR as well. I managed to run the iOS build :D

Will post updates this week for that so you guys can test it!

I expect to have more time to contribute, it's being on/off :(

@larkox
Copy link
Contributor

larkox commented Aug 20, 2024

@panoramix360 No problem. Take as much time as you need! Really happy to have you working on this.

As I said, I just ping you because the bot keeps adding the stale badge 😛

@panoramix360
Copy link
Contributor Author

hey @enahum!

Did something change in the way the application renders messages?

The state update of the audio_file.tsx seems a little weird, something like a memo is being used?

Before I did the merge, the duration was shown correctly and the state of the component was being updated correctly, but not anymore.

I'm having problems with the ProgressBar component as well that is rendered there.

My last commit implemented an onSeek method that is called using the PanResponder to get the location of the click so I can build the seek method as requested. But the width is not being updated correctly by the onLoad method.

Do you know something?

I'll continue to investigate but figure I could get some help, thank you!!

@panoramix360
Copy link
Contributor Author

Just fixed the issue with the Progress + Width, I changed the width from a useState to the useSharedValue from react native reanimated, and it worked great!

It seems the duration is being shown correctly just on iOS, so I'm trying to see what the issue could be.

Really weird, because it was working on Android before and not on iOS, and now it's inverted haha

I'll keep you guys posted.

Thank you!

@enahum
Copy link
Contributor

enahum commented Aug 26, 2024

The only thing I would say without even looking at the code is switch to use PanGesture from react-native-gesture-handler instead of the PanResponder

@panoramix360
Copy link
Contributor Author

panoramix360 commented Aug 28, 2024

@enahum it worked!

The seek method is working now, thanks for the tip! This probably fixed all state management to work correctly with react native reanimated!

Screen.Recording.2024-08-27.at.21.30.17.mov

Just one thing is not working when the user clicks on the bar here:
image

It's triggering the touch for advancing to the specific message page:
image

Do you have any ideas of how can I prevent that from happening just inside the progress bar? I probably need to call a stopPropagation or something like that, but I just wondering if you have an easy solution for that.

I'll proceed with the other tasks and investigate that

Thank you!

EDIT: Just fixed the issue above wrapping the entire audio_file component inside a TouchableWithoutFeedback so the user can interact with the ProgressBar freely. Will work on the other smaller issues now.

@panoramix360
Copy link
Contributor Author

hey @larkox and @abhijit-singh

I updated the PR to have the dragging on the Seek method, so the user now can drag the cursor to select a specific second.

It's ready to be tested.

@abhijit-singh

Regarding your points:

On Android, the audio duration showed a very long incorrect string until I played the file. Once you click on play, it shows correctly.

I was not able to reproduce this issue, can you provide the audio file that you used in this case?
Can you test it again? The library that I used to build this was updated so maybe this issue is fixed.

I noticed that it takes a few seconds for the duration to show on the card after opening the channel, both on Android and iOS. Is there a way to optimize this?

Can you try this one again? As I said above, the library was updated to the latest version, let's see if this is optimized more.
But if not, I can take a look to try to optimize it.

I was expecting to be able to scrub through the audio file by tapping and dragging the little circle on the progress bar. Is that functionality not implemented?

Done!

A small one, but on iOS the color of the download icon in the card seems black. Can we match it to the grey we have on Android?

Done!

Let me know about your review!

Excited to have that merged :)

@larkox
Copy link
Contributor

larkox commented Sep 5, 2024

@panoramix360 There seems to be some test failing. Can you look into that?

@panoramix360
Copy link
Contributor Author

panoramix360 commented Sep 5, 2024

Can you try again?

I fixed in the code.

@abhijit-singh abhijit-singh added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Sep 11, 2024
@larkox
Copy link
Contributor

larkox commented Sep 16, 2024

@abhijit-singh Here should be the builds for this:
https://community.mattermost.com/core/pl/4txx9r7j13djj8a6h3yzpe7wzy
https://community.mattermost.com/core/pl/m3mjca7enfnddx65egq7xxw7he

Copy link

@abhijit-singh abhijit-singh left a comment

Choose a reason for hiding this comment

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

Thanks @panoramix360 and @larkox. I still see a few issues, more prominently on Android. Attaching recordings of the test on both Android and iOS. The first file in the recording is recorded and sent from iOS (M4A format) whereas the file sent second is recorded and sent from Android (AAC format).

Also thanks for taking the time to add in the scroll functionality! It seems a bit laggy right now. Is there any way to optimize that? Might be okay doing that as a follow up even.

Android recording:
https://github.com/user-attachments/assets/b8c4bc97-cc20-43c1-af32-9a489641f4bb

List of issues observed on Android:

  1. The second file in the attached recording never shows the duration. It plays normally and the duration increments as expected when played, but it does not show the duration upfront. I can see the duration in iOS though.
  2. If you tap on the message containing the audio to go to the thread view, the duration flickers to a long string for a second.
  3. Also in the thread view, neither files show the actual duration in the default view and instead show 00:00. It should be visible ideally even before the user clicks on play.
  4. On Android, I was not able to use the scroll functionality for the AAC file which was recorded and shared from that same device. I was able to scroll on both files on iOS though.

iOS recording:
https://github.com/user-attachments/assets/5cd36611-5ee9-4de7-86c4-5b2352d56705

  1. While most things are working as expected, it takes significantly longer to load the duration here. Especially when you go into the thread view, it takes a few seconds before the duration of the file loads.
  2. The scroll dot can seemingly be dragged off the screen which also makes the duration value negative. Ideally dragging beyond the scroll line or the file container should not be allowed. If we can't prevent that, we should try to bring it back to the start immediately even if someone drags it away. This seems to be happening on Android as well.

@panoramix360
Copy link
Contributor Author

Thanks for the feedback @abhijit-singh!

I'll take a look at those and fix it accordingly, I think overall it will be a polished and nice experience if we manage to solve all of this.

Some of your issues are probably something related to the library we are using to load the audios, but I'll take a look at open issues there, like the duration issue on Android. The experience on iOS seems to be better, no idea why, will investigate it as well.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: UX Review Requires review by a UX Designer Build Apps for PR Build the mobile app for iOS and Android to test Contributor release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants