Keep-alive for widgets in lazy lists#11010
Conversation
|
cc @cbracken for review |
cbracken
left a comment
cbracken
left a comment
There was a problem hiding this comment.
Still digging around the codebase to better understand context, but sending off first round of nitpicks.
LGTM for the change in general. Just a few nitpicks.
There was a problem hiding this comment.
Just ${keepAlive ? "keepAlive; " : ""}? Also, leading space that shouldn't be there.
There was a problem hiding this comment.
Nothing prevents it from being false, and I wouldn't want the toString to crash if it was false (which it would if I didn't do == true).
There was a problem hiding this comment.
added a test to verify that nobody regresses this
There was a problem hiding this comment.
Shorter: .forEach(_childManager.removeChild), or go for a local and a for-loop to avoid the forEach with a function literal.
There was a problem hiding this comment.
Why the .toList() here? Can removeChild indirectly modify _keepAliveBucket?
There was a problem hiding this comment.
Yes, removeChild very much modifies _keepAliveBucket, that's the purpose of the call in fact. :-)
There was a problem hiding this comment.
added a comment to that effect
There was a problem hiding this comment.
Is it worth adding the caveat that addRepaintBoundaries must not be null here?
There was a problem hiding this comment.
We do that on the constructors, by convention.
There was a problem hiding this comment.
Assert that addRepaintBoundaries is non-null.
There was a problem hiding this comment.
Since this is no longer a closure, you could kill off bool result and return the literals instead.
I suspect I've got some sort of obsession relating to shortening lines code though -- probably dates back to those one-liners in the back of Compute! magazine in the 80s.
|
Updated per comments. Will land on green unless I hear otherwise. |
|
|
3 participants
No description provided.