From 87858277f2dea5a147c91acbbd3cafdd7fd91b56 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Wed, 6 Mar 2024 00:16:33 +0800 Subject: [PATCH 1/3] fix crash when expanding video player --- .../Attachments/AttachmentView/index.js | 11 ---------- src/components/VideoPlayer/BaseVideoPlayer.js | 21 ++++++++++++++++++- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/components/Attachments/AttachmentView/index.js b/src/components/Attachments/AttachmentView/index.js index 341b71657bbc..4ebf6c02d094 100755 --- a/src/components/Attachments/AttachmentView/index.js +++ b/src/components/Attachments/AttachmentView/index.js @@ -115,17 +115,6 @@ function AttachmentView({ updateCurrentlyPlayingURL(isVideo ? source : null); }, [isFocused, isVideo, source, updateCurrentlyPlayingURL, file, isUsedInAttachmentModal]); - // This should ensure we clean up any video references when closing the attachment modal as these only existed here in memory during attachment preview. - useEffect( - () => () => { - if (!isVideo) { - return; - } - currentVideoPlayerRef.current = null; - }, - [isVideo, currentVideoPlayerRef], - ); - const [imageError, setImageError] = useState(false); useNetwork({onReconnect: () => setImageError(false)}); diff --git a/src/components/VideoPlayer/BaseVideoPlayer.js b/src/components/VideoPlayer/BaseVideoPlayer.js index 61485d67e6d6..9341827128bb 100644 --- a/src/components/VideoPlayer/BaseVideoPlayer.js +++ b/src/components/VideoPlayer/BaseVideoPlayer.js @@ -60,6 +60,7 @@ function BaseVideoPlayer({ const canUseTouchScreen = DeviceCapabilities.canUseTouchScreen(); const isCurrentlyURLSet = currentlyPlayingURL === url; const isUploading = _.some(CONST.ATTACHMENT_LOCAL_URL_PREFIX, (prefix) => url.startsWith(prefix)); + const shouldUseSharedVideoElementRef = useRef(shouldUseSharedVideoElement); const togglePlayCurrentVideo = useCallback(() => { videoResumeTryNumber.current = 0; @@ -145,8 +146,26 @@ function BaseVideoPlayer({ }, [currentVideoPlayerRef, handleFullscreenUpdate, handlePlaybackStatusUpdate]); useEffect(() => { + if (!isUploading) { + return; + } + // If we are uploading a new video, we want to immediately set the video player ref. currentVideoPlayerRef.current = videoPlayerRef.current; - }, [url, currentVideoPlayerRef]); + }, [url, currentVideoPlayerRef, isUploading]); + + useEffect(() => { + shouldUseSharedVideoElementRef.current = shouldUseSharedVideoElement; + }, [shouldUseSharedVideoElement]); + + useEffect(() => { + return () => { + if (shouldUseSharedVideoElementRef.current) { + return; + } + // If it's not a shared video player, clear the video player ref. + currentVideoPlayerRef.current = null; + }; + }, []); // update shared video elements useEffect(() => { From c82070e81eb24807888a9beb13f1adf21410420c Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Wed, 6 Mar 2024 00:20:09 +0800 Subject: [PATCH 2/3] fix lint --- .../Attachments/AttachmentView/index.js | 2 +- src/components/VideoPlayer/BaseVideoPlayer.js | 16 +++++++--------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/components/Attachments/AttachmentView/index.js b/src/components/Attachments/AttachmentView/index.js index 4ebf6c02d094..f6a56dc73088 100755 --- a/src/components/Attachments/AttachmentView/index.js +++ b/src/components/Attachments/AttachmentView/index.js @@ -101,7 +101,7 @@ function AttachmentView({ isHovered, optionalVideoDuration, }) { - const {updateCurrentlyPlayingURL, currentVideoPlayerRef} = usePlaybackContext(); + const {updateCurrentlyPlayingURL} = usePlaybackContext(); const theme = useTheme(); const styles = useThemeStyles(); const StyleUtils = useStyleUtils(); diff --git a/src/components/VideoPlayer/BaseVideoPlayer.js b/src/components/VideoPlayer/BaseVideoPlayer.js index 9341827128bb..53943e53681b 100644 --- a/src/components/VideoPlayer/BaseVideoPlayer.js +++ b/src/components/VideoPlayer/BaseVideoPlayer.js @@ -157,15 +157,13 @@ function BaseVideoPlayer({ shouldUseSharedVideoElementRef.current = shouldUseSharedVideoElement; }, [shouldUseSharedVideoElement]); - useEffect(() => { - return () => { - if (shouldUseSharedVideoElementRef.current) { - return; - } - // If it's not a shared video player, clear the video player ref. - currentVideoPlayerRef.current = null; - }; - }, []); + useEffect(() => () => { + if (shouldUseSharedVideoElementRef.current) { + return; + } + // If it's not a shared video player, clear the video player ref. + currentVideoPlayerRef.current = null; + }, [currentVideoPlayerRef]); // update shared video elements useEffect(() => { From 346b0796d70d65e08a66626f9fa958e7239e7aed Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Wed, 6 Mar 2024 01:30:15 +0800 Subject: [PATCH 3/3] prettier --- src/components/VideoPlayer/BaseVideoPlayer.js | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/components/VideoPlayer/BaseVideoPlayer.js b/src/components/VideoPlayer/BaseVideoPlayer.js index 53943e53681b..23d0bb6f816b 100644 --- a/src/components/VideoPlayer/BaseVideoPlayer.js +++ b/src/components/VideoPlayer/BaseVideoPlayer.js @@ -157,13 +157,16 @@ function BaseVideoPlayer({ shouldUseSharedVideoElementRef.current = shouldUseSharedVideoElement; }, [shouldUseSharedVideoElement]); - useEffect(() => () => { - if (shouldUseSharedVideoElementRef.current) { - return; - } - // If it's not a shared video player, clear the video player ref. - currentVideoPlayerRef.current = null; - }, [currentVideoPlayerRef]); + useEffect( + () => () => { + if (shouldUseSharedVideoElementRef.current) { + return; + } + // If it's not a shared video player, clear the video player ref. + currentVideoPlayerRef.current = null; + }, + [currentVideoPlayerRef], + ); // update shared video elements useEffect(() => {