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

Wrap::Word inserts newline for trailing space (after word) #155

Closed
grovesNL opened this issue Jul 16, 2023 · 1 comment
Closed

Wrap::Word inserts newline for trailing space (after word) #155

grovesNL opened this issue Jul 16, 2023 · 1 comment

Comments

@grovesNL
Copy link
Contributor

grovesNL commented Jul 16, 2023

If we have text like Testing Spacing in the rich-text example, you can see that the space causes a full empty line to be inserted (tested with the rich-text example):

image

(the cursor also does an interesting thing in this situation)

The behavior seems to have changed in #78. Before #78 this would look like:

image

The previous behavior seems better for display, because I wouldn't expect a space to wrap a whole line. Instead I'd expect the space to kind of bind to the word before it, so that "Testing " remains on the first line.

If the old behavior is preferred, I'll try to submit a PR to change how we wrap in this case.

The current handling for word wrapping is located in

cosmic-text/src/shape.rs

Lines 831 to 873 in aa1b37a

// Wrap::Word
let mut trailing_space_width = None;
if let Some(previous_word) = span.words.get(i + 1) {
// Current word causing a wrap is not whitespace, so we ignore the
// previous word if it's a whitespace
if previous_word.blank {
trailing_space_width =
Some(previous_word.x_advance * font_size);
number_of_blanks = number_of_blanks.saturating_sub(1);
}
}
if let Some(width) = trailing_space_width {
add_to_visual_line(
&mut current_visual_line,
span_index,
(i + 2, 0),
fitting_start,
word_range_width - width,
number_of_blanks,
);
} else {
add_to_visual_line(
&mut current_visual_line,
span_index,
(i + 1, 0),
fitting_start,
word_range_width,
number_of_blanks,
);
}
visual_lines.push(current_visual_line);
current_visual_line = VisualLine::default();
number_of_blanks = 0;
if word.blank {
fit_x = line_width;
word_range_width = 0.;
fitting_start = (i, 0);
} else {
fit_x = line_width - word_width;
word_range_width = word_width;
fitting_start = (i + 1, 0);
}
and

cosmic-text/src/shape.rs

Lines 923 to 968 in aa1b37a

// Wrap::Word
let mut trailing_space_width = None;
if i > 0 {
if let Some(previous_word) = span.words.get(i - 1) {
// Current word causing a wrap is not whitespace, so we ignore the
// previous word if it's a whitespace
if previous_word.blank {
trailing_space_width =
Some(previous_word.x_advance * font_size);
number_of_blanks = number_of_blanks.saturating_sub(1);
}
}
}
if let Some(width) = trailing_space_width {
add_to_visual_line(
&mut current_visual_line,
span_index,
fitting_start,
(i - 1, 0),
word_range_width - width,
number_of_blanks,
);
} else {
add_to_visual_line(
&mut current_visual_line,
span_index,
fitting_start,
(i, 0),
word_range_width,
number_of_blanks,
);
}
visual_lines.push(current_visual_line);
current_visual_line = VisualLine::default();
number_of_blanks = 0;
if word.blank {
fit_x = line_width;
word_range_width = 0.;
fitting_start = (i + 1, 0);
} else {
fit_x = line_width - word_width;
word_range_width = word_width;
fitting_start = (i, 0);
}
}

@grovesNL grovesNL changed the title Wrap::Word inserts newline for trailing space Wrap::Word inserts newline for trailing space (after word) Jul 16, 2023
@Imberflur
Copy link
Contributor

Imberflur commented Aug 13, 2023

I've noticed that trailing spaces also affect centered and right aligned text. At least, when there are multiple trailing spaces.

edit: considering that it only seems to happen with multiple spaces, this might be intended, I'm not sure what the typical behavior is in this case...

(I see this was discussed a bit in #78, it seems like the ideal behavior is different for different cases)

Imberflur added a commit to Imberflur/cosmic-text that referenced this issue Aug 26, 2023
Try to ensure that using "the width computed during an unconstrained
layout" as the width constraint during a relayout produces the same
layout. This is useful for certain UI layout algorithms.

See pop-os#134

* Instead of computing the LayoutLine width from the positioned and
  aligned glyphs, we pass through width computed during line wrapping
  (unless justified alignment is used, in this case we use the old
  approach because the use case for measuring the width isn't really
  applicable to justified text since that will just expand to the
  provided width). For the produced width to later give the same
  wrapping results when passed in as the `line_width` it needs to use
  the same exact float arithmatic that was used to compute the width
  that is compared against `line_width` when making line wrapping
  choices. Passing this width through as the LayoutLine width is the
  most covenient option without making more major changse to the API.
  Nevertheless, I am imagining that if we get a dedicated measurement
  method (i.e. that doesn't do the final positioning and alignment of
  glyphs and which caches `Vec<VisualLine>`), then this width can just
  be exposed there instead of preservering it in LayoutLine.
* Incidentally, this fixes
  pop-os#169.
* Switch substraction from `fit_x` to checking whether potential
  addition to the current line width would exceed the `line_width`. This
  avoids the float error being dependent on the provided `line_width`
  value.
* When eliminating trailing space from the line width, we avoid
  backtracking with subtraction (which would not give the same exact
  value due to float error) and instead save the previous width and use
  that.
* If the previous word did not exceed the line_width, we now include a
  single blank word even if it would cross the width limit since its
  width won't be counted. This is necessary to get the same wrapping
  behavior when re-using the measured width (which doesn't count a
  single trailing blank word). Note, this whitespace logic may be
  reworked anyway if <pop-os#155>
  is addressed.
* Change tests to use `opt-level=1` to keep test runtime down.
* Add `fonts` folder for fonts used in tests.
* Fix an issue where a non-breaking whitespace was assumed to be the
  start of a section of spaces which included characters that weren't
  even whitespace.
* Add some TODOs about incongruencies between `is_whitespace`,
  justification, and line breaks.
dtzxporter added a commit to dtzxporter/cosmic-text that referenced this issue Feb 2, 2024
Fixes pop-os#155, and also fixes Word::Wrap when two buffer overflowing words are next to each other.
vorporeal pushed a commit to warpdotdev/cosmic-text that referenced this issue Mar 22, 2024
Fixes pop-os#155, and also fixes Word::Wrap when two buffer overflowing words are next to each other.
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 a pull request may close this issue.

2 participants