Fix video sharing #38407

Skalakid · 2024-03-15T16:40:37Z

Details

This PR reenables video sharing feature. With these changes videos from inside the chat will be sharing their state with the player that is inside the attachment carousel.

Fixed Issues

$ #36873
PROPOSAL: #36873 (comment)

Tests

Button linking:

Go to prerequisite chat
Play&Pause the video
Tap on the expand button on the video preview
Verify if video player showed there have the same state as the on from chat
Play&Pause the video
Exit from the attachment view via clicking X button top right
Try to Play&Pause the video in preview mode

Window dimension changing:
Web & desktop:

Open video in attachment carousel
Make your app widow smaller
Enter full screen
Exit fullscreen
Verify if video continue playing

Native devices:

Open video in attachment carousel
Enter full screen
Change device orientation
Exit fullscreen
Verify if video continue playing and if app changed layout

Verify that no errors appear in the JS console

Offline tests

Same as Tests

QA Steps

Same as Tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

…playing

Kicu

I tried to review this and left some of my comments and questions. As I'm not super familiar with the app code yet, maybe some of the questions don't make much sense.

I don't see any huge red flags here. BasePlayer itself has a lot of complex logic in hooks so Im not sure I understand how everything works there - but like I said Im not yet familiar with this.

Left you some naming suggestions and some questions

src/components/VideoPlayerContexts/types.ts

src/components/VideoPlayerContexts/FullScreenContext.tsx

src/components/VideoPlayer/BaseVideoPlayer.js

src/hooks/useWindowDimensions/index.ts

Kicu

Alright looks pretty good with my limited knowledge about video player 👍

melvin-bot · 2024-03-20T13:50:18Z

@ishpaul777 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

ishpaul777 · 2024-03-21T13:08:05Z

@Skalakid Can you please resolve conflicts

ishpaul777 · 2024-03-22T11:59:30Z

nvm, above is not related ⬆️

ishpaul777 · 2024-03-22T12:04:41Z

bug: Not able to pause video (by clicking on video itself) in full screen it starts playing as soon as i pause

Screen.Recording.2024-03-22.at.5.32.18.PM.mov

ishpaul777 · 2024-03-22T12:18:41Z

Bug: there's a loading indicator and video restarts when changing windowsize after a breakpoint in attachment carousel

Screen.Recording.2024-03-22.at.5.45.53.PM.mov

bug: video stops playing in chat after a breakpoing in chat

Screen.Recording.2024-03-22.at.5.54.32.PM.mov

Bug?: it takes few second to play video when video timing is backed (it seems it redownloads the video everytime)

Screen.Recording.2024-03-22.at.6.22.57.PM.mov

ishpaul777 · 2024-03-22T13:01:25Z

thats it for now @Skalakid can you address above comments once you got sometime, thank you!

Skalakid · 2024-03-25T09:46:10Z

Bug: there's a loading indicator and video restarts when changing window size after a breakpoint in attachment carousel

I don't think it is a bug. Video reloads when changing window size and hitting breakpoint because on small screens, the in-chat video player isn't available and we can't share video elements that don't exit. So it is like natural behaviour because we have to stop sharing video elements and render separate players directly inside the attachment carousel

bug: video stops playing in chat after a breakpoing in chat

Same as above. In-chat video player isn't available on small screen devices. That why we are changing currently playing video to video thumbnail button that opens attachment carousel with the video inside

Skalakid · 2024-03-25T10:18:44Z

Bug?: it takes few second to play video when video timing is backed (it seems it redownloads the video everytime)

Hmm, I think is not related to this PR. The same thing appears on main and probably that how native android player works. It can be connected how our backend returns video to our app. It accepts video ranges and probably Android native video player doesn't load entire file, only parts of it. After some time cached parts of the video are removed.
I will investigate this issue more, however it should block this PR, since it is not connected to it

Skalakid · 2024-03-25T10:21:44Z

@ishpaul777 I've added review changes and answered to you comments, let me know what do you think :D

Skalakid · 2024-03-26T19:02:37Z

@ishpaul777 I resolved conflicts

ishpaul777 · 2024-03-26T19:08:15Z

