Fix #1213: support for list-based content replacement in squoosh #1294
Conversation
|
Snapshot diff report vs base branch: main |
|
No need to review yet, I wanted to see what CI would do with it, but this somehow shows the design switcher's list items. We don't seem to be measuring the list items correctly yet -- we get a width of 299px out, but a height of 0px, and then still manage to lay them out reasonably in the list (!!). Next, I'll try to figure out what's going on with the height, and then see if the intrinsic width/height accessors can be made more efficient by implementing them directly (I'm not quite sure how they work right now -- Compose calls the base layout method in response to a query for the intrinsic size, but I don't know how that works, because Compose also tries to only do a single traversal in one layout pass, and this would seemingly violate that...). |
iamralpht
force-pushed
the
user/ralph/squoosh-list-content-replacement
branch
from
35e196a
to
404c300
Compare
iamralpht
changed the title
|
|
||
| composableList.add( | ||
| SquooshChildComposable( | ||
| component = @Composable { childComponent() }, |
There was a problem hiding this comment.
This is a bit clunky; I'm not sure if there's a nicer way to do this than adding an empty wrapper function, though.
There was a problem hiding this comment.
So the component is reused for list replacement, but the ComponentReplacementContext is unused?
|
This now seems to work well enough for the Design Switcher, so I removed the WIP status. I'll rebase on main next. |
This change implements support for list-based component replacement in Squoosh by adding synthetic layout nodes into Squoosh's layout tree which correspond to the list items in a list-based component replacement. During Compose layout, Squoosh calls the Rust layout implementation, which can then interrogate the list item children for their intrinsic sizes. If the list items are DesignCompose children, then their measure method is called multiple times to compute the intrinsic sizes. There are several opportunities to optimize this implementation (by special casing the DesignCompose child case, by providing a dedicated intrinsic measurement implementation instead of Compose's default, etc).
iamralpht
force-pushed
the
user/ralph/squoosh-list-content-replacement
branch
from
404c300
to
d09130e
Compare
| val customMeasurables = HashMap<Int, Measurable>() | ||
| val customMeasure: (Measurable, Float, Float, Float, Float) -> SizeF? = | ||
| { measurable, width, height, availableWidth, availableHeight -> | ||
| val maxConstraint = 3e5f |
There was a problem hiding this comment.
Where does this value come from?
There was a problem hiding this comment.
I need a Float version of Compose's Constraints.Infinity. I'll see if just calling toFloat catches the same bogus dimensions. That would be better than introducing a new constant here.
|
|
||
| composableList.add( | ||
| SquooshChildComposable( | ||
| component = @Composable { childComponent() }, |
There was a problem hiding this comment.
So the component is reused for list replacement, but the ComponentReplacementContext is unused?
| Dimension.Percent(0f) | ||
| ) | ||
|
|
||
| internal fun defaultLayoutStyle(): LayoutStyle.Builder { |
There was a problem hiding this comment.
You can't just call LayoutStyle.Builder()? Do those defaults not match with these?
There was a problem hiding this comment.
No, it has null for most stuff and then throws an exception when you call build() that those things can't be null. It's really not helpful and I hope that the proto generated Builders are better in this regard.
| StrokeAlign.Center(), | ||
| StrokeWeight.Uniform(5.0f), | ||
| listOf(Background.Solid(ColorOrVar.Color(Color(listOf(0x7F, 0x0, 0x0, 0x7F))))) | ||
| ) |
| listChildScrollInfo.overflow = OverflowDirection.NONE() | ||
|
|
||
| val listChildView = View.Builder() | ||
| listChildView.unique_id = (node.view.unique_id + 0x3000 + childIdx).toShort() |
There was a problem hiding this comment.
Can you add a comment to describe how/why this is calculated this way?
There was a problem hiding this comment.
Yeah, good call! I think I should move all of the layout id calculating stuff to one place. I'm a bit worried that it's easy to introduce new ways to make unique layout ids which actually might conflict.
| listChildView.render_method = RenderMethod.None() | ||
| listChildView.explicit_variable_modes = Optional.empty() | ||
|
|
||
| val layoutId = rootLayoutId + node.layoutId + 0x30000000 + childIdx |
There was a problem hiding this comment.
I am confused how this is related to the listChildView.unique_id calculated above.
There was a problem hiding this comment.
Not sure but does node.layoutId already have rootLayoutId added in it, causing this to be bigger than expected?
|
|
|
Closing this out, as I have a new change which addresses most of the issues listed here, except for the overlapping Design Switcher content. |
Support list-based content replacement in Squoosh by:
measurefunction to bridge between the Compose layout implementation of the child, and the hosting layout node in the Squoosh tree.These two changes mean we can host DesignCompose items or regular Compose items as replaced content, because we're just using the regular Compose API for layout (which DesignCompose also implements).