-
Notifications
You must be signed in to change notification settings - Fork 165
Compute totalSize and itemSizeAndPositionData at the start when using a fixed or array as itemSize #61
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: master
Are you sure you want to change the base?
Compute totalSize and itemSizeAndPositionData at the start when using a fixed or array as itemSize #61
Changes from 4 commits
5bffa99
97bec66
2e8184f
ac9e42a
8f7cb4c
6069821
93eb2fe
b2e454e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,20 +14,22 @@ interface SizeAndPositionData { | |
} | ||
|
||
export interface Options { | ||
itemSize: ItemSize; | ||
itemCount: number; | ||
itemSizeGetter: ItemSizeGetter; | ||
estimatedItemSize: number; | ||
} | ||
|
||
export default class SizeAndPositionManager { | ||
private itemSizeGetter: ItemSizeGetter; | ||
private itemCount: number; | ||
private estimatedItemSize: number; | ||
private itemSize: ItemSize; | ||
private lastMeasuredIndex: number; | ||
private justInTime: boolean; | ||
private estimatedItemSize: number; | ||
private totalSize?: number; | ||
private itemSizeAndPositionData: SizeAndPositionData; | ||
|
||
constructor({itemCount, itemSizeGetter, estimatedItemSize}: Options) { | ||
this.itemSizeGetter = itemSizeGetter; | ||
constructor({itemSize, itemCount, estimatedItemSize}: Options) { | ||
this.itemSize = itemSize; | ||
this.itemCount = itemCount; | ||
this.estimatedItemSize = estimatedItemSize; | ||
|
||
|
@@ -36,13 +38,11 @@ export default class SizeAndPositionManager { | |
|
||
// Measurements for items up to this index can be trusted; items afterward should be estimated. | ||
this.lastMeasuredIndex = -1; | ||
|
||
this.processConfig(); | ||
} | ||
|
||
updateConfig({ | ||
itemCount, | ||
itemSizeGetter, | ||
estimatedItemSize, | ||
}: Partial<Options>) { | ||
updateConfig({itemSize, itemCount, estimatedItemSize}: Partial<Options>) { | ||
if (itemCount != null) { | ||
this.itemCount = itemCount; | ||
} | ||
|
@@ -51,9 +51,61 @@ export default class SizeAndPositionManager { | |
this.estimatedItemSize = estimatedItemSize; | ||
} | ||
|
||
if (itemSizeGetter != null) { | ||
this.itemSizeGetter = itemSizeGetter; | ||
if (itemSize != null) { | ||
this.itemSize = itemSize; | ||
} | ||
|
||
this.processConfig(); | ||
} | ||
|
||
/** | ||
* This is called when the SizeAndPositionManager is created and updated. | ||
*/ | ||
processConfig() { | ||
const {itemSize} = this; | ||
|
||
if (typeof itemSize === 'function') { | ||
this.totalSize = undefined; | ||
this.justInTime = true; | ||
} else { | ||
this.justInTime = false; | ||
this.computeTotalSizeAndPositionData(); | ||
} | ||
} | ||
|
||
/** | ||
* Compute the totalSize and itemSizeAndPositionData at the start, | ||
* when itemSize is a number or array. | ||
*/ | ||
computeTotalSizeAndPositionData() { | ||
const {itemSize, itemCount} = this; | ||
const itemSizeIsArray = Array.isArray(itemSize); | ||
const itemSizeIsNumber = typeof itemSize === 'number'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think having a private There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, I will bring this back in! |
||
|
||
let totalSize = 0; | ||
for (let i = 0; i < itemCount; i++) { | ||
let size; | ||
if (itemSizeIsNumber) { | ||
size = itemSize; | ||
} else if (itemSizeIsArray) { | ||
size = itemSize[i]; | ||
|
||
// Break when you are not supplying the same itemCount as available itemSizes. | ||
if (typeof size === 'undefined') { | ||
break; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call! |
||
} | ||
|
||
const offset = totalSize; | ||
totalSize += size; | ||
|
||
this.itemSizeAndPositionData[i] = { | ||
offset, | ||
size, | ||
}; | ||
} | ||
|
||
this.totalSize = totalSize; | ||
} | ||
|
||
getLastMeasuredIndex() { | ||
|
@@ -62,7 +114,6 @@ export default class SizeAndPositionManager { | |
|
||
/** | ||
* This method returns the size and position for the item at the specified index. | ||
* It just-in-time calculates (or used cached values) for items leading up to the index. | ||
*/ | ||
getSizeAndPositionForIndex(index: number) { | ||
if (index < 0 || index >= this.itemCount) { | ||
|
@@ -71,13 +122,26 @@ export default class SizeAndPositionManager { | |
); | ||
} | ||
|
||
if (this.justInTime) { | ||
return this.getJustInTimeSizeAndPositionForIndex(index); | ||
} | ||
|
||
return this.itemSizeAndPositionData[index]; | ||
} | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
|
||
/** | ||
* This is used when itemSize is a function. | ||
* just-in-time calculates (or used cached values) for items leading up to the index. | ||
*/ | ||
getJustInTimeSizeAndPositionForIndex(index: number) { | ||
if (index > this.lastMeasuredIndex) { | ||
const lastMeasuredSizeAndPosition = this.getSizeAndPositionOfLastMeasuredItem(); | ||
const itemSizeGetter = this.itemSize as ItemSizeGetter; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a rule of thumb, you should avoid casting in TypeScript unless absolutely necessary. In this case, TypeScript should be able to infer the type of Assuming you choose to follow my suggestion above to have an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will try and implement your comment above with always having the |
||
let offset = | ||
lastMeasuredSizeAndPosition.offset + lastMeasuredSizeAndPosition.size; | ||
|
||
for (let i = this.lastMeasuredIndex + 1; i <= index; i++) { | ||
const size = this.itemSizeGetter(i); | ||
const size = itemSizeGetter(i); | ||
|
||
if (size == null || isNaN(size)) { | ||
throw Error(`Invalid size returned for index ${i} of value ${size}`); | ||
|
@@ -105,10 +169,16 @@ export default class SizeAndPositionManager { | |
|
||
/** | ||
* Total size of all items being measured. | ||
* This value will be completedly estimated initially. | ||
* As items as measured the estimate will be updated. | ||
*/ | ||
getTotalSize(): number { | ||
// Return the pre computed totalSize when itemSize is number or array. | ||
if (this.totalSize) return this.totalSize; | ||
|
||
/** | ||
* When itemSize is a function, | ||
* This value will be completedly estimated initially. | ||
* As items as measured the estimate will be updated. | ||
*/ | ||
const lastMeasuredSizeAndPosition = this.getSizeAndPositionOfLastMeasuredItem(); | ||
|
||
return ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -134,13 +134,9 @@ export default class VirtualList extends React.PureComponent<Props, State> { | |
width: PropTypes.oneOfType([PropTypes.number, PropTypes.string]), | ||
}; | ||
|
||
itemSizeGetter = (itemSize: Props['itemSize']) => { | ||
return index => this.getSize(index, itemSize); | ||
}; | ||
|
||
sizeAndPositionManager = new SizeAndPositionManager({ | ||
itemCount: this.props.itemCount, | ||
itemSizeGetter: this.itemSizeGetter(this.props.itemSize), | ||
itemSize: this.props.itemSize, | ||
estimatedItemSize: this.getEstimatedItemSize(), | ||
}); | ||
|
||
|
@@ -189,7 +185,7 @@ export default class VirtualList extends React.PureComponent<Props, State> { | |
|
||
if (nextProps.itemSize !== itemSize) { | ||
this.sizeAndPositionManager.updateConfig({ | ||
itemSizeGetter: this.itemSizeGetter(nextProps.itemSize), | ||
itemSize: nextProps.itemSize, | ||
}); | ||
} | ||
|
||
|
@@ -199,7 +195,7 @@ export default class VirtualList extends React.PureComponent<Props, State> { | |
) { | ||
this.sizeAndPositionManager.updateConfig({ | ||
itemCount: nextProps.itemCount, | ||
estimatedItemSize: this.getEstimatedItemSize(nextProps), | ||
// estimatedItemSize: this.getEstimatedItemSize(nextProps), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Brainfart. |
||
}); | ||
} | ||
|
||
|
@@ -388,14 +384,6 @@ export default class VirtualList extends React.PureComponent<Props, State> { | |
); | ||
} | ||
|
||
private getSize(index: number, itemSize) { | ||
if (typeof itemSize === 'function') { | ||
return itemSize(index); | ||
} | ||
|
||
return Array.isArray(itemSize) ? itemSize[index] : itemSize; | ||
} | ||
|
||
private getStyle(index: number, sticky: boolean) { | ||
const style = this.styleCache[index]; | ||
|
||
|
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.
Have you considered making
justInTime
a getter? (sincejustInTime
is derived based on the type ofitemSize
)Assuming you did so, you'd probably be able to remove this method. I think the
processConfig
method is trying to handle too much currently, and the name isn't really indicative of what it does. Instead, I'd privilege explicitly callingcomputeTotalSizeAndPositionData
and resettingthis.totalSize
where necessary.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.
Yeah
processConfig
is a bit weird right now. I will take a look how to move this back in theconstructor
andupdateConfig