Skip to content
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.

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

Hang trailing whitespace preceding explicit newline #276

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 22 additions & 7 deletions parley/src/layout/line/greedy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -535,14 +535,29 @@ impl<'a, B: Brush> BreakLines<'a, B> {
line.metrics.trailing_whitespace = run
.filter(|item| item.is_text_run())
.and_then(|run| {
let cluster = if self.layout.is_rtl() {
self.layout.data.clusters[run.cluster_range.clone()].first()
if self.layout.is_rtl() {
// An RTL layout that has a line that ends with a space followed by a
// forced line break will always have a separate run that looks like:
// [" ", "\n"]. This means that the first cluster of a run with trailing
Comment on lines +539 to +541
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why this is? Perhaps with an example? I believe you, but in terms of providing a meaningful review, I'm not currently fully following the logic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I have to be honest here, and tell you that I don't exactly know why RTL works like this. I assume that the space can be that the explicit newline forces mixed direction runs in the BiDi algorithm, but from my first skimming of https://www.unicode.org/reports/tr9/, line separators should be treated the same as any white space.

In any case, the current shaper gives you three runs like this: [RTL("ااااا"), LTR(" \n"), RTL("بببب")], with the explicit newline. Without it you get just one run like this: [RTL("بببب ااااا"].

I'll make this PR a draft until I understand it better and can explain it.

// white space is always a space, regardless of whether the forced line
// break follows, so we don't have to check for forced line breaks
// explicitly.
self.layout.data.clusters[run.cluster_range.clone()]
.first()
.filter(|cluster| cluster.info.whitespace().is_space_or_nbsp())
.map(|cluster| cluster.advance)
} else {
self.layout.data.clusters[run.cluster_range.clone()].last()
};
cluster
.filter(|cluster| cluster.info.whitespace().is_space_or_nbsp())
.map(|cluster| cluster.advance)
match &self.layout.data.clusters[run.cluster_range.clone()] {
[.., a, b]
if a.info.whitespace().is_space_or_nbsp()
&& b.info.whitespace() == Whitespace::Newline =>
{
Some(a.advance + b.advance)
}
[.., a] if a.info.whitespace().is_space_or_nbsp() => Some(a.advance),
_ => None,
}
}
})
.unwrap_or(0.0);

Expand Down
52 changes: 42 additions & 10 deletions parley/src/tests/test_basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,18 +116,50 @@ fn full_width_inbox() {
fn trailing_whitespace() {
let mut env = testenv!();

let text = "AAA BBB";
let mut builder = env.ranged_builder(text);
let mut layout = builder.build(text);
layout.break_all_lines(Some(45.));
layout.align(None, Alignment::Start, false);
{
let text = "AAA BBB";
let mut builder = env.ranged_builder(text);
let mut layout = builder.build(text);
layout.break_all_lines(Some(45.));
layout.align(None, Alignment::Start, false);

assert!(
layout.width() < layout.full_width(),
"Trailing whitespace should cause a difference between width and full_width"
);
env.with_name("soft_wrap").check_layout_snapshot(&layout);
}

env.check_layout_snapshot(&layout);
{
let text = "AAA \nBBB";
let mut builder = env.ranged_builder(text);
let mut layout = builder.build(text);
layout.break_all_lines(None);
layout.align(None, Alignment::Start, false);

env.with_name("hard_wrap").check_layout_snapshot(&layout);
}
}

#[test]
fn trailing_whitespace_rtl() {
let mut env = testenv!();

{
let text = "بببب ااااا";
let mut builder = env.ranged_builder(text);
let mut layout = builder.build(text);
layout.break_all_lines(Some(45.));
layout.align(None, Alignment::Start, false);

env.with_name("soft_wrap").check_layout_snapshot(&layout);
}

{
let text = "بببب \nااااا";
let mut builder = env.ranged_builder(text);
let mut layout = builder.build(text);
layout.break_all_lines(None);
layout.align(None, Alignment::Start, false);

env.with_name("hard_wrap").check_layout_snapshot(&layout);
}
}

#[test]
Expand Down
3 changes: 3 additions & 0 deletions parley/tests/snapshots/trailing_whitespace-soft_wrap.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions parley/tests/snapshots/trailing_whitespace_rtl-hard_wrap.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions parley/tests/snapshots/trailing_whitespace_rtl-soft_wrap.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.