Conversation
|
(Travis failure is dependency on #7367) |
There was a problem hiding this comment.
Ah ha! No! I'm ahead of you on this one. I wrote the code and published it on github in 2016! :-P
There was a problem hiding this comment.
assert that args aren't null.
There was a problem hiding this comment.
This file should probably be sliver.dart to parallel box.dart
There was a problem hiding this comment.
Is operator== allowed to throw exceptions?
There was a problem hiding this comment.
Apparently not. I'll remove this assert.
There was a problem hiding this comment.
Should we add a const constructor to make this into a const?
There was a problem hiding this comment.
I've removed the assert from the constructor and made the main constructor const.
There was a problem hiding this comment.
Would "consumedExtent" be a more accurate name? Presumably the sliver doesn't actually need to be opaque in this region.
There was a problem hiding this comment.
It actually needs to be opaque, but I've updated the docs to be clearer.
There was a problem hiding this comment.
Also changed the default to 0.0. In practice this is ignored for most slivers anyway, added mention of that to the docs too.
There was a problem hiding this comment.
Never mind. I'm removing opaqueExtent entirely. Careful examination revealed the embarrassing truth that it was never actually used.
There was a problem hiding this comment.
I'd make these separate asserts so that its easier to see which one failed.
There was a problem hiding this comment.
It's checking that they're not both true simultaneously.
There was a problem hiding this comment.
Is this needed for some mixin thing?
There was a problem hiding this comment.
Removed. I had copied in the interface and just forgot to remove the ones I didn't want to reimplement yet.
There was a problem hiding this comment.
Should this assert move to the base class?
There was a problem hiding this comment.
The base class has a default implementation which makes sense for most kinds of RenderObject.
For RenderSliver, there's no reasonable default paint transform.
What I'd really like to do is "re-abstract" the method, but Dart doesn't support that yet (see dart-lang/sdk#28250 for a suggested @mustOverride which we could use though).
There was a problem hiding this comment.
I'd grab the canvas into a final local
There was a problem hiding this comment.
ChangeNotifiers can't be const.
There was a problem hiding this comment.
Do you want to do any sort of debug-time validation of the center sliver?
There was a problem hiding this comment.
It's validated in performLayout. I don't want to validate it here because that would prevent you from setting the center pointer before adding the child, which, in addition to being annoying in general, would be particularly annoying for us at the Widgets layer IIRC.
There was a problem hiding this comment.
This is kind of ugly, but I understand why you did it this way.
There was a problem hiding this comment.
Yeah. I could also pass a struct around and have it be filled in then pull the values at the end and put them into the fields, but it seemed unnecessary since it's all private anyway. Happy to change it if you prefer it that way though.
There was a problem hiding this comment.
I'd skip this variable and just declare it on line 1437
There was a problem hiding this comment.
I'd just return this directly instead of capturing it into a local.
There was a problem hiding this comment.
Why do we paint the children after the center backwards?
There was a problem hiding this comment.
So that appbars end up on top.
There was a problem hiding this comment.
Couldn't you, just as easily, want an app-bar like effect at the bottom of the screen? It just seems like an odd sop to headers over footers given that we paint almost everything else in child-order.
There was a problem hiding this comment.
Even for a bottom effect, you'd still want the app bar to paint after the things after it in the list, otherwise it couldn't overlap them.
There was a problem hiding this comment.
But maybe I'm misunderstanding what effect you're looking for?
There was a problem hiding this comment.
Let's talk it over with a whiteboard on monday. In any case, don't feel blocked about landing this patch.
|
|
|
Something, something code-to-test ratio. |
|
The code is tested more by the stuff I have coming at the widgets layer, but yes, it's undertested for sure. Are there any specific things you'd like to see tested before I land it? I listed some things that do need testing sooner rather than later at the end of the included test file. |
|
Updated. |
Doesn't need to block landing, but we should probably test that scrolling a viewport doesn't trigger extraneous layout (e.g., of box children or of siblings of the viewport). I think the way you have things setup, if you have a shrinkwrapping viewport, scrolling will invalidate layout for the viewport's container. That's probably necessary but unfortunate. |
Will do.
Yeah, shrink-wrapping the viewport is all kinds of expensive. Arguably it's a bad feature to even support in the first place. I mostly just have it because otherwise if you put a viewport in an unbounded area, you'd assert, and we've had bad experience with that kind of thing. |
This implements a new RenderViewport2 class to replace the existing RenderViewport class.
|
Added test that we don't relayout the parent when changing a non-shrink-wrapping viewport: https://github.com/Hixie/flutter/blob/f0bbe7783371a4b648226744dfc7a4b1847dde08/packages/flutter/test/rendering/slivers_layout_test.dart Will land when it goes green. |
4 participants
This implements a new RenderViewport2 class to replace the existing
RenderViewport class.