From 8c25ffd6f5644f4346c8b5420d2f5eeb061af232 Mon Sep 17 00:00:00 2001 From: Evan Hahn <69474926+EvanHahn-Signal@users.noreply.github.com> Date: Fri, 8 Jan 2021 13:39:32 -0600 Subject: [PATCH] Link previews: show full size image less often --- ts/components/conversation/Message.tsx | 55 ++----- .../conversation/StagedLinkPreview.tsx | 2 +- .../shouldUseFullSizeLinkPreviewImage.ts | 34 ++++ .../shouldUseFullSizeLinkPreviewImage_test.ts | 146 ++++++++++++++++++ ts/types/Attachment.ts | 10 +- ts/types/message/LinkPreviews.ts | 14 ++ ts/util/lint/exceptions.json | 12 +- 7 files changed, 220 insertions(+), 53 deletions(-) create mode 100644 ts/linkPreviews/shouldUseFullSizeLinkPreviewImage.ts create mode 100644 ts/test-both/linkPreviews/shouldUseFullSizeLinkPreviewImage_test.ts create mode 100644 ts/types/message/LinkPreviews.ts diff --git a/ts/components/conversation/Message.tsx b/ts/components/conversation/Message.tsx index 8130245981ef..75b6f5deb84b 100644 --- a/ts/components/conversation/Message.tsx +++ b/ts/components/conversation/Message.tsx @@ -26,6 +26,8 @@ import { import { Props as ReactionPickerProps } from './ReactionPicker'; import { Emoji } from '../emoji/Emoji'; import { LinkPreviewDate } from './LinkPreviewDate'; +import { LinkPreviewType } from '../../types/message/LinkPreviews'; +import { shouldUseFullSizeLinkPreviewImage } from '../../linkPreviews/shouldUseFullSizeLinkPreviewImage'; import { AttachmentType, @@ -54,22 +56,10 @@ interface Trigger { handleContextClick: (event: React.MouseEvent) => void; } -// Same as MIN_WIDTH in ImageGrid.tsx -const MINIMUM_LINK_PREVIEW_IMAGE_WIDTH = 200; const STICKER_SIZE = 200; const SELECTED_TIMEOUT = 1000; const THREE_HOURS = 3 * 60 * 60 * 1000; -interface LinkPreviewType { - title: string; - description?: string; - domain: string; - url: string; - isStickerPack: boolean; - image?: AttachmentType; - date?: number; -} - export const MessageStatuses = [ 'delivered', 'error', @@ -850,12 +840,8 @@ export class Message extends React.PureComponent { Boolean(quote) || (conversationType === 'group' && direction === 'incoming'); - const previewHasImage = first.image && isImageAttachment(first.image); - const width = first.image && first.image.width; - const isFullSizeImage = - !first.isStickerPack && - width && - width >= MINIMUM_LINK_PREVIEW_IMAGE_WIDTH; + const previewHasImage = isImageAttachment(first.image); + const isFullSizeImage = shouldUseFullSizeLinkPreviewImage(first); const linkPreviewDate = first.date || null; @@ -1498,25 +1484,16 @@ export class Message extends React.PureComponent { } } - if (previews && previews.length) { - const first = previews[0]; - - if (!first || !first.image) { - return undefined; - } - const { width } = first.image; - - if ( - !first.isStickerPack && - isImageAttachment(first.image) && - width && - width >= MINIMUM_LINK_PREVIEW_IMAGE_WIDTH - ) { - const dimensions = getImageDimensions(first.image); - if (dimensions) { - // Add two for 1px border - return dimensions.width + 2; - } + const firstLinkPreview = (previews || [])[0]; + if ( + firstLinkPreview && + firstLinkPreview.image && + shouldUseFullSizeLinkPreviewImage(firstLinkPreview) + ) { + const dimensions = getImageDimensions(firstLinkPreview.image); + if (dimensions) { + // Add two for 1px border + return dimensions.width + 2; } } @@ -1547,10 +1524,6 @@ export class Message extends React.PureComponent { const first = previews[0]; const { image } = first; - if (!image) { - return false; - } - return isImageAttachment(image); } diff --git a/ts/components/conversation/StagedLinkPreview.tsx b/ts/components/conversation/StagedLinkPreview.tsx index e4e6d4f7e3c6..c31bba3eb207 100644 --- a/ts/components/conversation/StagedLinkPreview.tsx +++ b/ts/components/conversation/StagedLinkPreview.tsx @@ -32,7 +32,7 @@ export const StagedLinkPreview: React.FC = ({ date, domain, }: Props) => { - const isImage = image && isImageAttachment(image); + const isImage = isImageAttachment(image); return (
): boolean { + if (isStickerPack || !isImageAttachment(image)) { + return false; + } + + const { width, height } = image; + + return ( + isDimensionFullSize(width) && + isDimensionFullSize(height) && + !isRoughlySquare(width, height) + ); +} + +function isDimensionFullSize(dimension: unknown): dimension is number { + return ( + typeof dimension === 'number' && dimension >= MINIMUM_FULL_SIZE_DIMENSION + ); +} + +function isRoughlySquare(width: number, height: number): boolean { + return Math.abs(1 - width / height) < 0.05; +} diff --git a/ts/test-both/linkPreviews/shouldUseFullSizeLinkPreviewImage_test.ts b/ts/test-both/linkPreviews/shouldUseFullSizeLinkPreviewImage_test.ts new file mode 100644 index 000000000000..06212e0cd660 --- /dev/null +++ b/ts/test-both/linkPreviews/shouldUseFullSizeLinkPreviewImage_test.ts @@ -0,0 +1,146 @@ +// Copyright 2021 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { assert } from 'chai'; +import { IMAGE_JPEG, VIDEO_MP4 } from '../../types/MIME'; +import { AttachmentType } from '../../types/Attachment'; + +import { shouldUseFullSizeLinkPreviewImage } from '../../linkPreviews/shouldUseFullSizeLinkPreviewImage'; + +describe('shouldUseFullSizeLinkPreviewImage', () => { + const baseLinkPreview = { + title: 'Foo Bar', + domain: 'example.com', + url: 'https://example.com/foo.html', + isStickerPack: false, + }; + + const fakeAttachment = ( + overrides: Partial = {} + ): AttachmentType => ({ + contentType: IMAGE_JPEG, + fileName: 'foo.jpg', + url: '/tmp/foo.jpg', + width: 800, + height: 600, + ...overrides, + }); + + it('returns false if there is no image', () => { + assert.isFalse( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + }) + ); + }); + + it('returns false is the preview is a sticker pack', () => { + assert.isFalse( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + isStickerPack: true, + image: fakeAttachment(), + }) + ); + }); + + it("returns false if either of the image's dimensions are missing", () => { + assert.isFalse( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + image: fakeAttachment({ width: undefined }), + }) + ); + assert.isFalse( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + image: fakeAttachment({ height: undefined }), + }) + ); + assert.isFalse( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + image: fakeAttachment({ width: undefined, height: undefined }), + }) + ); + }); + + it("returns false if either of the image's dimensions are <200px", () => { + assert.isFalse( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + image: fakeAttachment({ width: 199 }), + }) + ); + assert.isFalse( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + image: fakeAttachment({ height: 199 }), + }) + ); + assert.isFalse( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + image: fakeAttachment({ width: 150, height: 199 }), + }) + ); + }); + + it('returns false if the image is square', () => { + assert.isFalse( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + image: fakeAttachment({ width: 200, height: 200 }), + }) + ); + assert.isFalse( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + image: fakeAttachment({ width: 500, height: 500 }), + }) + ); + }); + + it('returns false if the image is roughly square', () => { + assert.isFalse( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + image: fakeAttachment({ width: 200, height: 201 }), + }) + ); + assert.isFalse( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + image: fakeAttachment({ width: 497, height: 501 }), + }) + ); + }); + + it("returns false for large attachments that aren't images", () => { + assert.isFalse( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + image: fakeAttachment({ + contentType: VIDEO_MP4, + fileName: 'foo.mp4', + url: '/tmp/foo.mp4', + }), + }) + ); + }); + + it('returns true for larger images', () => { + assert.isTrue( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + image: fakeAttachment({ width: 200, height: 500 }), + }) + ); + assert.isTrue( + shouldUseFullSizeLinkPreviewImage({ + ...baseLinkPreview, + image: fakeAttachment(), + }) + ); + }); +}); diff --git a/ts/types/Attachment.ts b/ts/types/Attachment.ts index 14a73d9800ec..5277f24379c0 100644 --- a/ts/types/Attachment.ts +++ b/ts/types/Attachment.ts @@ -133,12 +133,12 @@ export function isImage( } export function isImageAttachment( - attachment: AttachmentType -): boolean | undefined { - return ( + attachment?: AttachmentType +): attachment is AttachmentType { + return Boolean( attachment && - attachment.contentType && - isImageTypeSupported(attachment.contentType) + attachment.contentType && + isImageTypeSupported(attachment.contentType) ); } export function hasImage( diff --git a/ts/types/message/LinkPreviews.ts b/ts/types/message/LinkPreviews.ts new file mode 100644 index 000000000000..a09ed1991742 --- /dev/null +++ b/ts/types/message/LinkPreviews.ts @@ -0,0 +1,14 @@ +// Copyright 2021 Signal Messenger, LLC +// SPDX-License-Identifier: AGPL-3.0-only + +import { AttachmentType } from '../Attachment'; + +export interface LinkPreviewType { + title: string; + description?: string; + domain: string; + url: string; + isStickerPack: boolean; + image?: AttachmentType; + date?: number; +} diff --git a/ts/util/lint/exceptions.json b/ts/util/lint/exceptions.json index 0e453b6a159d..747bd4bc649e 100644 --- a/ts/util/lint/exceptions.json +++ b/ts/util/lint/exceptions.json @@ -14782,7 +14782,7 @@ "rule": "React-createRef", "path": "ts/components/conversation/Message.js", "line": " this.audioRef = react_1.default.createRef();", - "lineNumber": 62, + "lineNumber": 61, "reasonCategory": "usageTrusted", "updated": "2020-08-28T16:12:19.904Z" }, @@ -14790,7 +14790,7 @@ "rule": "React-createRef", "path": "ts/components/conversation/Message.js", "line": " this.focusRef = react_1.default.createRef();", - "lineNumber": 63, + "lineNumber": 62, "reasonCategory": "usageTrusted", "updated": "2020-09-11T17:24:56.124Z", "reasonDetail": "Used for managing focus only" @@ -14799,7 +14799,7 @@ "rule": "React-createRef", "path": "ts/components/conversation/Message.js", "line": " this.reactionsContainerRef = react_1.default.createRef();", - "lineNumber": 64, + "lineNumber": 63, "reasonCategory": "usageTrusted", "updated": "2020-08-28T16:12:19.904Z", "reasonDetail": "Used for detecting clicks outside reaction viewer" @@ -14808,7 +14808,7 @@ "rule": "React-createRef", "path": "ts/components/conversation/Message.tsx", "line": " public audioRef: React.RefObject = React.createRef();", - "lineNumber": 226, + "lineNumber": 216, "reasonCategory": "usageTrusted", "updated": "2020-09-08T20:19:01.913Z" }, @@ -14816,7 +14816,7 @@ "rule": "React-createRef", "path": "ts/components/conversation/Message.tsx", "line": " public focusRef: React.RefObject = React.createRef();", - "lineNumber": 228, + "lineNumber": 218, "reasonCategory": "usageTrusted", "updated": "2020-09-08T20:19:01.913Z" }, @@ -14824,7 +14824,7 @@ "rule": "React-createRef", "path": "ts/components/conversation/Message.tsx", "line": " > = React.createRef();", - "lineNumber": 232, + "lineNumber": 222, "reasonCategory": "usageTrusted", "updated": "2020-08-28T19:36:40.817Z" },