From 64d17c11376dcf05f2c7930108fa2bb4208df72c Mon Sep 17 00:00:00 2001 From: Krishna Gupta Date: Tue, 19 Sep 2023 08:42:30 +0530 Subject: [PATCH 1/6] fix(#26046): cache detecting logic moved to Image component. Signed-off-by: Krishna Gupta --- src/components/Image/index.js | 29 +++++++++++++++++++--- src/components/ImageWithSizeCalculation.js | 26 ++----------------- 2 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/components/Image/index.js b/src/components/Image/index.js index 7434c16c6491..f05c60186a2a 100644 --- a/src/components/Image/index.js +++ b/src/components/Image/index.js @@ -1,4 +1,4 @@ -import React, {useEffect, useMemo} from 'react'; +import React, {useEffect, useMemo, useRef, useState} from 'react'; import {Image as RNImage} from 'react-native'; import {withOnyx} from 'react-native-onyx'; import lodashGet from 'lodash/get'; @@ -8,7 +8,10 @@ import {defaultProps, imagePropTypes} from './imagePropTypes'; import RESIZE_MODES from './resizeModes'; function Image(props) { - const {source: propsSource, isAuthTokenRequired, onLoad, session} = props; + const {source: propsSource, isAuthTokenRequired, onLoad, session, onLoadStart = () => {}, onLoadEnd = () => {}} = props; + + const [isLoading, setIsLoading] = useState(false); + const isLoadedRef = useRef(null); /** * Check if the image source is a URL - if so the `encryptedAuthToken` is appended * to the source. @@ -41,14 +44,34 @@ function Image(props) { }); }, [onLoad, source]); + /** Delay the loader to detect whether the image is being loaded from the cache or the internet. */ + useEffect(() => { + if (isLoadedRef.current || !isLoading) { + return; + } + const timeout = _.delay(() => { + if (!isLoading || isLoadedRef.current) { + return; + } + onLoadStart(); + }, 200); + return () => clearTimeout(timeout); + }, [isLoading, onLoadStart]); + // Omit the props which the underlying RNImage won't use - const forwardedProps = _.omit(props, ['source', 'onLoad', 'session', 'isAuthTokenRequired']); + const forwardedProps = _.omit(props, ['source', 'onLoad', 'session', 'isAuthTokenRequired', 'onLoadStart', 'onLoadEnd']); return ( setIsLoading(true)} + onLoadEnd={() => { + onLoadEnd(); + setIsLoading(false); + isLoadedRef.current = true; + }} /> ); } diff --git a/src/components/ImageWithSizeCalculation.js b/src/components/ImageWithSizeCalculation.js index 6aa87d07f23e..57b933380b88 100644 --- a/src/components/ImageWithSizeCalculation.js +++ b/src/components/ImageWithSizeCalculation.js @@ -1,5 +1,4 @@ -import _ from 'underscore'; -import React, {useState, useRef, useEffect} from 'react'; +import React, {useState} from 'react'; import {View} from 'react-native'; import PropTypes from 'prop-types'; import Log from '../libs/Log'; @@ -39,8 +38,6 @@ const defaultProps = { * */ function ImageWithSizeCalculation(props) { - const isLoadedRef = useRef(null); - const [isImageCached, setIsImageCached] = useState(true); const [isLoading, setIsLoading] = useState(false); const onError = () => { @@ -48,27 +45,12 @@ function ImageWithSizeCalculation(props) { }; const imageLoadedSuccessfully = (event) => { - isLoadedRef.current = true; props.onMeasure({ width: event.nativeEvent.width, height: event.nativeEvent.height, }); }; - /** Delay the loader to detect whether the image is being loaded from the cache or the internet. */ - useEffect(() => { - if (isLoadedRef.current || !isLoading) { - return; - } - const timeout = _.delay(() => { - if (!isLoading || isLoadedRef.current) { - return; - } - setIsImageCached(false); - }, 200); - return () => clearTimeout(timeout); - }, [isLoading]); - return ( { - if (isLoadedRef.current || isLoading) { - return; - } setIsLoading(true); }} onLoadEnd={() => { setIsLoading(false); - setIsImageCached(true); }} onError={onError} onLoad={imageLoadedSuccessfully} /> - {isLoading && !isImageCached && } + {isLoading && } ); } From bef2289446b6601643706cbd4fa3ccde16c9f27e Mon Sep 17 00:00:00 2001 From: Krishna Gupta Date: Tue, 19 Sep 2023 12:27:40 +0530 Subject: [PATCH 2/6] fix(#26046): loader on top of carousel image. Signed-off-by: Krishna Gupta --- .../Pager/AttachmentCarouselPage.js | 39 ++++++++++++++----- src/components/Image/index.js | 7 +++- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPage.js b/src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPage.js index b1a844e4172d..06827e00c8ad 100644 --- a/src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPage.js +++ b/src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPage.js @@ -1,5 +1,5 @@ /* eslint-disable es/no-optional-chaining */ -import React, {useContext, useEffect, useState} from 'react'; +import React, {useContext, useEffect, useRef, useState} from 'react'; import {ActivityIndicator, PixelRatio, StyleSheet, View} from 'react-native'; import PropTypes from 'prop-types'; import Image from '../../../Image'; @@ -46,13 +46,16 @@ function AttachmentCarouselPage({source, isAuthTokenRequired, isActive: initialI }, [initialIsActive]); const [initialActivePageLoad, setInitialActivePageLoad] = useState(isActive); - const [isImageLoading, setIsImageLoading] = useState(true); - const [showFallback, setShowFallback] = useState(isImageLoading); + const isImageLoaded = useRef(null); + const [isImageLoading, setIsImageLoading] = useState(false); + const [isFallbackLoading, setIsFallbackLoading] = useState(false); + const [showFallback, setShowFallback] = useState(true); // We delay hiding the fallback image while image transformer is still rendering useEffect(() => { - if (isImageLoading) setShowFallback(true); + if (isImageLoading || showFallback) setShowFallback(true); else setTimeout(() => setShowFallback(false), 100); + // eslint-disable-next-line react-hooks/exhaustive-deps }, [isImageLoading]); return ( @@ -73,8 +76,14 @@ function AttachmentCarouselPage({source, isAuthTokenRequired, isActive: initialI source={{uri: source}} style={dimensions == null ? undefined : {width: dimensions.imageWidth, height: dimensions.imageHeight}} isAuthTokenRequired={isAuthTokenRequired} - onLoadStart={() => setIsImageLoading(true)} - onLoadEnd={() => setIsImageLoading(false)} + onLoadStart={() => { + setIsImageLoading(true); + }} + onLoadEnd={() => { + setShowFallback(false); + setIsImageLoading(false); + isImageLoaded.current = true; + }} onLoad={(evt) => { const imageWidth = (evt.nativeEvent?.width || 0) / PixelRatio.get(); const imageHeight = (evt.nativeEvent?.height || 0) / PixelRatio.get(); @@ -100,8 +109,10 @@ function AttachmentCarouselPage({source, isAuthTokenRequired, isActive: initialI // On the initial render of the active page, the onLoadEnd event is never fired. // That's why we instead set isImageLoading to false in the onLoad event. if (initialActivePageLoad) { - setIsImageLoading(false); setInitialActivePageLoad(false); + setIsImageLoading(false); + setTimeout(() => setShowFallback(false), 100); + isImageLoaded.current = true; } }} /> @@ -110,12 +121,20 @@ function AttachmentCarouselPage({source, isAuthTokenRequired, isActive: initialI )} {/* Keep rendering the image without gestures as fallback while ImageTransformer is loading the image */} - {(!isActive || showFallback) && ( + {(showFallback || !isActive) && ( setIsImageLoading(true)} + onLoadStart={() => { + setIsImageLoading(true); + if (isImageLoaded.current) return; + setIsFallbackLoading(true); + }} + onLoadEnd={() => { + if (isImageLoaded.current) return; + setIsFallbackLoading(false); + }} onLoad={(evt) => { const imageWidth = evt.nativeEvent.width; const imageHeight = evt.nativeEvent.height; @@ -141,7 +160,7 @@ function AttachmentCarouselPage({source, isAuthTokenRequired, isActive: initialI )} {/* Show activity indicator while ImageTransfomer is still loading the image. */} - {isActive && isImageLoading && ( + {isActive && isFallbackLoading && !isImageLoaded.current && ( clearTimeout(timeout); - }, [isLoading, onLoadStart]); + }, [isLoading]); // Omit the props which the underlying RNImage won't use const forwardedProps = _.omit(props, ['source', 'onLoad', 'session', 'isAuthTokenRequired', 'onLoadStart', 'onLoadEnd']); @@ -67,10 +67,13 @@ function Image(props) { {...forwardedProps} source={source} onLoadStart={() => setIsLoading(true)} + onLoad={() => { + isLoadedRef.current = true; + }} onLoadEnd={() => { - onLoadEnd(); setIsLoading(false); isLoadedRef.current = true; + onLoadEnd(); }} /> ); From d3f8bd5d88a9789417a96920846cc0934f7991c4 Mon Sep 17 00:00:00 2001 From: Krishna Gupta Date: Tue, 19 Sep 2023 15:00:10 +0530 Subject: [PATCH 3/6] fix: useEffect missing dependency warning. Signed-off-by: Krishna Gupta --- src/components/Image/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/Image/index.js b/src/components/Image/index.js index 30c67bcd4d89..3cadac0157bb 100644 --- a/src/components/Image/index.js +++ b/src/components/Image/index.js @@ -56,6 +56,7 @@ function Image(props) { onLoadStart(); }, 200); return () => clearTimeout(timeout); + // eslint-disable-next-line react-hooks/exhaustive-deps }, [isLoading]); // Omit the props which the underlying RNImage won't use From e7d3272a218e02567eff58ad4450304999208bfa Mon Sep 17 00:00:00 2001 From: Krishna Gupta Date: Thu, 21 Sep 2023 15:39:06 +0530 Subject: [PATCH 4/6] fix: lint warning. Signed-off-by: Krishna Gupta --- .../AttachmentCarousel/Pager/AttachmentCarouselPage.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPage.js b/src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPage.js index cec6237744a2..b94f456f3a7e 100644 --- a/src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPage.js +++ b/src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPage.js @@ -56,8 +56,11 @@ function AttachmentCarouselPage({source, isAuthTokenRequired, isActive: initialI // We delay hiding the fallback image while image transformer is still rendering useEffect(() => { - if (isImageLoading || showFallback) setShowFallback(true); - else setTimeout(() => setShowFallback(false), 100); + if (isImageLoading || showFallback) { + setShowFallback(true); + } else { + setTimeout(() => setShowFallback(false), 100); + } // eslint-disable-next-line react-hooks/exhaustive-deps }, [isImageLoading]); From cfd953503cad90186836a5415613e723e3b54c77 Mon Sep 17 00:00:00 2001 From: Krishna Gupta Date: Thu, 21 Sep 2023 15:58:17 +0530 Subject: [PATCH 5/6] fix: lint warning. Signed-off-by: Krishna Gupta --- .../AttachmentCarousel/Pager/AttachmentCarouselPage.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPage.js b/src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPage.js index b94f456f3a7e..f7da8cfce894 100644 --- a/src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPage.js +++ b/src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPage.js @@ -134,11 +134,15 @@ function AttachmentCarouselPage({source, isAuthTokenRequired, isActive: initialI isAuthTokenRequired={isAuthTokenRequired} onLoadStart={() => { setIsImageLoading(true); - if (isImageLoaded.current) return; + if (isImageLoaded.current) { + return; + } setIsFallbackLoading(true); }} onLoadEnd={() => { - if (isImageLoaded.current) return; + if (isImageLoaded.current) { + return; + } setIsFallbackLoading(false); }} onLoad={(evt) => { From 68250402faa470e76a948e4f88ac59b24336a6a2 Mon Sep 17 00:00:00 2001 From: Krishna Gupta Date: Fri, 22 Sep 2023 07:34:16 +0530 Subject: [PATCH 6/6] revert back changes. Signed-off-by: Krishna Gupta --- src/components/Image/index.js | 33 ++-------------------- src/components/ImageWithSizeCalculation.js | 26 +++++++++++++++-- 2 files changed, 27 insertions(+), 32 deletions(-) diff --git a/src/components/Image/index.js b/src/components/Image/index.js index 3cadac0157bb..7434c16c6491 100644 --- a/src/components/Image/index.js +++ b/src/components/Image/index.js @@ -1,4 +1,4 @@ -import React, {useEffect, useMemo, useRef, useState} from 'react'; +import React, {useEffect, useMemo} from 'react'; import {Image as RNImage} from 'react-native'; import {withOnyx} from 'react-native-onyx'; import lodashGet from 'lodash/get'; @@ -8,10 +8,7 @@ import {defaultProps, imagePropTypes} from './imagePropTypes'; import RESIZE_MODES from './resizeModes'; function Image(props) { - const {source: propsSource, isAuthTokenRequired, onLoad, session, onLoadStart = () => {}, onLoadEnd = () => {}} = props; - - const [isLoading, setIsLoading] = useState(false); - const isLoadedRef = useRef(null); + const {source: propsSource, isAuthTokenRequired, onLoad, session} = props; /** * Check if the image source is a URL - if so the `encryptedAuthToken` is appended * to the source. @@ -44,38 +41,14 @@ function Image(props) { }); }, [onLoad, source]); - /** Delay the loader to detect whether the image is being loaded from the cache or the internet. */ - useEffect(() => { - if (isLoadedRef.current || !isLoading) { - return; - } - const timeout = _.delay(() => { - if (!isLoading || isLoadedRef.current) { - return; - } - onLoadStart(); - }, 200); - return () => clearTimeout(timeout); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [isLoading]); - // Omit the props which the underlying RNImage won't use - const forwardedProps = _.omit(props, ['source', 'onLoad', 'session', 'isAuthTokenRequired', 'onLoadStart', 'onLoadEnd']); + const forwardedProps = _.omit(props, ['source', 'onLoad', 'session', 'isAuthTokenRequired']); return ( setIsLoading(true)} - onLoad={() => { - isLoadedRef.current = true; - }} - onLoadEnd={() => { - setIsLoading(false); - isLoadedRef.current = true; - onLoadEnd(); - }} /> ); } diff --git a/src/components/ImageWithSizeCalculation.js b/src/components/ImageWithSizeCalculation.js index 57b933380b88..6aa87d07f23e 100644 --- a/src/components/ImageWithSizeCalculation.js +++ b/src/components/ImageWithSizeCalculation.js @@ -1,4 +1,5 @@ -import React, {useState} from 'react'; +import _ from 'underscore'; +import React, {useState, useRef, useEffect} from 'react'; import {View} from 'react-native'; import PropTypes from 'prop-types'; import Log from '../libs/Log'; @@ -38,6 +39,8 @@ const defaultProps = { * */ function ImageWithSizeCalculation(props) { + const isLoadedRef = useRef(null); + const [isImageCached, setIsImageCached] = useState(true); const [isLoading, setIsLoading] = useState(false); const onError = () => { @@ -45,12 +48,27 @@ function ImageWithSizeCalculation(props) { }; const imageLoadedSuccessfully = (event) => { + isLoadedRef.current = true; props.onMeasure({ width: event.nativeEvent.width, height: event.nativeEvent.height, }); }; + /** Delay the loader to detect whether the image is being loaded from the cache or the internet. */ + useEffect(() => { + if (isLoadedRef.current || !isLoading) { + return; + } + const timeout = _.delay(() => { + if (!isLoading || isLoadedRef.current) { + return; + } + setIsImageCached(false); + }, 200); + return () => clearTimeout(timeout); + }, [isLoading]); + return ( { + if (isLoadedRef.current || isLoading) { + return; + } setIsLoading(true); }} onLoadEnd={() => { setIsLoading(false); + setIsImageCached(true); }} onError={onError} onLoad={imageLoadedSuccessfully} /> - {isLoading && } + {isLoading && !isImageCached && } ); }