Make container_width a required alignment parameter

[tomcur]

Some prior discussion here: #278 (comment).

Aligning to the layout's calculated width is not necessarily a sensible default, as that width changes depending on how exactly lines were broken given the container width (max_advance) used for line breaking.

[tomcur]

Added this to the 0.3 milestone, as we probably should resolve this decision before 0.3 to reduce churn.

[nicoburns]

If we don't think layout.width() is a good default then we should probably also change the tests to use 40.0 here.

[nicoburns]

Ditto the other tests that are line-broken with a specified width.

[tomcur]

Yes... you're probably right. I was a bit hesitant due to the snapshot churn it'd cause.

[tomcur]

We can also continue the discussion on whether container_width should be part of AlignmentOptions here. If it's non-Option (as proposed here), it can't be. If it's Option, I think it should be.

My current thinking is exactly because there's no good no-context default, we should just make container_width a required parameter. Having access to Layout::width and setting the alignment width to that in case of missing container_width is not really a good substitute, as that depends on exactly how the lines were broken. In a text box, the alignment would now jitter as users are typing.

One alternative, reusing the max_advance of line breaking for alignment probably also won't really work, as floated boxes likely impact max_advance.

[nicoburns]

One alternative, reusing the max_advance of line breaking for alignment probably also won't really work, as floated boxes likely impact max_advance.

They will?

[wfdewith]

One alternative, reusing the max_advance of line breaking for alignment probably also won't really work, as floated boxes likely impact max_advance.

My understanding was that we should be moving to using line.max_advance for alignment anyway as the separate alignment width mostly existed to account for (near) infinite line.max_advance. One of the motivations for #259 was to have a sensible cap on that. If we want to support floated boxes, we need to be able to align every line individually anyway.

[tomcur]

One alternative, reusing the max_advance of line breaking for alignment probably also won't really work, as floated boxes likely impact max_advance.

They will?

Maybe? I'm not exactly sure yet what it will look like, but one option for line breaking in the presence of floated boxes is to line-break line-by-line, allowing the caller to adjust max_advance based on whether at this position there's now a box (or polygon!) that the text needs to wrap around.

[tomcur]

My understanding was that we should be moving to using line.max_advance for alignment anyway as the separate alignment width mostly existed to account for (near) infinite line.max_advance.

That's a good point actually, requiring a sensible max_advance to be set by the user in all cases (which we don't currently require). How I pictured it before your point, was to align to f32::min(container_width, line.max_advance), to work around that issue.

[DJMcNab]

I know it doesn't really help us now, but this would be a great use case for rust-lang/rust#132162

[wfdewith]

My understanding was that we should be moving to using line.max_advance for alignment anyway as the separate alignment width mostly existed to account for (near) infinite line.max_advance.

That's a good point actually, requiring a sensible max_advance to be set by the user in all cases (which we don't currently require). How I pictured it before your point, was to align to f32::min(container_width, line.max_advance), to work around that issue.

We don't even need a sensible max_advance to be set by the user, we can always do f32::min(user_max_advance, max_content_width), and this should do what the user intended, even if they set a much larger max_advance.

@nicoburns do you by any chance remember if there were other motivations than the one I mentioned for the separate alignment width?

[tomcur]

We don't even need a sensible max_advance to be set by the user, we can always do f32::min(user_max_advance, max_content_width), and this should do what the user intended, even if they set a much larger max_advance.

I'm not sure it would. Consider the case where the user has text that is not allowed to wrap, and should instead overflow its container if it's too wide. The max_advance is infinite, but the container has a definite width and (with default AlignmentOptions) the layout should be start-aligned to that container.

If my thinking is correct, this probably means with the current methods we can't use max_advance for alignment after all (except when line.max_advance is smaller than container_width).

[nicoburns]

Consider the case where the user has text that is not allowed to wrap, and should instead overflow its container if it's too wide. The max_advance is infinite.

This could also be represented by a finite max_advance but with a line-breaking mode that never breaks. I think #259 does open up the possibility of requiring max_advance (making it non-optional). Although I'm a little reluctant to jump on that change until #259 has been proven in production.

@nicoburns do you by any chance remember if there were other motivations than the one I mentioned for the separate alignment width?

Blitz is currently doing this for alignment width (scroll up for max_advance). It's possible it could be made to work with them being the same, but I couldn't make it work when I first implemented this. This funky alignment_width was the motivation for separating out alignment into a separate step with it's own width, as it didn't seem right to pass all that info into Parley.