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: video player #29

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

feat: video player #29

wants to merge 10 commits into from

Conversation

account0123
Copy link
Contributor

Features

Video player

In message, video opens as preview. Full-screen control activates full mode (it pauses preview and opens video on modal).
Modal contains same header as image viewer modal, with similar actions.
I haven't implemented video player in MessageEmbed yet.

Audio player

It was hard to find an audio player component. I tried installing a voice message player but installation failed. Then, I built my own component using TrackPlayer, a service useful for music queues, and I implemented a method to ensure that only one audio is played at a time.
Some known bugs:

  • No communication from remote controls (on notifications) to audio attachment. But background track can be stopped easily.
  • On player setup, the app could sometimes quit. This behavior happens once without explanation.

Please make sure to check the following tasks before opening and submitting a PR

  • I understand and have followed the contribution guide
  • I have tested my changes locally and they are working as intended
  • These changes do not have any notable side effects on other Revolt projects

I like using !1, !0 and void 0, but thats not professional
Audio players can sync on android background audio service, it needs background task config to completely work.
I cannot explain why, but the app crashes once with no errors. If it gives problems, this feat can be removed.
+ exports/imports from index
@Rexogamer
Copy link
Member

out of curiosity, what failed when trying to install the first library?

@Rexogamer Rexogamer marked this pull request as draft September 19, 2023 21:16
@Rexogamer Rexogamer added the enhancement New feature or request label Sep 19, 2023
@account0123
Copy link
Contributor Author

$ yarn add @carchaze/react-native-voice-message-player
<--- JS stacktrace --->

FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory
 1: 0x55916db37f30 node::Abort() [node]
 2: 0x55916da30f4b  [node]
 3: 0x55916dd538a0 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [node]

@Rexogamer
Copy link
Member

ahhh. there seems to be an issues where the latest releases of React Native - and, by extension, packages that depend on them - can't be installed due to a Yarn bug; your alternate solution should be fine (although it'd be preferable to fix the known issues first), and the seeming solution to this issue is to switch to Yarn Berry (I'll wait for v4 to release) so no worries

@Rexogamer
Copy link
Member

Conflicting

@account0123
Copy link
Contributor Author

Hi! As you said before I tried to install (in new branch) @carchaze/react-native-voice-message-player. Installation was successful, but in execution time it has a very weird bug related to react-native-sound dependency. I followed steps but nothing worked so I give up trying to install that library.
On other hand I am thinking of creating my own native sound module instead of using react-native-track-player because it's not necessary to use background audio service that this dependency requires. So in the new branch I am going to experiment with audio native modules or creating my own module and create a PR when everything works fine.
TL;DR: I will fix conflicts keeping only video components. Audio components will be in another PR.

@Rexogamer
Copy link
Member

Rexogamer commented Oct 30, 2023

I will also mention that you might be able to follow the steps here if you haven't already - however, if you have to no avail, no worries. I think splitting this PR would be a good idea regardless; take as much time as you need ^^

@account0123
Copy link
Contributor Author

account0123 commented Oct 30, 2023

hehe that task doesnt exist in the gradle project, then I manually deleted ~/.gradle/cache.
Same error.

@Rexogamer Rexogamer changed the title feat: video player & audio player feat: video player Nov 3, 2023
Copy link
Member

@Rexogamer Rexogamer left a comment

Choose a reason for hiding this comment

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

conflicts + a couple remnants; will review the main bit once this is done

@@ -4,3 +4,5 @@ export {MessageEmbed} from './MessageEmbed';
export {MessageReactions} from './MessageReactions';
export {PlatformModerationMessage} from './PlatformModerationMessage';
export {ReplyMessage} from './ReplyMessage';
export {VideoEmbed} from './VideoEmbed';
export {AudioPlayer} from './AudioPlayer';
Copy link
Member

Choose a reason for hiding this comment

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

With the splitting of this PR, this needs to be removed

@@ -1,4 +1,5 @@
import {ToastAndroid} from 'react-native';
import TrackPlayer, {Event} from 'react-native-track-player';
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants