Push SliverToBoxAdapter down to SingleColumnDocumentLayout

[knopp]

Background: The way SuperEditor currently handles document widgets where all the widgets in the document are instantiated at all times is presenting a significant bottleneck to us. We aim to implement an alternative document layout based on super_sliver_list, but in order to accommodate a sliver inside layout some adjustments needs to be made to SuperEditor infrastructure.

This PR ensures that when SuperEditor is placed into a CustomScrollView, it propagates the sliver geometry all the way down to the layout, instead of relying to be wrapped inside a SliverToBoxAdapter.

Breaking use-facing changes introduced in this PR:

• To use SuperEditor inside a custom scroll view, it must no longer be wrapped in a SliverToBoxAdapter. That's because the SuperEditor itself acts like a sliver. When SuperEditor is used as "standalone" widget, no changes are necessary.
• It is no longer possible to use IntrinsicHeight around SuperEditor, as this does not work with CustomScrollView, which is now used instead of SingleChildScrollView in DocumentScrollable. Instead of that, shrinkWrap argument has been added to SuperEditor and SuperReader to accomplish similar behavior.
• Getting the document layout state and casting it to RenderBox will fail, because the layout is a RenderSliver.

Some implementation details:

SliverHybridStack widget was introduced to allow combining slivers and render boxes. Exactly on of the children in the stack must produce a RenderSliver, while other children produce RenderObjects. The render objects are laid-out and painted in a way where they cover the entire sliver scrollExtent.

ContentLayers has been modified to work similar to SliverHybridStack, where the content is a sliver while the overlays and underlays are render boxes. I think this is acceptable solution as the underlays and overlays are expected to be much sparsely populated then the content itself.

ViewportBoundsReporter and RenderViewportBoundsReplicator are gone. The viewport bounds are part of sliver constraints. SliverHybridStack has an optional mode which ensures that the renderboxes fill up entire viewport, which is enabled for gesture system RenderBox in case the entire works in standalone mode (not in user provided scrollable).

[knopp]

Note that the affinity: TextAffinity.upstream was actually incorrect, and was just an artifact of AutoScroller.deltaWhileAutoScrolling being one frame late, giving slightly wrong base position after scrolling.

[matthew-carroll]

I haven't looked at the source code yet, but I have some thoughts based on the write-up. Mostly, I want to make sure that we don't over correct in the direction of slivers, to the detriment of those who would like to operate on boxes.

Getting the document layout state and casting it to RenderBox will fail, because the layout is a RenderSliver.

Unless I'm misunderstanding, this seems like a problem for those who want to work with boxes and not slivers. Boxes make it trivial to calculate offsets and assemble visual rectangle regions, so my first thought is that we don't want to eliminate the ability to treat a document as a box, if the user is actually using it as a box.

ContentLayers has been modified to work similar to SliverHybridStack, where the content is a sliver while the overlays and underlays are render boxes. I think this is acceptable solution as the underlays and overlays are expected to be much sparsely populated then the content itself.

I've long planned to move ContentLayers into its own package because I think it's generally useful. In so doing, I'm sure people will want to use boxes as the content. Should we instead look into the options to have two different content layer implementations - one for boxes and one for slivers? We'd need to chat about the optimal API for such a thing, but I thought I'd introduce this point at the start of the review.

[knopp]

I haven't looked at the source code yet, but I have some thoughts based on the write-up. Mostly, I want to make sure that we don't over correct in the direction of slivers, to the detriment of those who would like to operate on boxes.

Slivers are a superset of boxes. Whatever you want to do with boxes you can do with slivers.

Getting the document layout state and casting it to RenderBox will fail, because the layout is a RenderSliver.

Unless I'm misunderstanding, this seems like a problem for those who want to work with boxes and not slivers. Boxes make it trivial to calculate offsets and assemble visual rectangle regions, so my first thought is that we don't want to eliminate the ability to treat a document as a box, if the user is actually using it as a box.

We can expose the underlying box from the document if needed. But you can calculate those from slivers too if needed.

ContentLayers has been modified to work similar to SliverHybridStack, where the content is a sliver while the overlays and underlays are render boxes. I think this is acceptable solution as the underlays and overlays are expected to be much sparsely populated then the content itself.

