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

8130738: Add tabSize property to Text and TextFlow #32

Closed
wants to merge 7 commits into from
@@ -3028,6 +3028,12 @@ <h4><a id="text">Text</a></h4>
<td>&nbsp;</td>
</tr>
<tr>
<th class="propertyname" scope="row">-fx-tab-size</th>
<td class="value"><a href="#typenumber" class="typelink">&lt;integer&gt;</a></td>
<td>8</td>
<td>&nbsp;</td>
</tr>
<tr>
<th class="propertyname" scope="row">-fx-text-alignment</th>
<td class="value">[ left | center | right | justify ] </td>
<td>left</td>
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2012, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012, 2019, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -75,6 +75,8 @@
public static final int TYPE_TOP = 1 << 4;
public static final int TYPE_BEARINGS = 1 << 5;

public static final int DEFAULT_TAB_SIZE = 8;

public static class Hit {
int charIndex;
int insertionIndex;
@@ -188,6 +190,15 @@ public Hit(int charIndex, int insertionIndex, boolean leading) {
*/
public Shape getShape(int type, TextSpan filter);

/**
* Sets the tab size for the TextLayout.
*
* @param spaces the number of spaces represented by a tab. Default is 8.
* Minimum is 1, lower values will be clamped to 1.
* @return returns true if the call modifies the layout internal state.
*/
public boolean setTabSize(int spaces);

public Hit getHitInfo(float x, float y);

public PathElement[] getCaretShape(int offset, boolean isLeading,
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2012, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012, 2019, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -80,6 +80,7 @@
private LayoutCache layoutCache;
private Shape shape;
private int flags;
private int tabSize = DEFAULT_TAB_SIZE;

public PrismTextLayout() {
logicalBounds = new RectBounds();
@@ -648,6 +649,19 @@ public Shape getShape(int type, TextSpan filter) {
return outline;
}

@Override
public boolean setTabSize(int spaces) {
if (spaces < 1) {
spaces = 1;
This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Nov 27, 2019

Collaborator

Please surround the statement with curly braces (our coding style is to always surround the target of a conditional in curly braces unless it is on the same line).

}
if (tabSize != spaces) {
tabSize = spaces;
relayout();
return true;
}
return false;
}

/***************************************************************************
* *
* Text Layout Implementation *
@@ -1004,7 +1018,7 @@ private float getTabAdvance() {
} else {
spaceAdvance = strike.getCharAdvance(' ');
}
return 8 * spaceAdvance;
return tabSize * spaceAdvance;
}

private void layout() {
@@ -82,6 +82,7 @@
import javafx.css.Styleable;
import javafx.css.StyleableBooleanProperty;
import javafx.css.StyleableDoubleProperty;
import javafx.css.StyleableIntegerProperty;
import javafx.css.StyleableObjectProperty;
import javafx.css.StyleableProperty;
import javafx.geometry.BoundingBox;
@@ -307,6 +308,7 @@ private TextLayout getTextLayout() {
} else {
layout.setDirection(TextLayout.DIRECTION_LTR);
}
layout.setTabSize(getTabSize());
}
return layout;
}
@@ -1267,6 +1269,30 @@ private boolean doComputeContains(double localX, double localY) {
return TransformedShape.translatedShape(shape, x, y);
}

/**
* The size of a tab stop in spaces.
This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

Copy link
@nlisker

nlisker Dec 12, 2019

Contributor

"tab stop" seems to be an uncommon term. Better are "horizontal tab" (used in the JLS), "tab character" (used in Pattern, "horizontal tabulation" or the like.

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Dec 12, 2019

Collaborator

The terms "tab character" or "horizontal tab" refer to the ASCII tab character itself. Since a tab character isn't a fixed number of spaces, changing it to "size of a tab character" could be misleading. I'd be fine with another alternative, though, if someone could come up with a better one.

This comment has been minimized.

Copy link
@prrace

prrace Dec 13, 2019

Contributor

We are referring to the character here aren't we ? ie the actual character and the rest of it is about how it renders.
Paraphrasing the java doc it says :
If you display it will display as N instances of

This comment has been minimized.

Copy link
@swpalmer

swpalmer Dec 13, 2019

Author

I find "size of the tab character" to be a bit confusing. The number of space you advance is not a constant for one, so tab characters don't really have a fixed size in that sense. The javadoc could be rephrased as "the distance between tab stops". However, that keeps the unfamiliar, but correct IMO, "tab stop" wording. For the record the top google hit for "tab stop" results in: "A tab stop is a term used to describe the location the cursor stops after the Tab key is pressed." Wikipedia says something similar and adds, "In text editors on a computer, the same concept is implemented simplistically with automatic, fixed tab stops."

This comment has been minimized.

Copy link
@prrace

prrace Dec 13, 2019

Contributor

OK. Lets not do this to death. I knew what you meant by tab stop.

* Values less than 1 are treated as 1.
*
* @defaultValue 8
*
* @since 14
*/
public final IntegerProperty tabSizeProperty() {
return getTextAttribute().tabSizeProperty();
}

public final int getTabSize() {
if (attributes == null || attributes.tabSize == null) {
return TextLayout.DEFAULT_TAB_SIZE;
}
return getTextAttribute().getTabSize();
}

public final void setTabSize(int spaces) {
tabSizeProperty().set(spaces);
}


/***************************************************************************
* *
* Stylesheet Handling *
@@ -1276,9 +1302,9 @@ private boolean doComputeContains(double localX, double localY) {
/*
* Super-lazy instantiation pattern from Bill Pugh.
*/
private static class StyleableProperties {
private static class StyleableProperties {

private static final CssMetaData<Text,Font> FONT =
private static final CssMetaData<Text,Font> FONT =
new FontCssMetaData<Text>("-fx-font", Font.getDefault()) {

@Override
@@ -1290,11 +1316,11 @@ public boolean isSettable(Text node) {
public StyleableProperty<Font> getStyleableProperty(Text node) {
return (StyleableProperty<Font>)node.fontProperty();
}
};
};

private static final CssMetaData<Text,Boolean> UNDERLINE =
private static final CssMetaData<Text,Boolean> UNDERLINE =
new CssMetaData<Text,Boolean>("-fx-underline",
BooleanConverter.getInstance(), Boolean.FALSE) {
BooleanConverter.getInstance(), Boolean.FALSE) {

@Override
public boolean isSettable(Text node) {
@@ -1307,11 +1333,11 @@ public boolean isSettable(Text node) {
public StyleableProperty<Boolean> getStyleableProperty(Text node) {
return (StyleableProperty<Boolean>)node.underlineProperty();
}
};
};

private static final CssMetaData<Text,Boolean> STRIKETHROUGH =
private static final CssMetaData<Text,Boolean> STRIKETHROUGH =
new CssMetaData<Text,Boolean>("-fx-strikethrough",
BooleanConverter.getInstance(), Boolean.FALSE) {
BooleanConverter.getInstance(), Boolean.FALSE) {

@Override
public boolean isSettable(Text node) {
@@ -1324,13 +1350,13 @@ public boolean isSettable(Text node) {
public StyleableProperty<Boolean> getStyleableProperty(Text node) {
return (StyleableProperty<Boolean>)node.strikethroughProperty();
}
};
};

private static final
CssMetaData<Text,TextAlignment> TEXT_ALIGNMENT =
new CssMetaData<Text,TextAlignment>("-fx-text-alignment",
new EnumConverter<TextAlignment>(TextAlignment.class),
TextAlignment.LEFT) {
private static final
CssMetaData<Text,TextAlignment> TEXT_ALIGNMENT =
new CssMetaData<Text,TextAlignment>("-fx-text-alignment",
new EnumConverter<TextAlignment>(TextAlignment.class),
TextAlignment.LEFT) {

@Override
public boolean isSettable(Text node) {
@@ -1343,12 +1369,12 @@ public boolean isSettable(Text node) {
public StyleableProperty<TextAlignment> getStyleableProperty(Text node) {
return (StyleableProperty<TextAlignment>)node.textAlignmentProperty();
}
};
};

private static final CssMetaData<Text,VPos> TEXT_ORIGIN =
new CssMetaData<Text,VPos>("-fx-text-origin",
new EnumConverter<VPos>(VPos.class),
VPos.BASELINE) {
private static final CssMetaData<Text,VPos> TEXT_ORIGIN =
new CssMetaData<Text,VPos>("-fx-text-origin",
new EnumConverter<VPos>(VPos.class),
VPos.BASELINE) {

@Override
public boolean isSettable(Text node) {
@@ -1361,14 +1387,14 @@ public boolean isSettable(Text node) {
public StyleableProperty<VPos> getStyleableProperty(Text node) {
return (StyleableProperty<VPos>)node.textOriginProperty();
}
};
};

private static final CssMetaData<Text,FontSmoothingType>
FONT_SMOOTHING_TYPE =
new CssMetaData<Text,FontSmoothingType>(
"-fx-font-smoothing-type",
new EnumConverter<FontSmoothingType>(FontSmoothingType.class),
FontSmoothingType.GRAY) {
private static final CssMetaData<Text,FontSmoothingType>
FONT_SMOOTHING_TYPE =
new CssMetaData<Text,FontSmoothingType>(
"-fx-font-smoothing-type",
new EnumConverter<FontSmoothingType>(FontSmoothingType.class),
FontSmoothingType.GRAY) {

@Override
public boolean isSettable(Text node) {
@@ -1382,12 +1408,12 @@ public boolean isSettable(Text node) {

return (StyleableProperty<FontSmoothingType>)node.fontSmoothingTypeProperty();
}
};
};

private static final
CssMetaData<Text,Number> LINE_SPACING =
new CssMetaData<Text,Number>("-fx-line-spacing",
SizeConverter.getInstance(), 0) {
private static final
CssMetaData<Text,Number> LINE_SPACING =
new CssMetaData<Text,Number>("-fx-line-spacing",
SizeConverter.getInstance(), 0) {

@Override
public boolean isSettable(Text node) {
@@ -1400,14 +1426,14 @@ public boolean isSettable(Text node) {
public StyleableProperty<Number> getStyleableProperty(Text node) {
return (StyleableProperty<Number>)node.lineSpacingProperty();
}
};
};

private static final CssMetaData<Text, TextBoundsType>
BOUNDS_TYPE =
new CssMetaData<Text,TextBoundsType>(
"-fx-bounds-type",
new EnumConverter<TextBoundsType>(TextBoundsType.class),
DEFAULT_BOUNDS_TYPE) {
private static final CssMetaData<Text, TextBoundsType>
BOUNDS_TYPE =
new CssMetaData<Text,TextBoundsType>(
"-fx-bounds-type",
new EnumConverter<TextBoundsType>(TextBoundsType.class),
DEFAULT_BOUNDS_TYPE) {

@Override
public boolean isSettable(Text node) {
@@ -1418,10 +1444,27 @@ public boolean isSettable(Text node) {
public StyleableProperty<TextBoundsType> getStyleableProperty(Text node) {
return (StyleableProperty<TextBoundsType>)node.boundsTypeProperty();
}
};
};

private static final CssMetaData<Text, Number> TAB_SIZE =
new CssMetaData<Text,Number>("-fx-tab-size",
SizeConverter.getInstance(), TextLayout.DEFAULT_TAB_SIZE) {

@Override
public boolean isSettable(Text node) {
return node.attributes == null ||
node.attributes.tabSize == null ||
!node.attributes.tabSize.isBound();
}

@Override
public StyleableProperty<Number> getStyleableProperty(Text node) {
return (StyleableProperty<Number>)node.tabSizeProperty();
}
};

private final static List<CssMetaData<? extends Styleable, ?>> STYLEABLES;
static {
private final static List<CssMetaData<? extends Styleable, ?>> STYLEABLES;
static {
final List<CssMetaData<? extends Styleable, ?>> styleables =
new ArrayList<CssMetaData<? extends Styleable, ?>>(Shape.getClassCssMetaData());
styleables.add(FONT);
@@ -1432,8 +1475,9 @@ public boolean isSettable(Text node) {
styleables.add(FONT_SMOOTHING_TYPE);
styleables.add(LINE_SPACING);
styleables.add(BOUNDS_TYPE);
styleables.add(TAB_SIZE);
STYLEABLES = Collections.unmodifiableList(styleables);
}
}
}

/**
@@ -1821,6 +1865,37 @@ final BooleanProperty caretBiasProperty() {
}
return caretBias;
}

private IntegerProperty tabSize;

final int getTabSize() {
return tabSize == null ? TextLayout.DEFAULT_TAB_SIZE : tabSize.get();
}

final IntegerProperty tabSizeProperty() {
if (tabSize == null) {
tabSize = new StyleableIntegerProperty(TextLayout.DEFAULT_TAB_SIZE) {
@Override public Object getBean() { return Text.this; }
@Override public String getName() { return "tabSize"; }
@Override public CssMetaData getCssMetaData() {
return StyleableProperties.TAB_SIZE;
}
This conversation was marked as resolved by kevinrushforth
Comment on lines +1877 to +1882

This comment has been minimized.

Copy link
@nlisker

nlisker Dec 12, 2019

Contributor

The SimpleStyleableIntegerProperty subclass can be used instead of StyleableIntegerProperty to make the code cleaner:

new SimpleStyleableIntegerProperty(StyleableProperties.TAB_SIZE, Text.this, "tabSize", TextLayout.DEFAULT_TAB_SIZE);

(This can be done for many places in the library)

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Dec 12, 2019

Collaborator

I'd probably leave it as is for this PR, since it matches other attributes in this file.

@Override protected void invalidated() {
This conversation was marked as resolved by kevinrushforth

This comment has been minimized.

Copy link
@nlisker

nlisker Dec 12, 2019

Contributor

I think that annotations tend to go on a line above what they annotate,

This comment has been minimized.

Copy link
@swpalmer

swpalmer Dec 13, 2019

Author

I'm following the pattern established elsewhere in the file for other property overrides. (underline, strikethrough, textAlignment, selectionEnd, etc.). I would keep this for consistency and compactness, unless there are more votes against it.

if (!isSpan()) {
TextLayout layout = getTextLayout();
if (layout.setTabSize(get())) {
needsTextLayout();
}
NodeHelper.markDirty(Text.this, DirtyBits.TEXT_ATTRS);
if (getBoundsType() == TextBoundsType.VISUAL) {
NodeHelper.geomChanged(Text.this);
}
}
}
};
}
return tabSize;
}
}

/**
@@ -1853,6 +1928,11 @@ public String toString() {
sb.append(", wrappingWidth=").append(wrap);
}

int tab = getTabSize();
if (tab != TextLayout.DEFAULT_TAB_SIZE) {
sb.append(", tabSize=").append(tab);
}

sb.append(", font=").append(getFont());
sb.append(", fontSmoothingType=").append(getFontSmoothingType());

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.