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

8242548: Wrapped labeled controls using -fx-line-spacing cut text off #173

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

@@ -417,17 +417,27 @@ public static String computeClippedText(Font font, String text, double width,
}

public static String computeClippedWrappedText(Font font, String text, double width,
double height, OverrunStyle truncationStyle,
double height, double lineSpacing, OverrunStyle truncationStyle,
String ellipsisString, TextBoundsType boundsType) {
if (font == null) {
throw new IllegalArgumentException("Must specify a font");
}

// The height given does not need to include the line spacing after
// the last line to be able to render that last line correctly.
//
// However the calculations include the line spacing as part of a
// line's height. In order to not cut off the last line because its
// line spacing wouldn't fit, the height used for the calculation
// is increased here with the line spacing amount.

height += lineSpacing;
This conversation was marked as resolved by hjohn

This comment has been minimized.

@aghaisas

aghaisas Apr 22, 2020
Collaborator

Modifying 'height' parameter seems incorrect as this parameter is used in other calculations below. If needed, use a separate local variable to use 'height+lineSpacing'

This comment has been minimized.

@hjohn

hjohn Apr 22, 2020
Author Collaborator

The height used needs to be changed everywhere. However, I can move this to the caller instead, with a comment explaining why:

The computeClippedWrappedText call works with full lines (height of line + line spacing), including the last line. However the wrap height does not include the line spacing for the last line. In order to avoid wrapping the last line of text even though there is sufficient space the wrap height passed to computeClippedWrappedText is increased with the line spacing so it computes the correct text.

This comment has been minimized.

@aghaisas

aghaisas Apr 23, 2020
Collaborator

I see your point. I am OK with the changes that you have done.


This conversation was marked as resolved by hjohn

This comment has been minimized.

@aghaisas

aghaisas Apr 22, 2020
Collaborator

This method makes a call to -
double eHeight = computeTextHeight(font, ellipsis, 0, boundsType);

What is the behavior if we make a call to the other method that takes in lineSpacing?
double eHeight = computeTextHeight(font, ellipsis, 0, lineSpacing, boundsType);

This comment has been minimized.

@hjohn

hjohn Apr 22, 2020
Author Collaborator

The call only determines the ellipsis height (eHeight) to see if the space is so small that even an ellipsis wouldn't fit. For a multi-line ellipsis (if there is such a thing) we could pass in the line spacing here.

String ellipsis = (truncationStyle == CLIP) ? "" : ellipsisString;
int eLen = ellipsis.length();
// Do this before using helper, as it's not reentrant.
double eWidth = computeTextWidth(font, ellipsis, 0);
double eHeight = computeTextHeight(font, ellipsis, 0, boundsType);
double eHeight = computeTextHeight(font, ellipsis, 0, lineSpacing, boundsType);

if (width < eWidth || height < eHeight) {
// The ellipsis doesn't fit.
@@ -438,7 +448,7 @@ public static String computeClippedWrappedText(Font font, String text, double wi
helper.setFont(font);
helper.setWrappingWidth((int)Math.ceil(width));
helper.setBoundsType(boundsType);
helper.setLineSpacing(0);
helper.setLineSpacing(lineSpacing);

boolean leading = (truncationStyle == LEADING_ELLIPSIS ||
truncationStyle == LEADING_WORD_ELLIPSIS);
@@ -1107,7 +1107,7 @@ private void updateDisplayedText(double w, double h) {
String ellipsisString = labeled.getEllipsisString();

if (labeled.isWrapText()) {
result = Utils.computeClippedWrappedText(font, s, wrapWidth, wrapHeight, truncationStyle, ellipsisString, text.getBoundsType());
result = Utils.computeClippedWrappedText(font, s, wrapWidth, wrapHeight, labeled.getLineSpacing(), truncationStyle, ellipsisString, text.getBoundsType());
} else if (multiline) {
StringBuilder sb = new StringBuilder();

@@ -136,6 +136,12 @@
assertTrue(skin.propertyChanged);
}

@Test public void lineSpacingChangesOnLabelShouldInvoke_handleControlPropertyChanged() {
skin.addWatchedProperty(label.lineSpacingProperty());
label.setLineSpacing(1.0);
assertTrue(skin.propertyChanged);
}

@Test public void textChangesOnLabelShouldInvoke_handleControlPropertyChanged() {
skin.addWatchedProperty(label.textProperty());
label.setText("Bust my buffers!");
@@ -279,6 +285,15 @@
assertFalse(skin.get_invalidText());
}

@Test public void lineSpacingChangesOnLabelShouldInvalidateLayoutAndDisplayText() {
label.layout();
skin.updateDisplayedText();

label.setLineSpacing(1.0);
assertTrue(label.isNeedsLayout());
assertTrue(skin.get_invalidText());
}

@Test public void textChangesOnLabelShouldInvalidateLayoutAndDisplayTextAndTextWidth() {
label.layout();
skin.updateDisplayedText();
@@ -1145,6 +1160,26 @@
assertTrue(height >= singleLineHeight * 5);
}

@Test public void whenTextHasNewlinesAndPositiveLineSpacing_computePrefHeight_IncludesTheMultipleLinesAndLineSpacingInThePrefHeight() {
label.setLineSpacing(2);
label.setText("This\nis a test\nof the emergency\nbroadcast system.\nThis is only a test");
label.setPadding(new Insets(0, 0, 0, 0));
final double singleLineHeight = Utils.computeTextHeight(label.getFont(), " ", 0, text.getBoundsType());
final double expectedHeight = singleLineHeight * 5 + label.getLineSpacing() * 5 - label.getLineSpacing();
final double height = label.prefHeight(-1);
assertEquals(expectedHeight, height, 0);
}

@Test public void whenTextHasNewlinesAndNegativeLineSpacing_computePrefHeight_IncludesTheMultipleLinesAndLineSpacingInThePrefHeight() {
label.setLineSpacing(-2);
label.setText("This\nis a test\nof the emergency\nbroadcast system.\nThis is only a test");
label.setPadding(new Insets(0, 0, 0, 0));
final double singleLineHeight = Utils.computeTextHeight(label.getFont(), " ", 0, text.getBoundsType());
final double expectedHeight = singleLineHeight * 5 + label.getLineSpacing() * 5 - label.getLineSpacing();
final double height = label.prefHeight(-1);
assertEquals(expectedHeight, height, 0);
}

@Test public void whenTextHasNewlinesAfterPreviousComputationOf_computePrefHeight_IncludesTheMultipleLinesInThePrefHeight() {
label.setText("This is a test");
final double oldPrefHeight = label.prefHeight(-1);
@@ -48,6 +48,7 @@ public boolean setContent(TextSpan[] spans) {
private Font font;
private int tabSize = DEFAULT_TAB_SIZE;
private int nullFontSize = 0;
private float spacing;

@Override
public boolean setContent(String text, Object font) {
@@ -69,6 +70,8 @@ public boolean setWrapWidth(float wrapWidth) {

@Override
public boolean setLineSpacing(float spacing) {
this.spacing = spacing;

return true;
}

@@ -92,7 +95,7 @@ public BaseBounds getBounds(TextSpan filter, BaseBounds bounds) {
final double fontSize = (font == null ? nullFontSize : ((Font)font).getSize());
final String[] lines = getText().split("\n");
double width = 0.0;
double height = fontSize * lines.length;
double height = fontSize * lines.length + spacing * (lines.length - 1);
for (String line : lines) {
final int length;
if (line.contains("\t")) {
ProTip! Use n and p to navigate between commits in a pull request.