I've long planned to move ContentLayers into its own package because I think it's generally useful. In so doing, I'm sure people will want to use boxes as the content. Should we instead look into the options to have two different content layer implementations - one for boxes and one for slivers? We'd need to chat about the optimal API for such a thing, but I thought I'd introduce this point at the start of the review.

This would make things much more complicated. Basically to be able to use sliver as layout, every render object produced by super editor all the way down to the layout must be a RenderSliver. If you want to use both boxes and slivers throughout to super editor you'll need to duplicate all the render objects and then pass along a flag to know which one to produce.

[matthew-carroll]

Slivers are a superset of boxes. Whatever you want to do with boxes you can do with slivers.

Let's dig into this claim a bit. Certainly, from an API standpoint, this isn't the case. RenderSliver extends RenderObject and RenderBox extends RenderObject. It's not the case that RenderSliver extends RenderBox. For easy reader reference, here are some API links:

https://api.flutter.dev/flutter/rendering/RenderObject-class.html

https://api.flutter.dev/flutter/rendering/RenderBox-class.html

https://api.flutter.dev/flutter/rendering/RenderSliver-class.html

---

Looking beyond the inheritance differences, let's look at a few methods and properties.

It's very common to want to call localToGlobal and globalToLocal. These methods don't exist on RenderSliver. Perhaps that's an equally simple way of achieving the same result, but I'm not sure what it is. Can you point that out?

RenderBox exposes its size, which, again, is very useful. Given the localToGlobal and size, it's trivial to find the global bounding rect for a given box. However, RenderSliver doesn't have a size property. It has a getAbsoluteSize() method, but that's marked "protected". RenderSliver has a geometry, but that geometry doesn't have a size property. Instead, the geometry reports things like paintExtent, layoutExtent, cacheExtent, etc. Is there a combination of these properties that would recover the expected return value of size on a RenderBox?

We've already talked a little bit about intrinsic width and height. While it may be acceptable for a specific chosen SuperEditor layout to use slivers, and therefore not support intrinsic measurement, I think it's reasonable that a developer might want a SuperEditor layout that does support intrinsic measurement. Is there a workaround for this with slivers? If the choice between boxes and slivers is provided, then this point doesn't matter. But if we're essentially migrating exclusively to slivers, then this does matter.

---

I have some concerns in other areas as well, but let's start by aligning on what the relationship really looks like between slivers and boxes. I want to make sure we have a crystal clear picture for everyone involved as to what it means to eliminate the guarantee of boxes and force people to either work with slivers, or force people to work only with generic RenderObjects.

[matthew-carroll]

Let me also loop in some other stakeholders here for visibility. CC @Jethro87 @jmatth

[knopp]

@matthew-carroll, please look at render_object_ext inside the PR. It implements size, hasSize, globalToLocal and localToGlobal on RenderObject, which can be either a RenderSliver or RenderBox.

[knopp]

The alternative for intrinsic height is shrinkWrap. As is common for other scrollable widgets.

[matthew-carroll]

The alternative for intrinsic height is shrinkWrap. As is common for other scrollable widgets.

But shrinkWrap is a request, not a query, right? Whereas a RenderBox getMaxIntrinsicWidth is a query, rather than a request?

[knopp]

The alternative for intrinsic height is shrinkWrap. As is common for other scrollable widgets.

But shrinkWrap is a request, not a query, right? Whereas a RenderBox getMaxIntrinsicWidth is a query, rather than a request?

Yes. ShrinkWrap will cause the scrollview to size itself to content, but does not allow querying intrinsic size. The question is, whether this difference is significant enough in practice to warrant supporting both slivers and boxes through-out super editor, which would increase the complexity significantly and add pretty big surface for runtime errors in cases where sliver and boxes would be mixed by mistake.

[matthew-carroll]

The question is, whether this difference is significant enough in practice to warrant supporting both slivers and boxes through-out super editor, which would increase the complexity significantly and add pretty big surface for runtime errors in cases where sliver and boxes would be mixed by mistake.

I agree that's the final question. I'm trying to start by putting all the tradeoffs on the table.

I'll take a look at your implementation of box stuff in the sliver extensions.

