parley: add style-run builder; accept precomputed runs from styled_text#516
Conversation
|
This builds on top of PR #495 so only look at the right commit here ... but this shows a way to build a |
|
This is still doing some style resolution via |
4e0a8ae to
7f92f04
Compare
|
Makes a lot of sense. #387 does something very similar. |
|
I will note that I think the "flat list of non-overlapping styles" model may be insufficient to represent some tree-shaped data. For example, it's going to be hard to represent |
|
Why is that span example hard to represent? It's not clear to me, especially as there is no BiDi in that example. |
|
@nicoburns This is probably just the first phase ... I think those sorts of things might change with time. There's a lot of stuff downstream from this. |
@xStrom The key thing is that the two spans start/end at exactly the same offset in the text. So in the model where the styles apply to glyphs in the text, how do you determine that you have "pushed" or "popped" two spans? For most styles (e.g. styles the modify the text itself) this doesn't matter: two nested spans isn't really any different to one span that applies both sets of styles. But for (horizontal) padding, the two nested spans here imply that the I think that to implement this you would need some kind of modelling of the tree structure within Parley (perhaps each style struct having |
|
@waywardmonkeys Unrelated to my previous comment: Is there a way to reuse styles with this API? For example, in Basically, it might nice to have a layer of indirection between the style structs and the text ranges, so that more than one text range can point to the same style struct. |
|
Regarding I haven't checked the PR but in genral style reuse via references seems like an important memory optimization. Just imagine a text where there's a bold word every now and then. You only need two actual style structs for this, not a multiple of how many bold words you have. |
We're in the first steps of a long journey. |
Handling it as part of flattening is interesting possibility that for some reason I had not considered. I'd need to think it through, but I think this would probably work.
Luckily margin collapsing only applies to block-level elements, not inline-level elements, so we don't need to deal with this in Parley. |
The |
taj-p
left a comment
There was a problem hiding this comment.
I love the idea of enabling an API into Parley that bypasses style resolution - to pass precomputed runs. Please re-request review once you're ready
You’re right that first and third can have identical computed styles, but on We can do better as part of a separate patch series that allows for style sharing and then adapt the builders to potentially support that (or a fast path for this (I have several branches iterating on this concept and will submit some PRs soon.) |
d6a7c3c to
4c55df4
Compare
4c55df4 to
a3f7d6b
Compare
a3f7d6b to
b1ceee4
Compare
|
I've now removed everything from this except for the changes intended to land. This is ready for review. |
| /// Reserves additional capacity for styles and runs. | ||
| /// | ||
| /// This is an optional optimization for callers that know counts | ||
| /// up front; call it before pushing styles and runs to reduce | ||
| /// reallocations. | ||
| pub fn reserve(&mut self, additional_styles: usize, additional_runs: usize) { | ||
| self.lcx.style_table.reserve(additional_styles); | ||
| self.lcx.style_runs.reserve(additional_runs); | ||
| } |
There was a problem hiding this comment.
Might be interesting at some point to have some reuse, so that there's not a reallocation cycle every time... but I guess I won't know until I'm using it.
There was a problem hiding this comment.
These are reused already. This is just another knob.
b1ceee4 to
388dcf5
Compare
|
This has an approval, so I am likely to land this in 12-24 hours unless I hear otherwise. It would be good to have feedback from the people who are interested in using it apart from my own code. |
|
I'm currently unwell, so I only skimmed, but this looks pretty reasonable to me. |
taj-p
left a comment
There was a problem hiding this comment.
I ran out of time today. But this definitely looks reasonable 👍🎉
LayoutContext::style_run_builderand aStyleRunBuilderAPI for constructing a Layout from contiguous, non-overlapping style runs, skipping Parley’s internal range-splitting step.styled_text_parleyto lower StyledText’s computed runs directly intoStyleRunBuilderusingTextStyle<'static, '_, B>.StyleRunBuilderoutput matchesRangedBuilderfor equivalent styles.