pop-os / cosmic-text

Pure Rust multi-line text handling
https://pop-os.github.io/cosmic-text/cosmic_text/
Apache License 2.0
1.58k stars 100 forks source link

Let a `Buffer` figure out its `height` during `set_size` #70

Closed hecrj closed 3 months ago

hecrj commented 1 year ago

While it is possible to use i32::MAX if there is no height limit for a Buffer, the resulting Buffer will use i32::MAX as the final dimension. This means that Buffer::visible_lines will not be very useful.

I think it'd be more useful to let layouting dictate the final height of a Buffer.

jackpot51 commented 1 year ago

@hecrj could you provide some guidance on a desired API?

nicoburns commented 1 year ago

@jackpot51 I'm not Hector, but perhaps I can help here, as I submitted https://github.com/pop-os/cosmic-text/issues/42 which I believe is more or less a duplicate of this issue (except it requests that the Buffer be able to figure out both dimensions, not just height).

(you may also wish to read the comments on that issue, as there is more discussion than here).

Background / Motivation

I think the key thing to understand here is that UI layout algorithms may want to determine their layout based on the size of an unconstrained text layout, rather than giving the text box an explicit size ahead of time.

An obvious case is laying out a vertical page within a scroll view, where you want a paragraph of text to take up however much vertical space it needs: it doesn't matter how much space that is so long as you can know how much space that ends up being. I believe it is possible to get this information out of cosmic-text currently by manually iterating of layout lines, but it's not particularly nice or easy. It would be nice if that height was calculated during layout and stored in an easy to access property.

A less obvious, but currently more problematic case is if you want to leave the width unspecified. As documented in https://github.com/pop-os/cosmic-text/issues/42#issuecomment-1607731931, you can kind of do this by setting the width to some large value, but this breaks alignment as alignment as the alignment will be based on that large size rather than the actual with of the text. It would be much better if that width could explicitly be set to "undefined" which would then cause alignment to use the computed width.

This could be done by making set_size on Buffer take Option<i32> rather than i32 (or perhaps Option<u32> as I don't think negative dimensions make sense).

An extra bonus for interop with CSS style layout.

(Regarding users of cosmic-text: Bevy and Floem are both using Taffy which implements this style of layout)

CSS-style layout modes actually have two separate "undefined" layout modes - "min-content" and "max-content". Specifically for horizontal text, these should be thought of as constraints on the width of the laid out text:

These sizes are used as hard bounds between which the layout algorithm can decide the final size of the text-containing box.

Suggested API

Add the following struct:

struct SizeConstraint {
     MinContent,
     MaxContent,
     Definite(u32),
 }

Extension: pass input dimensions to layout method(s)

Perhaps this is going out of scope a little, but it might be nice if the "input dimensions" weren't stored on the Buffer at all, but were passed to the method that performs the layout. This is because in many layout systems, a text buffer doesn't have a single set of "input dimensions" (perhaps better thought of as "size constraints"), but may need to be sized repeatedly under multiple constraints to determine the final layout. But this change is much less important and doesn't block use of cosmic-text.

jackpot51 commented 1 year ago

Thank you @nicoburns for the description, I will look into this.

Imberflur commented 1 year ago

I've seen cases in iced text layout that the proposed SizeConstraint doesn't cover (afaict):

  1. A finite maximum width is specified but the size of the box shrinks to the size of the text.
  2. The fill width (from iced::layout::Limits) will be used if it is greater than the measured text width but less than the max width. (Although I'm not familiar with whether any arrangement of widgets could produce this case)
nicoburns commented 1 year ago

I believe (1) would be partially covered by making the output dimension in each axis:

max(min(input size, max-content size), min-content size)

Then the UI layout can use the output width.

I'm not 100% sure how that would interact with alignment though. If you want to customise whether the alignment was based on the specified width vs. the measured width then I guess you would need some kind of extra input (either a flag or a second size).

Imberflur commented 1 year ago

I somewhat feel like needing to cover all these behaviors (with output size and alignment) is a motivation for having a separate measurement operation that doesn't interact with alignment at all. Thus, allowing the user to operate on the measured dimensions with any behavior desired to determine the final bounds that they pass to a function that does alignment.

Although this might be a bit out of scope. And I guess the user can already do this by just performing full layout twice, so this could just be considered a future optimization. Also, it may not easily fit in the current Buffer API.

hecrj commented 1 year ago

@nicoburns described the use cases very well. I also like the design proposed.

iced does not have plans to support the min-content layout approach, at least for now; so that's not a priority for us.

In iced, we are currently rolling our own logic to measure a Buffer:

https://github.com/iced-rs/iced/blob/a3489e4af960388e9f73988b88df361022a654a4/wgpu/src/text.rs#L293-L301

This is what prompted me to create this issue. The sizes we initially use for the Buffer are, as @nicoburns described, maximum boundaries; but we want the Buffer to end up with minimum ones.

DasLixou commented 11 months ago

Bump

Is there any ongoing work here?

maxall41 commented 11 months ago

Bump

ElhamAryanpur commented 10 months ago

Bump

ElhamAryanpur commented 8 months ago

bump

jackpot51 commented 3 months ago

This should be fixed in #271, which allows a Buffer to be created without a defined width and size. Buffer::layout_runs can then be used to figure out the actual size. If there are follow-up changes to request, please make a new issue.