Skip to content

Always align when line breaking - #941

Merged
DJMcNab merged 8 commits into
linebender:mainfrom
DJMcNab:always-align
Nov 3, 2026
Merged

DJMcNab merged 8 commits into
linebender:mainfrom
DJMcNab:always-align

Conversation

@DJMcNab

@DJMcNab DJMcNab commented Oct 31, 2026

Copy link
Copy Markdown
Member

LLM Contributions: Initially generated, review.

From what I've seen, every consumer of Parley always immediately calls align after breaking, without ever re-aligning.
As such, we can take advantage of this to store the results of spacing in the alignment pass, massively simplifying glyph iteration (and making future features - tabs and CJK justification - a lot easier to add performantly). This is as was proposed in #office hours > Text 2026-10-27. See also #935.

This change is the API diff needed to realise that performance gain.

Performance numbers for this just noise (as it doesn't actually change the operations which happen).

Working on this change has made it quite starkly apparent how our current Layout/line breaking APIs are very unintuitive. I don't necessarily have a complete answer for what to do about it, but I've put a discussion in the agenda for next week.

Changelog

Major Changes

Breaking change: Layout::align has been removed. You must now instead use Layout::set_alignment before line breaking:

   let mut layout = builder.build(...);
+ layout.set_alignment(Alignment::Center, AlignmentOptions::default());
  layout.break_all_lines(Some(max_width));
- layout.align(Alignment::Center, AlignmentOptions::default());

Start alignment is now the default (it was previously implicitly Left), so an align(Alignment::Start, AlignmentOptions::default) call can be removed instead.

Changed

  • Breaking change: Layout::align has been removed in favour of Layout::set_alignment.

@tomcur tomcur left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment thread parley/src/layout/alignment.rs Outdated
Comment on lines +47 to +48
/// If set to `None`, those lines use the alignment passed as the first argument to [`Layout::set_alignment`],
/// except that [`Alignment::Justify`] falls back to [`Alignment::Start`]. This is the behavior of

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "as the first argument" is perhaps a bit too much detail here.

Comment thread parley/src/layout/layout.rs Resolved
Comment thread parley/src/layout/alignment.rs Outdated
Comment on lines +68 to +73
/// Align the layout. This applies to the full layout at a time (rather than to individual lines as we go),
/// because the alignment implicitly uses the widest width in the layout if no max advance was provided.
///
/// TODO: if we ever end up getting rid of multi-paragraph layout, can we avoid that; is this what
/// `calculate_content_widths` is for instead?
pub(crate) fn align<B: Brush>(layout: &mut LayoutData<B>) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// TODO: if we ever end up getting rid of multi-paragraph layout, can we avoid that; is this what
/// calculate_content_widths is for instead?

Yeah, I think if there's a single paragraph (assuming you mean no forced breaks) we could always align as we go. calculate_content_widths is really quite fast, so I suppose we could theoretically call that if there's no max advance and use the max width to break/align to. (The max width should be correct, the min can theoretically differ a bit as it can't reasonably account for reshaping).

Is aligning as we go noticeably faster?

@DJMcNab DJMcNab Nov 2, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the TODO to track seeing if as-we-go would be faster.

@DJMcNab
DJMcNab enabled auto-merge November 2, 2026 23:29
@DJMcNab
DJMcNab disabled auto-merge November 3, 2026 00:06
@DJMcNab
DJMcNab enabled auto-merge November 3, 2026 00:06
@DJMcNab
DJMcNab added this pull request to the merge queue Nov 3, 2026
Merged via the queue into linebender:main with commit fe8ea13 Nov 3, 2026
24 checks passed
@DJMcNab
DJMcNab deleted the always-align branch November 3, 2026 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants