Merge pull request #4629 from matrix-org/t3chguy/fix-baseavatar-hooks

Fix BaseAvatar wrongly retrying urls

src/components/views/avatars/BaseAvatar.js

@@ -26,58 +26,48 @@ import MatrixClientContext from "../../../contexts/MatrixClientContext";
 import {useEventEmitter} from "../../../hooks/useEventEmitter";
 import {toPx} from "../../../utils/units";
 
-const useImageUrl = ({url, urls, idName, name, defaultToInitialLetter}) => {
+const useImageUrl = ({url, urls}) => {
     const [imageUrls, setUrls] = useState([]);
     const [urlsIndex, setIndex] = useState();
 
-    const onError = () => {
+    const onError = useCallback(() => {
-        const nextIndex = urlsIndex + 1;
+        setIndex(i => i + 1); // try the next one
-        if (nextIndex < imageUrls.length) {
+    }, []);
-            // try the next one
+    const memoizedUrls = useMemo(() => urls, [JSON.stringify(urls)]); // eslint-disable-line react-hooks/exhaustive-deps
-            setIndex(nextIndex);
-        }
-    };
-
-    const defaultImageUrl = useMemo(() => AvatarLogic.defaultAvatarUrlForString(idName || name), [idName, name]);
 
     useEffect(() => {
         // work out the full set of urls to try to load. This is formed like so:
-        // imageUrls: [ props.url, ...props.urls, default image ]
+        // imageUrls: [ props.url, ...props.urls ]
 
         let _urls = [];
         if (!SettingsStore.getValue("lowBandwidth")) {
-            _urls = urls || [];
+            _urls = memoizedUrls || [];
 
             if (url) {
                 _urls.unshift(url); // put in urls[0]
             }
         }
 
-        if (defaultToInitialLetter) {
-            _urls.push(defaultImageUrl); // lowest priority
-        }
-
         // deduplicate URLs
         _urls = Array.from(new Set(_urls));
 
         setIndex(0);
         setUrls(_urls);
-    }, [url, ...(urls || [])]); // eslint-disable-line react-hooks/exhaustive-deps
+    }, [url, memoizedUrls]); // eslint-disable-line react-hooks/exhaustive-deps
 
     const cli = useContext(MatrixClientContext);
     const onClientSync = useCallback((syncState, prevState) => {
         // Consider the client reconnected if there is no error with syncing.
         // This means the state could be RECONNECTING, SYNCING, PREPARED or CATCHUP.
         const reconnected = syncState !== "ERROR" && prevState !== syncState;
-        if (reconnected && urlsIndex > 0 ) { // Did we fall back?
+        if (reconnected) {
-            // Start from the highest priority URL again
             setIndex(0);
         }
-    }, [urlsIndex]);
+    }, []);
     useEventEmitter(cli, "sync", onClientSync);
 
     const imageUrl = imageUrls[urlsIndex];
-    return [imageUrl, imageUrl === defaultImageUrl, onError];
+    return [imageUrl, onError];
 };
 
 const BaseAvatar = (props) => {
@@ -96,9 +86,9 @@ const BaseAvatar = (props) => {
         ...otherProps
     } = props;
 
-    const [imageUrl, isDefault, onError] = useImageUrl({url, urls, idName, name, defaultToInitialLetter});
+    const [imageUrl, onError] = useImageUrl({url, urls});
 
-    if (isDefault) {
+    if (!imageUrl && defaultToInitialLetter) {
         const initialLetter = AvatarLogic.getInitialLetter(name);
         const textNode = (
             <span
@@ -116,7 +106,7 @@ const BaseAvatar = (props) => {
         const imgNode = (
             <img
                 className="mx_BaseAvatar_image"
-                src={imageUrl}
+                src={AvatarLogic.defaultAvatarUrlForString(idName || name)}
                 alt=""
                 title={title}
                 onError={onError}

src/hooks/useEventEmitter.js

@@ -32,7 +32,7 @@ export const useEventEmitter = (emitter, eventName, handler) => {
             if (!emitter) return;
 
             // Create event listener that calls handler function stored in ref
-            const eventListener = event => savedHandler.current(event);
+            const eventListener = (...args) => savedHandler.current(...args);
 
             // Add event listener
             emitter.on(eventName, eventListener);

test/components/views/messages/TextualBody-test.js

@@ -205,9 +205,8 @@ describe("<TextualBody />", () => {
             expect(content.html()).toBe('<span class="mx_EventTile_body markdown-body" dir="auto">' +
                 'Hey <span>' +
                 '<a class="mx_Pill mx_UserPill" title="@user:server">' +
-                '<img class="mx_BaseAvatar mx_BaseAvatar_image" ' +
+                '<img class="mx_BaseAvatar mx_BaseAvatar_image" src="mxc://avatar.url/image.png" ' +
-                'style="width: 16px; height: 16px;" ' +
+                'style="width: 16px; height: 16px;" title="@member:domain.bla" alt="" aria-hidden="true">Member</a>' +
-                'title="@member:domain.bla" alt="" aria-hidden="true" src="mxc://avatar.url/image.png">Member</a>' +
                 '</span></span>');
         });
     });