-----Original Message-----
From: openjfx-dev <openjfx-dev-boun...@openjdk.java.net> On Behalf Of
Phil Race
Sent: Thursday, October 17, 2019 2:25 PM
To: Scott Palmer <swpal...@gmail.com>
Cc: openjfx-dev@openjdk.java.net Mailing <openjfx-dev@openjdk.java.net>
Subject: Re: JDK-8130738 TextFlow's tab width is static
Hi,
As I wrote, adding units complicates things.
We would have two linked properties in what you have below and one
modifies the interpretation of the other.
If we expose the number of spaces integer property today, I think we'd have
to add the units as a separate property, but it might be that this is what you'd
want anyway unless right now, today you add some new TabSize class which
includes both and perhaps today would support only "spaces" or "ems" or
something as the only value of the enum of sizes.
FWIW I assumed that CSS is using "ems" as an explicit name for the default if
you don't specify units.
With TabSize there you could later add "pixels".
But I am not sure it is really worth supporting pixels but I raised it because
we should have the discussion up front to know if we have a way to add it
later if it is needed.
If we aren't limiting the value, I see no reason to use short, since any value >
100 or thereabouts pretty much means " line break", or "clip" if there's no
line breaking.
However I still prefer a sensible maximum.
Kevin noted off-line that clamping isn't completely off-base for ranges.
-phil.
On 10/17/19 10:40 AM, Scott Palmer wrote:
Hi Phil,
Thanks for taking a look. I was going to get back to this soon to attempt
adding the CSS property as you mention in your previous email. I considered
tabSize as a name as well. I don’t have a preference if we leave things as
representing tabs as a number of spaces. But it wouldn’t be too difficult to
support units, making it mostly compatible with CSS rules. The way I envision
that is having two properties, tabSize and tabUnit.
David mentioned javafx.css.SizeUnits… I looked quickly at the java
docs for it, and it’s basically undocumented . So I went to
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
.
w3.org%2FTR%2Fcss-values-
3%2F%23lengths&data=02%7C01%7CDavid.Griev
e%40microsoft.com%7Cec03031ecb6c484da76008d7533002ce%7C72f988bf8
6f141a
f91ab2d7cd011db47%7C1%7C0%7C637069337978947508&sdata=li0kW
L1C1hNRA
In0C5ehMIHz6WD%2B%2F3F1xgRH%2Fl9Piv4%3D&reserved=0 and I
see there
is no CSS unit like ‘ems’ but meaning the width of a space in the
current font. The problem with that is it would leave out the
possibility to set the tab width to anything relative to the current
implementation of 8 spaces. (In hindsight it should have been
implemented based on ‘ems’, which for a fixed width font as typically
used in a code editor would be the same as 8 spaces anyway) Do we add
something to SizeUnits so we can work around this? e.g. ‘fxsp’
(FX-space - fx prefix to avoid a potential collision with any future
official CSS units)
// Two tab-related properties
public void setTabSize(int size); // defaults to 8 public void
setTabUnits(SizeUnits units); // ?? no unit for width of space in
current font
If we did the above, I would consider adding this for convenience:
public void setTabWidth(int size, TabUnits units);
As far as setting a range, I’m not sure there is any worthwhile benefit to
enforcing a maximum. If we want to store the value in a short instead of an
int to potentially save a couple bytes, sure. Otherwise, if someone wants to
set tabs to be 20 spaces or 100, why should we prevent it? If there isn't a
performance or memory impact, I wouldn’t enforce a maximum.
Ignoring any out of range values rather than clamping seems fine to me as
well. I was thinking of what happens if the value was bound to another value
that strayed out of range. Clamping would get you as close as possible to the
bound value, rather than stuck at the previously observed value. I just
guessed that would be preferred, but if ignoring is more consistent with
other values, then I agree it makes sense. As long as the behaviour is
documented, I can’t think of any significant downside either way.
Regards,
Scott
On Oct 17, 2019, at 12:45 PM, Phil Race <philip.r...@oracle.com> wrote:
Hi,
Some more points which I thought about but for whatever reason did not
pen ..
First one minor thing: tabWidth is an OK name, but it does not
conjure up that the value you specify isn't a width in pixels but the
number of discrete space characters to use. FWIW CSS calls it tab-size.
But CSS then supports pixels and ems .. that complicates matters a lot.
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev
eloper.mozilla.org%2Fen-US%2Fdocs%2FWeb%2FCSS%2Ftab-
size&data=02%
7C01%7CDavid.Grieve%40microsoft.com%7Cec03031ecb6c484da76008d7533
002c
e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6370693379789475
08&
;sdata=UGYGSC%2F1S85T1XPBcsaUhHp1s4gmDYOKrcjQxaPdi0M%3D&r
eserved=
0
Thee rest is mostly to do with "legal or sensible values"
You have :
if (spaces < 0)
spaces = 0;
since negative values are not something we want to support!
But I think instead of clamping you could "ignore", any out of range value.
This is practiced elsewhere in FX where illegal values are ignored
rather than throwing an exception, although it might be that clamping
is quite common in range cases.
The follow on to that is that what is a sensible maximum value.
Currently we have int which is more than we need. For that matter so is
short.
I think we should have a documented and enforced sensible maximum.
Perhaps it could be "8" meaning we only support reducing tab width as
I don't know if anyone really wants to increase it.
CSS doesn't have a limit that I can see but if we are already only
supporting it described as number of spaces, we can have this other
limitation too.
If you think 8 is too small, then maybe 16 ?
Also remember we can compatibly relax it later but we can't compatibly
tighten it.
-phil.
On 10/15/19 12:17 PM, Phil Race wrote:
Hi,
I've looked over this and I don't see any issues - meaning gotchas.
It just provides a way to over-ride the hard-coded 8, whether using
a Text node directly or a TextFlow.
Two observations of what one might call limitations
1) This is a rendering time property, which is controlled by the program.
A document containing tabs saved and re-rendered somewhere else
won't be helped.
2) This just provides API on the scene graph node types, not any of
the UI controls which use Text nodes. Something like a CSS property
may be the way to go if you wanted that.
Text has a nested class StyleableProperties for CSS properties with
which it plays nice : font, underline, strikethrough, text-alignment
So creating an fx-tabWidth (or similar name) CSS property could be
propagated through to there in a similar way.
-phil.
On 9/30/19 9:48 AM, Scott Palmer wrote:
Okay, current work relocated to a clone of the new official Git repo.
My initial implementation is here:
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg
ithub.com%2Fswpalmer%2Fjfx%2Fcommit%2Fcc6193451bf8a693093f3ded5d
cbe
47af2fcbe8f&data=02%7C01%7CDavid.Grieve%40microsoft.com%7Cec0
30
31ecb6c484da76008d7533002ce%7C72f988bf86f141af91ab2d7cd011db47%7
C1%
7C0%7C637069337978947508&sdata=LcefWjPpbrsy5tOSVZx%2FL56saH
3L%2
FeeqgHp%2Ftbg4gFY%3D&reserved=0
<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2F
github.com%2Fswpalmer%2Fjfx%2Fcommit%2Fcc6193451bf8a693093f3ded5
dcb
e47af2fcbe8f&data=02%7C01%7CDavid.Grieve%40microsoft.com%7Cec
03
031ecb6c484da76008d7533002ce%7C72f988bf86f141af91ab2d7cd011db47%
7C1
%7C0%7C637069337978947508&sdata=LcefWjPpbrsy5tOSVZx%2FL56sa
H3L%
2FeeqgHp%2Ftbg4gFY%3D&reserved=0>
I would submit it as a pull request but that seems premature since
there hasn’t been any real discussion of challenges I’ve overlooked. All I
have are the famous last words, “It works for me.”
I saw in the archives a concern about tab width vs tab stops. For some
reason I didn’t get the email. Anyway, I don’t think arbitrary tab stop
positions, at different intervals that is, are what was asked for. That
certainly
would require a significantly different implementation.
Would love to keep some momentum going on this before I become
busy with other things and won’t have time for it.
Scott
On Sep 23, 2019, at 3:57 PM, Scott Palmer <swpal...@gmail.com>
wrote:
My current work is here
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2F
github.com%2Fjavafxports%2Fopenjdk-
jfx%2Fcompare%2Fdevelop...swpal
mer%3Ajdk-
8130738%3Fexpand%3D1&data=02%7C01%7CDavid.Grieve%40m
icrosoft.com%7Cec03031ecb6c484da76008d7533002ce%7C72f988bf86f141af
91ab2d7cd011db47%7C1%7C0%7C637069337978947508&sdata=BbNzTf
l7Ai
J16Lg3GD%2FpflnxJ5EllT93W0CNv62gdWw%3D&reserved=0
<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2
Fgithub.com%2Fjavafxports%2Fopenjdk-
jfx%2Fcompare%2Fdevelop...swpa
lmer%3Ajdk-
8130738%3Fexpand%3D1&data=02%7C01%7CDavid.Grieve%40
microsoft.com%7Cec03031ecb6c484da76008d7533002ce%7C72f988bf86f141
a
f91ab2d7cd011db47%7C1%7C0%7C637069337978947508&sdata=BbNzT
fl7A
iJ16Lg3GD%2FpflnxJ5EllT93W0CNv62gdWw%3D&reserved=0>
While writing a unit test I realized that the StubToolkit isn’t really
running the Prism layout code, so I’m not sure how that gets tested. I made
it work with StubTextLayout for now.
Regards,
Scott
On Sep 20, 2019, at 12:57 PM, Scott Palmer <swpal...@gmail.com
<mailto:swpal...@gmail.com>> wrote:
Thanks Kevin.
My current implementation appears to be working for TextFlow and
Text, with the TextFlow overriding the tabWidth of the child Text nodes. This
seems to work out naturally from the way TextFlow overrides the TextLayout
instance used by the Text node.
If there are tricky corner-cases that I’m missing, I guess figuring out
all the cases it will need to handle is part of this discussion. It didn’t
seem to
be that challenging so far, so perhaps I am missing something (hopefully
not). I wrote a small test app to visually see that things were working as I
expected. I have not yet written the unit tests.
Cheers,
Scott
On Sep 20, 2019, at 10:58 AM, Kevin Rushforth
<kevin.rushfo...@oracle.com <mailto:kevin.rushfo...@oracle.com>> wrote:
Hi Scott,
I'm sure Phil will have more comments on this. While the API seems
simple enough on the surface, I suspect that this will be a challenging feature
to implement correctly for all of the cases it will need to handle. It would
need careful review and testing as well. My only comment on the API itself is
that if we do accept this feature, it should probably go on both Text and
TextFlow, and be one of the attributes of Text that is ignored / overridden
when a Text node is in a TextFlow.
-- Kevin
On 9/18/2019 6:14 PM, Scott Palmer wrote:
I would like to implement this feature, being able to adjust
the tab size in a TextFlow or Text node (JDK-8130738
<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2
F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-
8130738&data=02%7C
01%7CDavid.Grieve%40microsoft.com%7Cec03031ecb6c484da76008d7533
002ce%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637069337978
947508&sdata=bD3fdm4cbXVppm7F7hced9HDRt8WXtDMiZ9%2FOyTl3o
8%
3D&reserved=0
<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2
F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-
8130738&data=02%7C
01%7CDavid.Grieve%40microsoft.com%7Cec03031ecb6c484da76008d7533
002ce%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637069337978
947508&sdata=bD3fdm4cbXVppm7F7hced9HDRt8WXtDMiZ9%2FOyTl3o
8%
3D&reserved=0>>). It involves new public API, so I want to
start a discussion about it here. (My motivation is that
RichTextFX suggests an entirely unacceptable workaround of
substituting actual spaces when the tab character is typed and
cites the lack of this API.)
I’ve already jumped the gun and taken a crack at an
implementation. It is currently incomplete as I was just poking around to see
if it was going to be easy enough to not take up too much of my time. It boils
down to:
TextFlow and Text get a new property for tab width, an integer
representing the number of spaces taken by a tab. (The value is
only used to initialize the tab width for the TextLayout when
needed.) TextLayout interface gets a new method: boolean
setTabWidth(int spaces) TextLayout gets a new constant:
DEFAULT_TAB_WIDTH = 8; PrismTextLayout implements the new
setTabWidth API.
I’m not sure that the Text node needs this new property. I figured
it would be rarely used on that class, so I had implemented it via an added
property in the private TextAttributes class. Maybe it isn’t needed at all.
What’s the next step?
Regards,
Scott