[matthew-carroll]

I did an initial pass of the PR. Overall I'd say it's a good sign that relatively little code was needed to migrate SuperEditor and SuperReader behavior to a sliver world.

I do have concerns about some places where this migration has narrowed the composability and/or features of existing artifacts, including ContentLayers and the SingleColumnDocumentLayout. I'm also concerned about the RenderObject extension that bridges between RenderSliver and RenderBox.

[matthew-carroll]

To help understand UX tradeoffs, I'm going to note the places like this where a SliverHybridStack apparently is required, but is probably a surprise to the developer. I'm sure the explanation for why this is needed here is very simple, but even reading this now, I can't immediately see why slivers are relevant to this little subtree.

Related to that concern is a concern is about composition. If the reason that this subtree is wrapped in a SliverHybridStack is because it happens to be a top-level child within SuperEditor, then that means we're installing global knowledge about where a given widget sits within the SuperEditor tree. There are the widgets which are immediate children of the scrolling SuperEditor system, and then there are all other widgets. Discriminating between those positions in the tree makes it more difficult to support generic widget composition within SuperEditor.

[knopp]

The fact that SuperEditor insist on bringing it own scrolling infrastructure is the surprising part to me. This seems like a peculiar decision and results in a rather magical behavior where SuperEditor builds differently when placed into a scrollable or not.

If SuperEditor did not bring it's own scrollable, then conceptually every layer of it would need to be sliver, which would basically mean replacing all Stack instances with SliverHybridStack and making ContentLayers a sliver.

However because super editor does bring it's own scrollable sometimes, the PR made some adjustment to ensure that the scrollable is at the top level, and all layers are within the scrollable. There are no layers of SuperEditor anymore on top of scrollable.

So the only difference is that when not put into a scrollable, SuperEditor wraps itself in a CustomScrollView. All the other layers remain same regardless of whether put in a scrollable or not. This also means that each layer of SuperEditor is a sliver now. I'm hoping that explains all the "Surprise" sliver references. Yes, this PR turns SuperEditor into a sliver, which means every layer of it down to the layout is a Sliver.

Turning SuperEditor into a sliver does make things slightly more complicated, but I don't see what other way there is to achieve acceptable performance for medium to large documents.

[matthew-carroll]

Surprise sliver reference.

[matthew-carroll]

Surprise sliver reference.

[matthew-carroll]

Are there any implications with this definition of constraints in the case that SuperEditor is used with an ancestor scrollable, in which there's additional scrollable content above and/or below the editor?

[knopp]

None I can think of.

[matthew-carroll]

Can you do this without defining a method inside of a method?

[matthew-carroll]

Given how convoluted render objects are, can you please document each of the standard behaviors that you've added and adjusted in this PR? I'd like to make sure that every Super Editor contributor is able to understand the intended behavior of these overrides.

[matthew-carroll]

Why was a CustomScrollView added here?

[knopp]

Becuase ContentLayers is a Sliver and as such it needs to be put in a CustomScrollView.

[knopp]

I tried running Superlist with this branch, and apart from making sure there are no render boxes outside of editor (which is a given) the only change I needed was to not make assumption about the layout object render type:

That meant changing code that positions overlay from

    final docBox =
        _documentLayoutKey.currentContext?.findRenderObject() as RenderBox?;
    if (docBox == null) return;

    final position = Rect.fromPoints(
      docBox.localToGlobal(selectionBoundingBox.topLeft),
      docBox.localToGlobal(selectionBoundingBox.bottomRight),
    );

Into

    final layout = _documentLayoutKey.currentState as DocumentLayout;
    final position = Rect.fromPoints(
      layout.getGlobalOffsetFromDocumentOffset(selectionBoundingBox.topLeft),
      layout.getGlobalOffsetFromDocumentOffset(selectionBoundingBox.bottomRight),
    );

[matthew-carroll]

@knopp I think the most reasonable path forward is to just land this and see how it shakes out. If we need to bring more box stuff back in, we can.

Can you address the few minor comments that were originally made?

[matthew-carroll]

@knopp ping on this

[knopp]

Apologies for the delay. Right now trying to dogfooding this in superlist to figure out if there are some loose ends that need to be addressed in this PR.