Skip to content

Compute whitespace properties and line metrics for each line eagerly #420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 29, 2025
+226 −205
Merged

Compute whitespace properties and line metrics for each line eagerly #420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
61a5667
Move `compute_whitespace_properties` to method
nicoburns Sep 23, 2025
a0cfd51
Make line metrics computation work on a per-line basis
nicoburns Sep 23, 2025
4181b05
PR feedback changes
nicoburns Oct 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions parley/src/layout/data.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,11 @@ impl LineItemData {
self.kind == LayoutItemKind::TextRun
}

#[inline(always)]
pub(crate) fn is_rtl(&self) -> bool {
self.bidi_level & 1 != 0
}

pub(crate) fn compute_line_height<B: Brush>(&self, layout: &LayoutData<B>) -> f32 {
match self.kind {
LayoutItemKind::TextRun => {
Expand Down Expand Up @@ -235,6 +240,41 @@ impl LineItemData {
}
}
}

/// If the item is a text run
/// - Determine if it consists entirely of whitespace (`is_whitespace` property)
/// - Determine if it has trailing whitespace (`has_trailing_whitespace` property)
pub(crate) fn compute_whitespace_properties<B: Brush>(&mut self, layout_data: &LayoutData<B>) {
Copy link
Contributor

@taj-p taj-p Oct 29, 2025

Choose a reason for hiding this comment

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

Should we add a test for this change? (I know it's changing existing functionality that I don't believe has a test (?), but it might be nice to add anyway to protect against future regression)

Copy link
Contributor Author

@nicoburns nicoburns Oct 29, 2025

Choose a reason for hiding this comment

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

Are you thinking a unit test for the function (probably doesn't make sense if we're going to change it again soon anyway) or an intergration test for the functionality (probably makes sense, but I'd need to work out what this actually affects).

Copy link
Contributor

@taj-p taj-p Oct 29, 2025

Choose a reason for hiding this comment

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

It's probably a touch faster to run this as a running calculation during breaking (similar to #443) for the degenerate case of submitting thousands of whitespace, but I think we might be able to consider that an extremely unexpected / malicious input

Copy link
Contributor Author

@nicoburns nicoburns Oct 29, 2025

Choose a reason for hiding this comment

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

Makes sense. Perhaps we could even compute it earlier in pipeline as part of "text analysis". Certainly that seems possible for the "entirely whitespace" flag, which would leave the cheaper trailing whitespace (in fact if we already knew whether a run was entirely whitespace then we'd only need to check one cluster to check trailing whitespace)

// Skip items which are not text runs
if self.kind != LayoutItemKind::TextRun {
return;
}

self.is_whitespace = true;
if self.is_rtl() {
// RTL runs check for "trailing" whitespace at the front.
for cluster in layout_data.clusters[self.cluster_range.clone()].iter() {
if cluster.info.is_whitespace() {
self.has_trailing_whitespace = true;
} else {
self.is_whitespace = false;
break;
}
}
} else {
for cluster in layout_data.clusters[self.cluster_range.clone()]
.iter()
.rev()
{
if cluster.info.is_whitespace() {
self.has_trailing_whitespace = true;
} else {
self.is_whitespace = false;
break;
}
}
}
}
}

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
Expand Down
Loading