[New Arch] Android Fabric #3204

yungblud · 2023-08-11T13:29:33Z

Thanks for opening a PR!
Since this is a volunteer project and is very active, anything you can do to reduce the amount of time needed to review and merge your PR is appreciated.
The following steps will help get your PR merged quickly:

Update the documentation

For now, I don't know what to be added to README.md
Let me know if you want something to add.

Update the changelog

implemented android fabric

Provide an example of how to test the change

cd examples/FabricExample
yarn
yarn android

OR if above command doesn't work, you can directly

open Android Studio.
Sync gradle
Run

If you see build failure with Android Studio, you can try open -a /Applications/Android\ Studio.app

Focus the PR on only one area

Testing multiple features takes longer than isolated changes and if there is a bug in one feature, prevents the other parts of your PR from getting merged until it gets fixed.
If you're touching multiple different areas that aren't related, break the changes up into multiple PRs.

Describe the changes

In this PR, we've implemented android fabric.
So there are lots of file changes in android directory.
We implemented Kotlin for fabric android.

Major changes

We divided directory structure with src/fabric and src/oldarch on android side.
migrated fabric events
added finished (boolean) parameter on seek event for detecting when seek is finished.
implemented typescript on javascript side.
implemented fabric component view on javascript side.
migrated JS to TS
implemented codegen types with typescript

Deprecated methods (because of codegen doesn't support array callback parameters, see this issue)

onTimedMetadata?: (e: OnTimedMetadataData) => void //Android, iOS
onAudioTracks?: (e: OnAudioTracksData) => void // Android
onTextTracks?: (e: OnTextTracksData) => void //Android
onVideoTracks?: (e: OnVideoTracksData) => void //Android

React Native New Architecture's codegen doesn't support array callback parameters for native methods.

// @todo: fix type. for now react native doesn't support array codegen type for native event
export type OnAudioTracksData = Readonly<{}>
// export type _OnAudioTracksData = Readonly<{
//   audioTracks: ReadonlyArray<Readonly<{
//     index?: Int32
//     title?: string
//     language?: string
//     bitrate?: Float
//     type?: string
//     selected?: boolean
//   }>>
// }>

You can see this kind of comments for above methods on src/fabric/VideoNativeComponent.tsx.

There are some meaningless file changes.

examples/basic/src/VideoPlayer.ios.tsx
ios/Video/DataStructures/DRMParams.swift
ios/Video/Features/RCTResourceLoaderDelegate.swift
ios/Video/RCTVideoManager.swift
These files are changed because of simple lint issue. So you can skip that files.

…en issue)

freeboub · 2023-12-19T21:20:32Z

android/src/oldarch/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java

    private static final String PROP_DRM_LICENSESERVER = "licenseServer";
    private static final String PROP_DRM_HEADERS = "headers";
    private static final String PROP_SRC_HEADERS = "requestHeaders";
    private static final String PROP_RESIZE_MODE = "resizeMode";
    private static final String PROP_REPEAT = "repeat";
    private static final String PROP_SELECTED_AUDIO_TRACK = "selectedAudioTrack";
-    private static final String PROP_SELECTED_AUDIO_TRACK_TYPE = "type";


these changes change the api ?!

3f12e34

Please look at this commit.
It would change api as you said.
But it's necessary.

For similar case, we can find out "selectedAudioTrack".
This naming change is needed because of CodeGen with "WithDefault".
We use WithDefault type for codegen, but with it, we cannot declare same naming of WithDefault.

freeboub · 2023-12-19T21:21:31Z

android/src/oldarch/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java

+     * @return A HashMap containing the data that was in the ReadableMap.
+     * @see 'Adapted from https://github.com/artemyarulin/react-native-eval/blob/master/android/src/main/java/com/evaluator/react/ConversionUtil.java'
+     */
+    public static Map<String, String> toStringMap(@Nullable ReadableMap readableMap) {


This function can be extracted to a helper function !

Yeah it's true.
But we're not using it anywhere in the codebase.
So if you are okay, I will remove it.
But it's okay to move that function to another directory such as "helper"

freeboub · 2023-12-19T21:22:15Z

package.json

    "description": "A <Video /> element for react-native",
-    "main": "lib/index",


I think this is an error here ?

Nope. It's correct way.
We should export "src/index.ts"

Oh, my bad.
I didn't know that you export build files.
yep, this should be changed to "lib/index"

a813592

I've finished !

freeboub · 2023-12-19T21:26:01Z

src/fabric/VideoNativeComponent.tsx

@@ -0,0 +1,322 @@
+import codegenNativeComponent from 'react-native/Libraries/Utilities/codegenNativeComponent';


Is there a way to factorize this file with VideoNativeComponent.ts already available on master ?

Yep, I've finished that task.
We won't use 'src/fabric' folder anymore.

freeboub · 2023-12-21T22:09:40Z

@yungblud thank you very much for the maintenance on this PR 🙏
If you want to extract ts change sin a separated PR it would allow to merge changes step by step !
I would appreciate if possible ...

freeboub · 2023-12-21T22:10:20Z

android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java

@@ -1431,8 +1431,9 @@ public void onPlaybackParametersChanged(PlaybackParameters params) {

    @Override
    public void onVolumeChanged(float volume) {
-        eventEmitter.volumeChange(volume);
-    }
+//        todo


I think onVolumeChanged and onAdError are new APIs
So I have to implement it for Fabric.
I will let you know when it's implemented.

freeboub · 2023-12-21T22:10:36Z

android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java

-            }
-        }
-        eventEmitter.timedMetadata(metadataArray);
+//        ArrayList<TimedMetadata> metadataArray = new ArrayList<>();


