Skip to content

Commit

Permalink
[native] correctly measure multimedia messages that have an inline en…
Browse files Browse the repository at this point in the history
…gagement

Summary:
This diff handles correctly measuring multimedia messages. Multimedia needed to be handled a little bit differently since we use the image height and the composed message width to determine the `contentHeight` and we actually don't perform a measurement on multimedia messages.

This means that for multimedia messages we only need to measure the inline engagement and then in `multimediaMessageItemHeight` we can add the `contentHeight` w/ the height of the inline engagement we get from the `NodeHeightMeasurer`.

Depends on D8632

Test Plan:
I tested modifying the height of the inline engagement for multimedia messages, and when I did this I no longer got incorrect height measurement logs from the console. I performed the same tests outlined in the test plan D8632 (except edit label test, but you can't edit multimedia message)

Before:
{F654799}

After:
{F654800}

Reviewers: atul, kamil, ashoat, tomek

Reviewed By: tomek

Subscribers: rohan, ashoat, tomek

Differential Revision: https://phab.comm.dev/D8634
  • Loading branch information
ginsueddy committed Jul 31, 2023
1 parent 73b3551 commit 1930b1c
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 17 deletions.
1 change: 0 additions & 1 deletion native/chat/chat-constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ export const composedMessageStyle = {
};

export const inlineEngagementStyle = {
height: 38,
marginTop: 5,
marginBottom: 3,
topOffset: -10,
Expand Down
36 changes: 29 additions & 7 deletions native/chat/chat-item-height-measurer.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { entityTextToRawString } from 'lib/utils/entity-text.js';

import type { MeasurementTask } from './chat-context-provider.react.js';
import { useComposedMessageMaxWidth } from './composed-message-width.js';
import { dummyNodeForInlineEngagementHeightMeasurement } from './inline-engagement.react.js';
import { dummyNodeForRobotextMessageHeightMeasurement } from './inner-robotext-message.react.js';
import { dummyNodeForTextMessageHeightMeasurement } from './inner-text-message.react.js';
import type { NativeChatMessageItem } from './message-data.react.js';
Expand Down Expand Up @@ -51,6 +52,13 @@ const heightMeasurerKey = (item: NativeChatMessageItem) => {
sidebar: getInlineEngagementSidebarText(threadCreatedFromMessage),
reactions: reactionsToRawString(reactions),
});
} else if (threadCreatedFromMessage || Object.keys(reactions).length > 0) {
// we enter this condition when the item is a multimedia message with an
// inline engagement
return JSON.stringify({
sidebar: getInlineEngagementSidebarText(threadCreatedFromMessage),
reactions: reactionsToRawString(reactions),
});
}
return null;
};
Expand All @@ -62,24 +70,37 @@ const heightMeasurerDummy = (item: NativeChatMessageItem) => {
item.itemType === 'message',
'NodeHeightMeasurer asked for dummy for non-message item',
);
const { messageInfo, hasBeenEdited } = item;
const { messageInfo, hasBeenEdited, threadCreatedFromMessage, reactions } =
item;

if (messageInfo.type === messageTypes.TEXT) {
const label = getMessageLabel(hasBeenEdited, messageInfo.threadID);
return dummyNodeForTextMessageHeightMeasurement(
messageInfo.text,
label,
item.threadCreatedFromMessage,
item.reactions,
threadCreatedFromMessage,
reactions,
);
} else if (item.robotext) {
return dummyNodeForRobotextMessageHeightMeasurement(
item.robotext,
item.messageInfo.threadID,
item.threadCreatedFromMessage,
item.reactions,
messageInfo.threadID,
threadCreatedFromMessage,
reactions,
);
} else if (threadCreatedFromMessage || Object.keys(reactions).length > 0) {
// we enter this condition when the item is a multimedia message with an
// inline engagement

return dummyNodeForInlineEngagementHeightMeasurement(
threadCreatedFromMessage,
reactions,
);
}
invariant(false, 'NodeHeightMeasurer asked for dummy for non-text message');
invariant(
false,
'NodeHeightMeasurer asked for dummy for multimedia message with no inline engagement',
);
};