Thanks, Did breif testing for web desktop, it seems to work well, i'll priortize a full review for tomorrow its EOD for me, sorry!

Skalakid · 2024-03-27T07:08:28Z

Okay, no problem. Thanks

ishpaul777

lgtm!

melvin-bot · 2024-03-28T08:49:44Z

@lakchote looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2024-03-28T08:55:25Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-03-29T02:27:08Z

🚀 Deployed to staging by https://github.com/lakchote in version: 1.4.58-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

lakchote · 2024-04-01T06:09:20Z

Not an emergency, reviewer has completed all the checklist boxes. Removing label.

OSBotify · 2024-04-02T11:03:24Z

🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.58-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

tienifr · 2024-05-22T06:53:40Z

src/components/VideoPlayer/BaseVideoPlayer.js

    // update shared video elements
    useEffect(() => {
-        if (shouldUseSharedVideoElement || url !== currentlyPlayingURL) {
+        if (shouldUseSharedVideoElement || url !== currentlyPlayingURL || isFullScreenRef.current) {


Hi @Skalakid I wonder why do we need to add isFullScreenRef.current?

isFullScreenRef is responsible for locking the video player when screen orientation changes in full-screen mode. It prevents video players from changing their state because of device rotation or window size changes. Changing state causes the full screen to dismiss. We need it here to block updating currently shared video element

Can you help me reproduce the bug if we remove isFullScreenRef.current? Thanks

Hi, @Skalakid
I'm also curious about test steps that cause unexpected errors when we remove isFullScreenRef.current.

We try to remove isFullScreenRef.current only in this useEffect if statement.

@Skalakid
Thank you for your confirm. I can't reproduce on my end.
Could you please let me know if this issue is reproducible on native devices?

Confirmed again, it's not reproducible on my end.

We try to remove isFullScreenRef.current only in this useEffect if statement.

@tienifr oh thanks for information, in the video above I deleted isFullScreenRef in whole BaseVideo Player file 😅 So if it works after deleting isFullScreenRef.current in useEffect, just do the test steps from this issue description and if everything works you can remove it

@Skalakid
thank you for your check.

Skalakid added 9 commits March 15, 2024 13:49

Add window dimensions blocking POC

b307865

Enhance bug fix code structure

344c68d

Merge branch 'main' into @Skalakid/fix-video-element-sharing

deec3ab

Fix TS errors

4188a33

Fix video sharing bug when changing window dimensions while video is …

29a802a

…playing

Restore code that is used in other places

4bbae6c

Fix video starting playing when exiting fullscreen with paused video

cab0496

Change fix structure

f54eb12

Merge branch 'main' into @Skalakid/fix-video-element-sharing

05ed715

francoisl mentioned this pull request Mar 18, 2024

Chat - Video is restarted when opened on attachment window #37853

Closed

6 tasks

Skalakid added 5 commits March 19, 2024 09:55

Add locked screen dimensions updating when orientation changes

1f04efc

Enhance device orientation checking

82ca579

Fix video pausing when dismissing fullscreen mode

59ff4a2

Change orginal size detction

4e06538

Add comment

1ca5515

Kicu reviewed Mar 19, 2024

View reviewed changes

Skalakid added 4 commits March 20, 2024 10:08

Merge branch 'main' into @Skalakid/fix-video-element-sharing

72b0fb1

Merge branch 'main' into @Skalakid/fix-video-element-sharing

df3db7d

Add review changes

ac35413

Fix fullscreen mode bug

3f75fe3

Kicu approved these changes Mar 20, 2024

View reviewed changes

Skalakid marked this pull request as ready for review March 20, 2024 13:50

Skalakid requested a review from a team as a code owner March 20, 2024 13:50

melvin-bot bot requested review from ishpaul777 and removed request for a team March 20, 2024 13:50

This was referenced Mar 21, 2024

[$500] Web - Video - play speed after returning from thread does not update #37722

Closed

[HOLD for payment 2024-04-09] [HOLD for payment 2024-03-13] [$500] Video - Play&Pause buttons don't function in preview mode if expanded the video before #36873

Closed

Merge branch 'main' into @Skalakid/fix-video-element-sharing

5b35c6e

Skalakid added 3 commits March 22, 2024 15:59

Merge branch 'main' into @Skalakid/fix-video-element-sharing

328fbd8

Add review changes

e75f1c4

Fix video pausing in fullscreen mode

75896d2

Skalakid requested a review from ishpaul777 March 26, 2024 18:51

Merge branch 'main' into @Skalakid/fix-video-element-sharing

9facbb7

Merge branch 'main' into @Skalakid/fix-video-element-sharing

47f92d3

ishpaul777 approved these changes Mar 27, 2024

View reviewed changes

melvin-bot bot requested a review from lakchote March 27, 2024 17:34

lakchote approved these changes Mar 28, 2024

View reviewed changes

lakchote merged commit 5888c68 into Expensify:main Mar 28, 2024
15 of 17 checks passed

melvin-bot bot added the Emergency label Mar 28, 2024

github-actions bot mentioned this pull request Mar 29, 2024

Deploy Checklist: New Expensify 2024-03-29 #39222

Closed

lakchote removed the Emergency label Apr 1, 2024

tienifr reviewed May 22, 2024

View reviewed changes

hoangzinh mentioned this pull request May 24, 2024

[Awaiting Contributor Response for Payment - Payment 27th June] [$250] Incorrect video is paused after turning on and off full screen mode #42427

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix video sharing #38407

Fix video sharing #38407

Skalakid commented Mar 15, 2024 •

edited

Loading

Kicu left a comment

Kicu left a comment

melvin-bot bot commented Mar 20, 2024

ishpaul777 commented Mar 21, 2024

ishpaul777 commented Mar 22, 2024

ishpaul777 commented Mar 22, 2024 •

edited

Loading

ishpaul777 commented Mar 22, 2024 •

edited

Loading

ishpaul777 commented Mar 22, 2024

Skalakid commented Mar 25, 2024 •

edited

Loading

Skalakid commented Mar 25, 2024

Skalakid commented Mar 25, 2024

Skalakid commented Mar 26, 2024

ishpaul777 commented Mar 26, 2024

Skalakid commented Mar 27, 2024

ishpaul777 left a comment

melvin-bot bot commented Mar 28, 2024

OSBotify commented Mar 28, 2024

OSBotify commented Mar 29, 2024

lakchote commented Apr 1, 2024

OSBotify commented Apr 2, 2024

tienifr May 22, 2024

Skalakid May 22, 2024

tienifr May 23, 2024

tienifr May 25, 2024

jacobnguyen0000 May 28, 2024

tienifr May 28, 2024

jacobnguyen0000 May 28, 2024 •

edited

Loading

jacobnguyen0000 May 28, 2024

Skalakid May 28, 2024 •

edited

Loading

jacobnguyen0000 May 28, 2024

Fix video sharing #38407

Fix video sharing #38407

Conversation

Skalakid commented Mar 15, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Kicu left a comment

Choose a reason for hiding this comment

Kicu left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Mar 20, 2024

ishpaul777 commented Mar 21, 2024

ishpaul777 commented Mar 22, 2024

ishpaul777 commented Mar 22, 2024 • edited Loading

ishpaul777 commented Mar 22, 2024 • edited Loading

ishpaul777 commented Mar 22, 2024

Skalakid commented Mar 25, 2024 • edited Loading

Skalakid commented Mar 25, 2024

Skalakid commented Mar 25, 2024

Skalakid commented Mar 26, 2024

ishpaul777 commented Mar 26, 2024

Skalakid commented Mar 27, 2024

ishpaul777 left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Mar 28, 2024

OSBotify commented Mar 28, 2024

OSBotify commented Mar 29, 2024

lakchote commented Apr 1, 2024

OSBotify commented Apr 2, 2024

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

jacobnguyen0000 May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skalakid May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skalakid commented Mar 15, 2024 •

edited

Loading

ishpaul777 commented Mar 22, 2024 •

edited

Loading

ishpaul777 commented Mar 22, 2024 •

edited

Loading

Skalakid commented Mar 25, 2024 •

edited

Loading

jacobnguyen0000 May 28, 2024 •

edited

Loading

Skalakid May 28, 2024 •

edited

Loading