commented code here also

Yep, I cleaned it.

yungblud · 2024-01-02T06:16:20Z

@freeboub Hello. No problem.
So do you want more splitted PR for implementing android fabric?
I think we can start reviews from now on on this PR.

yungblud · 2024-01-12T05:18:21Z

@freeboub
Hello, I think we should not use enum typescript types on codegen codebase (like VideoNativeComponent.ts)
Because of codegen.

As you can see, codegen cannot handle with typescript enum type.
So we have to switch enums to just object values.

…ndroid

yungblud · 2024-01-12T06:04:42Z

@freeboub hey, I think I've almost finished your review.
So can you check about them?
Thanks.

yungblud · 2024-01-19T07:20:29Z

@freeboub hey, I splitted codegen support PR on this
Let's start from that PR.

yungblud and others added 30 commits August 11, 2023 16:26

feat: ⚡ added gitignore

0aceefb

feat: ⚡ turn on newarch enabled on example app

649b2e9

feat: ⚡ setup example app new arch

5c371bb

feat: ⚡ setup android example app for fabric android

8782d7a

feat: ⚡ setup kotlin

6bd2489

feat: ⚡ migrate to fabric

d851d56

refactor: js to ts

ddcb252

chore: update pacakge

37c5a02

fabric support for js

43a8645

feat: ⚡ added gitignore

8ba7950

feat: modified seek, headers

68320b0

fix: fixed drm headers type android

153186d

chore: modified type

797f527

feat: modified package json

ace7e55

refactor: remove legacy video js

70afe83

fix: fixed android fabric include isssue

dbec302

feat: modified seek, headers

92448ba

fix: fixed drm headers type android

5db9e7e

refactor: refactored properties

63f4131

feat: replaced null to map

20842fb

feat: temporarily remove put array on event callback parameter (codeg…

f3948a5

…en issue)

fix: fix selected props typo

dcee4ff

fix: add useExternalGetLicense field

b8b6f53

feat: changed prop name

54f4fdc

fix: fixed drmType value

310b4e2

fix: upgrade deprecated kotlin plugin

80f56ca

fix: add onDropViewInstance method

e53ad84

fix: fixed endTime

0df0282

feat: video seek param update

33e9144

feat: video seek event android

ebae8eb

freeboub reviewed Dec 19, 2023

View reviewed changes

yungblud added 8 commits December 21, 2023 14:56

chore: lint fix

fe890b5

chore: modified tsconfig.json

5cb06e5

chore: modified Podfile.lock

dbca3ec

fix: fixed VideoEventEmitter with fabric

72de90c

feat: modified typescripts

9bfe250

feat: modified android events

f105bf5

fix: synced build.gradle Fabric example

9945637

feat: modified react exo player android

03b0ab2

freeboub reviewed Dec 21, 2023

View reviewed changes

yungblud added 4 commits January 2, 2024 14:46

fix: fixed basic example metro.config.js

7863d9c

fix: fixed VideoEventEmitter import class, metadata array

61cb62e

chore: modified tsconfig.json

58bf621

feat: implemented receiveCommand

b57185d

yungblud added 5 commits January 12, 2024 13:54

feat: removed unnecessary VideoResizeMode.ts

32eb9d2

refactor: refactored on timed metadata event

b3a1857

chore: removed unnecessary import

de4bfa6

feat: modified codegen naming property of selected video track

3f12e34

feat: modified package.json main property

a813592

Merge remote-tracking branch 'upstream/master' into feature/newarch-a…

52a56a0

…ndroid

yungblud requested a review from freeboub January 12, 2024 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New Arch] Android Fabric #3204

[New Arch] Android Fabric #3204

yungblud commented Aug 11, 2023 •

edited

Loading

freeboub Dec 19, 2023

yungblud Jan 12, 2024

freeboub Dec 19, 2023

yungblud Jan 12, 2024

freeboub Dec 19, 2023

yungblud Jan 2, 2024

yungblud Jan 2, 2024

yungblud Jan 12, 2024

freeboub Dec 19, 2023

yungblud Jan 12, 2024

freeboub commented Dec 21, 2023

freeboub Dec 21, 2023

yungblud Jan 12, 2024

freeboub Dec 21, 2023

yungblud Jan 12, 2024

yungblud commented Jan 2, 2024

yungblud commented Jan 12, 2024 •

edited

Loading

yungblud commented Jan 12, 2024

yungblud commented Jan 19, 2024

		"description": "A <Video /> element for react-native",
		"main": "lib/index",

		@@ -0,0 +1,322 @@
		import codegenNativeComponent from 'react-native/Libraries/Utilities/codegenNativeComponent';

[New Arch] Android Fabric #3204

Are you sure you want to change the base?

[New Arch] Android Fabric #3204

Conversation

yungblud commented Aug 11, 2023 • edited Loading

Update the documentation

Update the changelog

Provide an example of how to test the change

Focus the PR on only one area

Describe the changes

Major changes

Deprecated methods (because of codegen doesn't support array callback parameters, see this issue)

There are some meaningless file changes.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freeboub commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yungblud commented Jan 2, 2024

yungblud commented Jan 12, 2024 • edited Loading

yungblud commented Jan 12, 2024

yungblud commented Jan 19, 2024

yungblud commented Aug 11, 2023 •

edited

Loading

yungblud commented Jan 12, 2024 •

edited

Loading