function ChatItemHeightMeasurer(props: Props) {
Expand Down Expand Up @@ -130,6 +151,7 @@ function ChatItemHeightMeasurer(props: Props) {
reactions: item.reactions,
hasBeenEdited: item.hasBeenEdited,
isPinned: item.isPinned,
inlineEngagementHeight: height,
...sizes,
};
}
Expand Down
21 changes: 20 additions & 1 deletion native/chat/inline-engagement.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,20 @@ import { useSelector } from '../redux/redux-utils.js';
import { useStyles } from '../themes/colors.js';
import type { ChatMessageInfoItemWithHeight } from '../types/chat-types.js';

function dummyNodeForInlineEngagementHeightMeasurement(
sidebarInfo: ?ThreadInfo,
reactions: ReactionInfo,
): React.Element<typeof View> {
return (
<View>
<DummyInlineEngagementNode
sidebarInfo={sidebarInfo}
reactions={reactions}
/>
</View>
);
}

type DummyInlineEngagementNodeProps = {
...React.ElementConfig<typeof View>,
+editedLabel?: ?string,
Expand Down Expand Up @@ -547,4 +561,9 @@ function TooltipInlineEngagement(
);
}

export { InlineEngagement, TooltipInlineEngagement, DummyInlineEngagementNode };
export {
InlineEngagement,
TooltipInlineEngagement,
DummyInlineEngagementNode,
dummyNodeForInlineEngagementHeightMeasurement,
};
22 changes: 14 additions & 8 deletions native/chat/multimedia-message-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { messageKey } from 'lib/shared/message-utils.js';
import type { MediaInfo } from 'lib/types/media-types.js';
import type { MultimediaMessageInfo } from 'lib/types/message-types.js';

import { inlineEngagementStyle, clusterEndHeight } from './chat-constants.js';
import { clusterEndHeight } from './chat-constants.js';
import { failedSendHeight } from './failed-send.react.js';
import { authorNameHeight } from './message-header.react.js';
import type {
Expand Down Expand Up @@ -104,10 +104,19 @@ function multimediaMessageContentSizes(
function multimediaMessageItemHeight(
item: ChatMultimediaMessageInfoItem,
): number {
const { messageInfo, contentHeight, startsCluster, endsCluster } = item;
const {
messageInfo,
contentHeight,
startsCluster,
endsCluster,
inlineEngagementHeight,
} = item;

const { creator } = messageInfo;
const { isViewer } = creator;
let height = 5 + contentHeight; // 5 from marginBottom in ComposedMessage

// 5 from marginBottom in ComposedMessage
let height = 5 + contentHeight;
if (!isViewer && startsCluster) {
height += authorNameHeight;
}
Expand All @@ -117,11 +126,8 @@ function multimediaMessageItemHeight(
if (multimediaMessageSendFailed(item)) {
height += failedSendHeight;
}
if (item.threadCreatedFromMessage || Object.keys(item.reactions).length > 0) {
height +=
inlineEngagementStyle.height +
inlineEngagementStyle.marginTop +
inlineEngagementStyle.marginBottom;
if (inlineEngagementHeight) {
height += inlineEngagementHeight;
}
return height;
}
Expand Down
4 changes: 4 additions & 0 deletions native/types/chat-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ export type ChatTextMessageInfoItemWithHeight = {
+isPinned: ?boolean,
};

// We "measure" the contentHeight of a multimedia message using the media
// dimensions. This means for multimedia messages we only need to actually
// measure the inline engagement node
export type MultimediaContentSizes = {
+imageHeight: number,
+contentHeight: number,
Expand All @@ -62,6 +65,7 @@ export type ChatMultimediaMessageInfoItem = {
+reactions: ReactionInfo,
+hasBeenEdited: ?boolean,
+isPinned: ?boolean,
+inlineEngagementHeight: ?number,
};

export type ChatMessageInfoItemWithHeight =
Expand Down

0 comments on commit 1930b1c

Please sign in to comment.