-
-
Notifications
You must be signed in to change notification settings - Fork 361
Load Thumbnail first and fall back to the original image if it cannot be loaded #2445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
🤔 Maybe let users control the max pixels of width and height of thumbnail is a better idea? |
|
I have read the CLA Document and I hereby sign the CLA |
… image When viewing messages, thumbnails are loaded first, if loading fails, the original image is loaded as a fallback.
CyanChanges
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| const decrypt = (encBuf: ArrayBuffer) => decryptFile(encBuf, mimeType ?? FALLBACK_MIMETYPE, encInfo); | ||
| let fileContent; | ||
| try { | ||
| fileContent = await downloadEncryptedMedia(mediaUrl, decrypt); | ||
| } catch { | ||
| fileContent = await downloadEncryptedMedia(mxcUrlToHttp(mx, url, useAuthentication) ?? url, decrypt); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should always use the original url for encrypted image as it is always going to fail to generate thumbnail for it.
| if (useThumbnail) { | ||
| setUseThumbnail(false); | ||
| loadSrc(); | ||
| } else { | ||
| setLoad(false); | ||
| setError(true); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any particular reason to use this fallback strategy?
| > | ||
| {renderViewer({ | ||
| src: srcState.data, | ||
| src: useThumbnail ? (mxcUrlToHttp(mx, url, useAuthentication) ?? url) : srcState.data, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since encrypted message always going to use full image we should use srcState.data for them and url for normal images.
| } | ||
|
|
||
| if (msgType === MsgType.Image) { | ||
| const content: IImageContent = getContent(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IImageContent already provide the thumbnail image along with dimensions, should not we be passing that to ImageContent component?
| resizeMethod: string | ||
| } | undefined; | ||
| if (width && height) { | ||
| const scale = (width > height ? width : height) / 800; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already has a standard thumbnail scaling function getThumbnailDimensions which is used to generate the thumbnail while sending images.
| url?: string; | ||
| info?: IImageInfo & IThumbnailContent; | ||
| file?: IEncryptedFile; | ||
| info?: { h: number, w: number }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
info is already described at line 50.
Description
When viewing messages, thumbnails are loaded first, if loading fails, the original image is loaded as a fallback.
Type of change
Checklist: