[native] Migrate VideoPlaybackModal to Reanimated V2 API
ClosedPublic
Actions

Authored by angelika on Jan 10 2025, 6:17 PM.

Details

Reviewers

bartek
tomek

Commits

rCOMM103a860ae36c: [native] Migrate VideoPlaybackModal to Reanimated V2 API

Summary

https://linear.app/comm/issue/ENG-8154/convert-videoplaybackmodal-to-reanimated-2-syntax

I tried to split this diff into parts somehow, but it doesn’t look like it’s possible: if one part is migrated then any parts that use or are used by this parts must be migrated as well.

I used a new gesture-handler API, because it works well with Reanimated v2 API and allows for a much shorter and readable code. However there are some problems with flow definitions and it’s not a quick fix so created a task for that: https://linear.app/comm/issue/ENG-10079/add-react-native-gesture-handler-v2-flow-definitions and for now I added FlowFixMe comment
Instead of TapGestureHandler there is now a GestureDetector that allows us to set a callback (onEnd) if a tap gesture ends so there is no need for checking if gesture has ended with gestureJustEnded function and no need for tracking the last tap coordinates (they’re just passed via callback)
toggleControls: I simplified it by using useAnimatedReaction but it does the exact same thing: tracks ceil(activeControlsOpacity) and if ceiling changes it calls enable/disable controls
animateTowards: this function is only used in this component. As far as I understand it, it animates a reanimated value towards a given target value with linear interpolation. And if for example the current value is 0.5 and it animates toward 1, it will animate in half of full animation length, so the speed of the animation is always the same. It does that by looking at how much time passed from the last frame and increasing/decreasing value accordingly.  In this case the animation is only 150ms so it will not be noticeable anyway (I checked it, I was clicking quickly in my simulator but there is no noticeable difference), so I deleted this function and just used withTiming. We can implement the behavior from animateTowards by calculating the duration based on current value and setting easing to linear but in my opinion it’s not worth it.
updates and updated* variables: toggleControls is now handled by useAnimatedReaction so it can be removed from updates. As for setting curBackdropOpacity to progressiveOpacity I don’t know what the author meant here: progressiveOpacity was used just to update curBackdropOpacity so we might rename it to curBackdropOpacity and have the same effect. And so we can get rid of updates and updated* values.
All other changes are straighforward: changing useValue to useSharedValue, using useAnimatedStyle and calculate stuff there and in worklets instead of using animated nodes

Before:

After:

Depends on D14203

Test Plan

Open a video in VideoPlaybackModal. Verify that:

the video has a correct size and position
the video position and scale is animated when it opens and closes
the video controls are hidden and animated when user clicks outside them
the video controls are shown and animated when user click again
user can interact with video controls (play/pause video etc.)
user can't click the controls when they're hidden

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

angelika created this revision.Jan 10 2025, 6:17 PM

Herald added a subscriber: ashoat. · View Herald TranscriptJan 10 2025, 6:18 PM

Harbormaster completed remote builds in B33222: Diff 46644.Jan 10 2025, 6:46 PM

angelika requested review of this revision.Jan 10 2025, 6:46 PM

However there are some problems with flow definitions and it’s not a quick fix so created a task for that: https://linear.app/comm/issue/ENG-10079/add-react-native-gesture-handler-v2-flow-definitions and for now I added FlowFixMe comment

Rather than a $FlowFixMe, please type the surface you're using. No need to type the whole thing – only the parts that you're importing. I think it's better to address this "piecemeal" as you need some part of the library, rather than deferring it until we have time for typing the whole thing (which will probably never happen)

I tried to split this diff into parts somehow, but it doesn’t look like it’s possible: if one part is migrated then any parts that use or are used by this parts must be migrated as well.

High level, while creating the diffs, we should prioritize how easy it is to review them. Sometimes it makes sense to create diffs that don't work just to make them easier to review (e.g. we're doing that as a part of RN upgrades). It sounds like it could be the case here, but I'm not sure.

for now I added FlowFixMe comment

It's usually better to start with the types and then use them. This approach should be helpful for both the reviewer and the author because they can rely on the type checks. This also makes it less likely to forget about FlowFixMe annotation. So please add the definitions in a diff(s) that is sequenced before this one.

It is great that you explained all the unclear changes! To make the explanations more convenient, you can consider adding them as inline comments next to the section they cover.

In diffs like this, it is helpful to see a video capturing how the animations look like. Ideally, next to a video with the same interactions, but before the changes from the diff.

Overall, this diff makes sense and I don't think any significant changes need to be made! I'm requesting changes because we should see the videos capturing the old and the new animations before landing this diff.

native/media/video-playback-modal.react.js
234 ↗	(On Diff #46644)	Is this code correct? What if a `tap` happened before `controlsShowing` reached `0` or `1`? Would that mean that we're animating towards a fractional value? Or maybe we're guaranteed that it has only discrete values?

This revision now requires changes to proceed.Jan 14 2025, 4:52 PM

Rebase & remove flow fix me

angelika planned changes to this revision.Jan 14 2025, 7:42 PM

angelika edited the summary of this revision. (Show Details)

angelika added a parent revision: D14203: [native] Add Flow types for react-native-gesture-handler v2 basic gestures.

Harbormaster completed remote builds in B33240: Diff 46679.Jan 14 2025, 7:59 PM

Change controlsShowing to boolean

angelika edited the summary of this revision. (Show Details)Jan 14 2025, 10:42 PM

angelika added inline comments.Jan 14 2025, 10:45 PM

native/media/video-playback-modal.react.js
234 ↗	(On Diff #46644)	It is correct, the `controlsShowing` is never animated and it's set to `0`, `1` or `1-controlsShowing` so it will be always `0` or `1`. The `activeControlsOpacity` is animated and can be fractional. But for readability I changed `controlsShowing` type to boolean.