DirectLoadingStrategy is just for testing #2219
Conversation
| awaitSnapshot() | ||
| assertThat(index5ComposeCount).isEqualTo(1) | ||
|
|
||
| // Scrolling to a non-contiguous range causes a recomposition. |
There was a problem hiding this comment.
This was awkward because it needed to pick bounds carefully.
| @@ -92,11 +91,6 @@ public class ScrollOptimizedLoadingStrategy( | |||
|
|
|||
| // Expand the range depending on scroll direction. | |||
| when { | |||
| // Ignore preloads. | |||
| !preloadItems -> { | |||
There was a problem hiding this comment.
Seems fine, this was only ever used in one unit test anyway
| /** | ||
| * A loading strategy that's appropriate for tests because it's simple and predictable. | ||
| * | ||
| * This is not suitable for production use it must show a placeholder before an item is loaded. |
There was a problem hiding this comment.
Is this two sentences, or missing a comma or something?
There was a problem hiding this comment.
Fixed!
* This is not suitable for production use, because it must show a placeholder before an item is
* loaded.
| * This is not suitable for production use it must show a placeholder before an item is loaded. | ||
| */ | ||
| public class DirectLoadingStrategy : LoadingStrategy { | ||
| private var loadRange: IntRange by mutableStateOf(0..0) |
There was a problem hiding this comment.
| private var loadRange: IntRange by mutableStateOf(0..0) | |
| private var loadRange: IntRange = 0..0 |
The only read is firstIndex which is only used when saving. Don't think you need a full-on state object tracking this value.
There was a problem hiding this comment.
Don’t I need this to trigger a recomposition after the user scrolls?
There was a problem hiding this comment.
I don't think the composition reads this. Maybe I missed it.
There was a problem hiding this comment.
It calls loadRange(), which reads variables here. I assume using State variables is the easiest way to express the data dependency.
| * | ||
| * This is not suitable for production use it must show a placeholder before an item is loaded. | ||
| */ | ||
| public class DirectLoadingStrategy : LoadingStrategy { |
There was a problem hiding this comment.
naming nit: I think the name should make it abundantly clear that this should never be used in production. Something like Fake/Test/Mock instead of Direct would help with this.
There was a problem hiding this comment.
Good idea. Changed to TestLoadingStrategy.
I'm using longer names to disambiguate two related ranges: - the visible range - the loaded range
CHANGELOG.md's "Unreleased" section has been updated, if applicable.