Skip to content
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

Fix for getItemType causing incorrect measures #25431

Merged
merged 8 commits into from
Jan 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 25 additions & 28 deletions shared/chat/conversation/list-area/index.native.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import SpecialTopMessage from '../messages/special-top-message'
import sortedIndexOf from 'lodash/sortedIndexOf'
import type * as Types from '../../../constants/types/chat2'
import type {ItemType} from '.'
import {Animated /*, View*/} from 'react-native'
import {Animated} from 'react-native'
import {ConvoIDContext} from '../messages/ids-context'
import {FlashList, type ListRenderItemInfo} from '@shopify/flash-list'
import {getMessageRender} from '../messages/wrapper'
Expand Down Expand Up @@ -64,10 +64,10 @@ const AnimatedChild = React.memo(function AnimatedChild({children, animatingKey}

type SentProps = {
children?: React.ReactElement
conversationIDKey: Types.ConversationIDKey
ordinal: Types.Ordinal
}
const Sent_ = ({conversationIDKey, ordinal}: SentProps) => {
const Sent_ = ({ordinal}: SentProps) => {
const conversationIDKey = React.useContext(ConvoIDContext)
const {type, youSent} = Container.useSelector(state => {
const you = state.config.username
const message = state.chat2.messageMap.get(conversationIDKey)?.get(ordinal)
Expand Down Expand Up @@ -194,13 +194,9 @@ const ConversationList = React.memo(function ConversationList(p: {

const listRef = React.useRef<FlashList<ItemType> | null>(null)
const {markInitiallyLoadedThreadAsRead} = Hooks.useActions({conversationIDKey})
const keyExtractor = React.useCallback(
(_item: ItemType, index: number) => {
const ordinal = messageOrdinals[index]
return String(ordinal)
},
[messageOrdinals]
)
const keyExtractor = React.useCallback((ordinal: ItemType) => {
return String(ordinal)
}, [])
const renderItem = React.useCallback(
(info: ListRenderItemInfo<ItemType> | null | undefined) => {
const index = info?.index ?? 0
Expand All @@ -209,26 +205,26 @@ const ConversationList = React.memo(function ConversationList(p: {
return null
}
if (!index) {
return <Sent ordinal={ordinal} conversationIDKey={conversationIDKey} />
return <Sent ordinal={ordinal} />
}

const type = messageTypeMap?.get(ordinal) ?? 'text'
if (!type) return null
const Clazz = getMessageRender(type)
if (!Clazz) return null
return <Clazz ordinal={ordinal} />
// used to debug measuring issues w/ items
// uncomment to debug measuring issues w/ items
// return (
// <View
// onLayout={e => {
// console.log('aaa', ordinal, e.nativeEvent.layout.height)
// console.log('debug', ordinal, e.nativeEvent.layout.height)
// }}
// >
// <Clazz ordinal={ordinal} />
// </View>
// )
},
[messageOrdinals, conversationIDKey, messageTypeMap]
[messageOrdinals, messageTypeMap]
)

const recycleTypeRef = React.useRef(new Map<Types.Ordinal, string>())
Expand All @@ -239,19 +235,20 @@ const ConversationList = React.memo(function ConversationList(p: {
recycleTypeRef.current.set(ordinal, type)
}, [])

// put this back when https://github.com/Shopify/flash-list/issues/600 is figured out
// const getItemType = React.useCallback(
// (ordinal: Types.Ordinal, idx: number) => {
// if (!ordinal) {
// return 'null'
// }
// if (messageOrdinals.length - 1 === idx) {
// return 'sent'
// }
// return recycleTypeRef.current.get(ordinal) ?? messageTypeMap?.get(ordinal) ?? 'text'
// },
// [messageOrdinals, messageTypeMap]
// )
const numOrdinals = messageOrdinals.length

const getItemType = React.useCallback(
(ordinal: Types.Ordinal, idx: number) => {
if (!ordinal) {
return 'null'
}
if (numOrdinals - 1 === idx) {
return 'sent'
}
return recycleTypeRef.current.get(ordinal) ?? messageTypeMap?.get(ordinal) ?? 'text'
},
[numOrdinals, messageTypeMap]
)

const {scrollToCentered, scrollToBottom, onEndReached} = useScrolling({
centeredOrdinal,
Expand Down Expand Up @@ -288,7 +285,7 @@ const ConversationList = React.memo(function ConversationList(p: {
overScrollMode="never"
contentContainerStyle={styles.contentContainer}
data={messageOrdinals}
getItemType={undefined /*getItemType*/}
getItemType={getItemType}
inverted={true}
renderItem={renderItem}
maintainVisibleContentPosition={maintainVisibleContentPosition}
Expand Down
17 changes: 17 additions & 0 deletions shared/chat/conversation/messages/text/wrapper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,23 @@ const WrapperText = React.memo(function WrapperText(p: Props) {
setRecycleType(ordinal, 'text' + subType)
}

// Uncomment to test effective recycling
// const DEBUGOldOrdinalRef = React.useRef(0)
// const DEBUGOldTypeRef = React.useRef('')
// React.useEffect(() => {
// const oldtype = DEBUGOldTypeRef.current
// if (DEBUGOldOrdinalRef.current) {
// console.log(
// 'debug textwrapperRecycle',
// DEBUGOldOrdinalRef.current,
// ordinal,
// subType === oldtype ? `SAME ${subType}` : `${subType} != ${oldtype} <<<<<<<<<<<<<<<<<`
// )
// }
// DEBUGOldOrdinalRef.current = ordinal
// DEBUGOldTypeRef.current = subType
// }, [ordinal, subType])

const style = React.useMemo(
() => getStyle(textType, isEditing, showCenteredHighlight),
[textType, isEditing, showCenteredHighlight]
Expand Down
2 changes: 1 addition & 1 deletion shared/local-debug.native.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ const config = {

// Developer settings
if (__DEV__) {
config.enableActionLogging = true
config.enableActionLogging = false
config.enableStoreLogging = false
config.immediateStateLogging = false
// Move this outside the if statement to get notifications working
Expand Down
2 changes: 1 addition & 1 deletion shared/patches/@shopify+flash-list+1.4.0.patch
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
diff --git a/node_modules/@shopify/flash-list/dist/FlashList.js b/node_modules/@shopify/flash-list/dist/FlashList.js
index ea05c45..f194afa 100644
index ea05c45..59ab7ab 100644
--- a/node_modules/@shopify/flash-list/dist/FlashList.js
+++ b/node_modules/@shopify/flash-list/dist/FlashList.js
@@ -24,7 +24,7 @@ var FlashList = /** @class */ (function (_super) {
Expand Down
44 changes: 44 additions & 0 deletions shared/patches/recyclerlistview+4.2.0.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
diff --git a/node_modules/recyclerlistview/dist/reactnative/core/layoutmanager/LayoutManager.js b/node_modules/recyclerlistview/dist/reactnative/core/layoutmanager/LayoutManager.js
index 9cd3c57..f65012e 100644
--- a/node_modules/recyclerlistview/dist/reactnative/core/layoutmanager/LayoutManager.js
+++ b/node_modules/recyclerlistview/dist/reactnative/core/layoutmanager/LayoutManager.js
@@ -114,7 +114,7 @@ var WrapGridLayoutManager = /** @class */ (function (_super) {
for (var i = startIndex; i < itemCount; i++) {
oldLayout = this._layouts[i];
var layoutType = this._layoutProvider.getLayoutTypeForIndex(i);
- if (oldLayout && oldLayout.isOverridden && oldLayout.type === layoutType) {
+ if (oldLayout && oldLayout.isOverridden /*&& oldLayout.type === layoutType*/) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the relationship between the types has no greater significance here. If its overridden keep the size or it can get lost as its possible it never gets re-added and then fires an onLayout losing the size forever

itemDim.height = oldLayout.height;
itemDim.width = oldLayout.width;
}
@@ -145,9 +145,12 @@ var WrapGridLayoutManager = /** @class */ (function (_super) {
itemRect.x = startX;
itemRect.y = startY;
itemRect.type = layoutType;
+ const nextOverriden = !!itemRect.isOverridden && itemRect.width === itemDim.width && itemRect.height === itemDim.height;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we happened to adopt the size and have the same dimensions we can keep the override flag or we don't have it

+ itemRect.isOverridden = nextOverriden
itemRect.width = itemDim.width;
itemRect.height = itemDim.height;
}
+
if (this._isHorizontal) {
startY += itemDim.height;
}
diff --git a/node_modules/recyclerlistview/dist/reactnative/platform/reactnative/viewrenderer/ViewRenderer.js b/node_modules/recyclerlistview/dist/reactnative/platform/reactnative/viewrenderer/ViewRenderer.js
index d9c7031..dfc6d92 100644
--- a/node_modules/recyclerlistview/dist/reactnative/platform/reactnative/viewrenderer/ViewRenderer.js
+++ b/node_modules/recyclerlistview/dist/reactnative/platform/reactnative/viewrenderer/ViewRenderer.js
@@ -44,9 +44,10 @@ var ViewRenderer = /** @class */ (function (_super) {
};
_this._onLayout = function (event) {
//Preventing layout thrashing in super fast scrolls where RN messes up onLayout event
- var xDiff = Math.abs(_this.props.x - event.nativeEvent.layout.x);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i haven't see this messed up RN onLayout event but, this can trip up the FlashList as it makes an assumption that when reportItemLayout is called the dimensions have actually been updated (https://github.com/Shopify/flash-list/blob/main/src/GridLayoutProviderWithProps.ts#L104). So we just disable this path

- var yDiff = Math.abs(_this.props.y - event.nativeEvent.layout.y);
- if (xDiff < 1 && yDiff < 1 &&
+ //var xDiff = Math.abs(_this.props.x - event.nativeEvent.layout.x);
+ //var yDiff = Math.abs(_this.props.y - event.nativeEvent.layout.y);
+
+ if (/*xDiff < 1 && yDiff < 1 &&*/
(_this.props.height !== event.nativeEvent.layout.height ||
_this.props.width !== event.nativeEvent.layout.width)) {
_this._dim.height = event.nativeEvent.layout.height;