I would like another review or two on this one… timebix it to within 48 hours.

For clarity, From<f32> was not what I had in mind, but rather From<LineHeight> for StyleProperty (like From<GenericFamily> for StyleProperty, where there is no ambiguity).
The builders accept impl Into<StyleProperty>, which is why we are able to push a GenericFamily directly.

For clarity, From<f32> was not what I had in mind, but rather From<LineHeight> for StyleProperty (like From<GenericFamily> for StyleProperty, where there is no ambiguity). The builders accept impl Into<StyleProperty>, which is why we are able to push a GenericFamily directly.

That would make much more sense 🤦

I am concerned about the ergonomics. I'm also not sure about having something where almost everything in Parley is absolute, but this one was relative, but now we want to make it do both.

My main concern is that going from relative to absolute units while keeping the same type for both would result in existing code silently doing the wrong thing, and API consumers would have to track down all the places they're passing in either a StyleProperty::LineHeight or a TextStyle.

If we’re going to make this configurable then it would be nice to offer an option to fully respect font metrics as the code originally did.

That is, I’d like to see something like:

enum LineHeight {
    MetricsRelative(f32),
    SizeRelative(f32),
    Absolute(f32),
}

where the first is the original behavior, the second is the current behavior and the last does what this PR proposes. For the first two variants, the associated value is a multiplier. The default should be MetricsRelative(1.0) because this is generally what you want unless you’re dealing with CSS.

In a better world, the line breaker would take a callback to determine line height so we don’t have to be opinionated but we’re not there yet.

The other arguments which I think can be dismissed:

• CSS overloads the line-height property to mean both

• It's a pretty bad breaking change

There is another argument, which is one of defaults: any default absolute line height is stupid.

Is that third argument one that you're putting forward or dismissing?

If we’re going to make this configurable then it would be nice to offer an option to fully respect font metrics as the code originally did.

Agreed. If others are fine with your proposed API, I could try implementing that. I think egui's old font layout code may also be metrics-relative?

In a better world, the line breaker would take a callback to determine line height so we don’t have to be opinionated but we’re not there yet.

This ties into some discussion in #325 (from @nicoburns):

My main observation beyond what you've written is that the BreakLines struct is already well setup for incremental layout where Parley lays content out up to a point, yields control flow back to the caller, and then the caller run it's own logic and calls back into Parley when it's ready.

This would definitely be very helpful. Right now, the line breaking code doesn't seem to even calculate line height until the finish method after all the lines have been broken (right now, unless I'm doing something wrong, the vertical advance returned by BreakLines::break_next is always 0.0 as a consequence). I'm not sure if this is intrinsic to the architecture or just how it happens to be implemented now, but it'd be nice to change.

Some CSS vertical alignment properties require two-pass layout vertically (they're relative to the bounds of the line, but the bounds of the line depend on vertical alignment, so items with those alignments are aligned in a second pass). Not sure how that would interact with the line breaking code if we want to support custom callbacks.

There is another argument, which is one of defaults: any default absolute line height is stupid.

Is that third argument one that you're putting forward or dismissing?

I'm putting it forward; that is to say, I don't dismiss it out of hand. I don't class it as a "definite" blocker for an "absolute line height only" world, but it's an argument against it.

I think with Chad's proposal, we can actually remove SizeRelative entirely. If we want a mode based on the font's metrics (which is pretty reasonable), then I agree that a default based on metrics is what we want. Although any such method does still have the issue of jumpy (/potentially overlapping(?)) rounded line heights.

I agree that if BreakLines would let you handle this, say by post-hoc specifying the "real height" of a line once it is broken, that would make the most sense. That way, you can implement whatever rounding you like even when you're metrics aware. But that needn't happen in this PR; it's orthogonal.

So concretely, my suggested way forward is to change this to:

enum LineHeight {
    MetricsRelative(f32),
    Absolute(f32),
}

and wire that up. Wiring up RelativeSize if we decide we want it can be a very quick follow-up, as a rewrite layer into Absolute (which is why I say we don't need it; the user can do so themselves if they want it).

My take on SizeRelative is that I think it's nice from an API perspective (esp. if we're going to have an enum anyway) as it effectively documents how to achieve that effect. And it's a minimal cost to us from a maintenance perspective (a single multiply). But I don't feel super-strongly about this as it is indeed easy to build this on top of absolute line height.

I agree with Nico that having SizeRelative is an easy ergonomics win if we're already going enum. The implementation burden is trivial and it doesn't really make API consumers' life harder, because resulting line heights are always just absolute f32, so no matching required.

Thus, I support Chad's API proposal of having all three variants.

I'll let someone else redo this PR with whatever consensus exists. This seems like a quagmire and the current hack/workaround is good enough for my egui work.