From fcf7406dd4a11aa548711822846d1af9c331a4dd Mon Sep 17 00:00:00 2001 From: Josh Perez <60019601+josh-signal@users.noreply.github.com> Date: Wed, 3 Aug 2022 20:38:41 -0400 Subject: [PATCH] Adds error states to story images --- _locales/en/messages.json | 16 ++++-- stylesheets/components/StoryImage.scss | 12 +++++ ts/components/MyStories.tsx | 2 + ts/components/MyStoriesButton.tsx | 2 + ts/components/StoryImage.stories.tsx | 37 +++++++++++++ ts/components/StoryImage.tsx | 26 ++++++++-- ts/components/StoryListItem.tsx | 1 + ts/components/StoryViewer.tsx | 1 + ts/messageModifiers/AttachmentDownloads.ts | 60 +++++++++++++++++----- ts/state/ducks/stories.ts | 13 ++++- ts/types/Attachment.ts | 4 ++ ts/util/getStoryDuration.ts | 5 ++ 12 files changed, 158 insertions(+), 21 deletions(-) diff --git a/_locales/en/messages.json b/_locales/en/messages.json index 06a905062058..66cbf5c8721e 100644 --- a/_locales/en/messages.json +++ b/_locales/en/messages.json @@ -7481,9 +7481,19 @@ "message": "Hide", "description": "Action button for the confirmation dialog to hide a story" }, - "StoryImage__error": { - "message": "Error displaying image", - "description": "aria-label for image errors" + "StoryImage__error2": { + "message": "Can’t download story. $name$ will need to share it again.", + "description": "Description for image errors", + "placeholders": { + "name": { + "content": "$1", + "example": "Clara" + } + } + }, + "StoryImage__error--you": { + "message": "Can’t download story. You will need to share it again.", + "description": "Description for image errors but when it is your own image" }, "StoryCreator__text-bg": { "message": "Toggle text background color", diff --git a/stylesheets/components/StoryImage.scss b/stylesheets/components/StoryImage.scss index c135a43ff361..96700c387a07 100644 --- a/stylesheets/components/StoryImage.scss +++ b/stylesheets/components/StoryImage.scss @@ -33,6 +33,18 @@ width: 100%; } + &__error { + @include color-svg( + '../images/full-screen-flow/alert-outline.svg', + $color-white + ); + align-items: center; + display: flex; + height: 32px; + justify-content: center; + width: 32px; + } + &__spinner-bubble { align-items: center; background-color: $color-gray-75; diff --git a/ts/components/MyStories.tsx b/ts/components/MyStories.tsx index 87abb5c86326..b9675c186de5 100644 --- a/ts/components/MyStories.tsx +++ b/ts/components/MyStories.tsx @@ -92,7 +92,9 @@ export const MyStories = ({ > ( })} /> ); + +export const ErrorImage = (): JSX.Element => ( + +); + +export const ErrorImageThumbnail = (): JSX.Element => ( + +); + +ErrorImageThumbnail.story = { + name: 'Error Image (thumbnail)', +}; + +export const ErrorImageYou = (): JSX.Element => ( + +); diff --git a/ts/components/StoryImage.tsx b/ts/components/StoryImage.tsx index 7dbb954118c7..4efa0eb6dfcf 100644 --- a/ts/components/StoryImage.tsx +++ b/ts/components/StoryImage.tsx @@ -2,7 +2,7 @@ // SPDX-License-Identifier: AGPL-3.0-only import type { ReactNode } from 'react'; -import React, { useEffect, useRef } from 'react'; +import React, { useEffect, useRef, useState } from 'react'; import classNames from 'classnames'; import { Blurhash } from 'react-blurhash'; @@ -13,6 +13,7 @@ import { TextAttachment } from './TextAttachment'; import { ThemeType } from '../types/Util'; import { defaultBlurHash, + hasFailed, hasNotResolved, isDownloaded, isDownloading, @@ -24,7 +25,9 @@ import { isVideoTypeSupported } from '../util/GoogleChrome'; export type PropsType = { readonly attachment?: AttachmentType; readonly children?: ReactNode; + readonly firstName: string; readonly i18n: LocalizerType; + readonly isMe?: boolean; readonly isMuted?: boolean; readonly isPaused?: boolean; readonly isThumbnail?: boolean; @@ -37,7 +40,9 @@ export type PropsType = { export const StoryImage = ({ attachment, children, + firstName, i18n, + isMe, isMuted, isPaused, isThumbnail, @@ -50,6 +55,7 @@ export const StoryImage = ({ (!isDownloaded(attachment) && !isDownloading(attachment)) || hasNotResolved(attachment); + const [hasImgError, setHasImgError] = useState(false); const videoRef = useRef(null); useEffect(() => { @@ -74,8 +80,10 @@ export const StoryImage = ({ return null; } - const isPending = Boolean(attachment.pending) && !attachment.textAttachment; - const isNotReadyToShow = hasNotResolved(attachment) || isPending; + const hasError = hasImgError || hasFailed(attachment); + const isPending = + Boolean(attachment.pending) && !attachment.textAttachment && !hasError; + const isNotReadyToShow = hasNotResolved(attachment) || isPending || hasError; const isSupportedVideo = isVideoTypeSupported(attachment.contentType); const getClassName = getClassNamesFor('StoryImage', moduleClassName); @@ -118,6 +126,7 @@ export const StoryImage = ({ {label} setHasImgError(true)} src={ isThumbnail && attachment.thumbnail ? attachment.thumbnail.url @@ -136,6 +145,17 @@ export const StoryImage = ({ ); + } else if (hasError) { + let content =
; + if (!isThumbnail) { + if (isMe) { + content = <>{i18n('StoryImage__error--you')}; + } else { + content = <>{i18n('StoryImage__error2', [firstName])}; + } + } + + overlay =
{content}
; } return ( diff --git a/ts/components/StoryListItem.tsx b/ts/components/StoryListItem.tsx index 3ebeb3bd3a5b..79df56c3467d 100644 --- a/ts/components/StoryListItem.tsx +++ b/ts/components/StoryListItem.tsx @@ -179,6 +179,7 @@ export const StoryListItem = ({
{ delete _activeAttachmentDownloadJobs[job.id]; try { - await removeAttachmentDownloadJob(job.id); + await _markAttachmentAsFailed(job); } catch (deleteError) { log.error( `${logId}: Failed to delete attachment job`, @@ -261,20 +261,10 @@ async function _runJob(job?: AttachmentDownloadJobType): Promise { const pending = true; await setAttachmentDownloadJobPending(id, pending); - message = window.MessageController.getById(messageId); - if (!message) { - const messageAttributes = await getMessageById(messageId); - if (!messageAttributes) { - logger.error( - `attachment_downloads/_runJob(${id}): ` + - 'Source message not found, deleting job' - ); - await _finishJob(null, id); - return; - } + message = await _getMessageById(id, messageId); - strictAssert(messageId === messageAttributes.id, 'message id mismatch'); - message = window.MessageController.register(messageId, messageAttributes); + if (!message) { + return; } await _addAttachmentToMessage( @@ -370,6 +360,48 @@ async function _runJob(job?: AttachmentDownloadJobType): Promise { } } +async function _markAttachmentAsFailed( + job: AttachmentDownloadJobType +): Promise { + const { id, messageId, attachment, type, index } = job; + const message = await _getMessageById(id, messageId); + + if (!message) { + return; + } + + await _addAttachmentToMessage( + message, + _markAttachmentAsPermanentError(attachment), + { type, index } + ); + await _finishJob(message, id); +} + +async function _getMessageById( + id: string, + messageId: string +): Promise { + const message = window.MessageController.getById(messageId); + + if (message) { + return message; + } + + const messageAttributes = await getMessageById(messageId); + if (!messageAttributes) { + logger.error( + `attachment_downloads/_runJob(${id}): ` + + 'Source message not found, deleting job' + ); + await _finishJob(null, id); + return; + } + + strictAssert(messageId === messageAttributes.id, 'message id mismatch'); + return window.MessageController.register(messageId, messageAttributes); +} + async function _finishJob( message: MessageModel | null | undefined, id: string diff --git a/ts/state/ducks/stories.ts b/ts/state/ducks/stories.ts index f553e496a5a4..661de32373ed 100644 --- a/ts/state/ducks/stories.ts +++ b/ts/state/ducks/stories.ts @@ -34,6 +34,7 @@ import { replaceIndex } from '../../util/replaceIndex'; import { sendDeleteForEveryoneMessage } from '../../util/sendDeleteForEveryoneMessage'; import { showToast } from '../../util/showToast'; import { + hasFailed, hasNotResolved, isDownloaded, isDownloading, @@ -378,7 +379,10 @@ function markStoryRead( return; } - if (!isDownloaded(matchingStory.attachment)) { + if ( + !isDownloaded(matchingStory.attachment) && + !hasFailed(matchingStory.attachment) + ) { return; } @@ -449,6 +453,10 @@ function queueStoryDownload( return; } + if (hasFailed(attachment)) { + return; + } + if (isDownloaded(attachment)) { if (!attachment.path) { return; @@ -1001,6 +1009,8 @@ export function reducer( const hasAttachmentDownloaded = !isDownloaded(prevStory.attachment) && isDownloaded(newStory.attachment); + const hasAttachmentFailed = + hasFailed(newStory.attachment) && !hasFailed(prevStory.attachment); const readStatusChanged = prevStory.readStatus !== newStory.readStatus; const reactionsChanged = prevStory.reactions?.length !== newStory.reactions?.length; @@ -1014,6 +1024,7 @@ export function reducer( const shouldReplace = isDownloadingAttachment || hasAttachmentDownloaded || + hasAttachmentFailed || hasBeenDeleted || hasSendStateChanged || readStatusChanged || diff --git a/ts/types/Attachment.ts b/ts/types/Attachment.ts index 3cf75f2589e5..526e507b861b 100644 --- a/ts/types/Attachment.ts +++ b/ts/types/Attachment.ts @@ -768,6 +768,10 @@ export function isDownloading(attachment?: AttachmentType): boolean { return Boolean(attachment && attachment.downloadJobId && attachment.pending); } +export function hasFailed(attachment?: AttachmentType): boolean { + return Boolean(attachment && attachment.error); +} + export function hasVideoBlurHash(attachments?: Array): boolean { const firstAttachment = attachments ? attachments[0] : null; diff --git a/ts/util/getStoryDuration.ts b/ts/util/getStoryDuration.ts index 7d6fda9b0f2b..4084a53a2992 100644 --- a/ts/util/getStoryDuration.ts +++ b/ts/util/getStoryDuration.ts @@ -3,6 +3,7 @@ import type { AttachmentType } from '../types/Attachment'; import { + hasFailed, hasNotResolved, isDownloaded, isGIF, @@ -18,6 +19,10 @@ const MIN_TEXT_DURATION = 3 * SECOND; export async function getStoryDuration( attachment: AttachmentType ): Promise { + if (hasFailed(attachment)) { + return DEFAULT_DURATION; + } + if (!isDownloaded(attachment) || hasNotResolved(attachment)) { return; }