On 10/17/19 12:32 PM, Kevin Rushforth wrote:
It seems that you have a path forward then. So, there are a couple
questions to answer:
1. Should the type of the property be int or double? If it really is
only ever going to be a number of spaces, then int seems fine. That,
along with the name of the property, would underscore the fact that
no, this isn't a size where units make sense; it's a number of spaces.
A discrete number of spaces, so int.
2. Should the range be [1,MAXINT] or [1,16] or [1,something-else]?
Once that is decided, I think clamp on use would be the most
consistent with other similar properties, but that's worth checking.
If we go with "MAXINT" (which is not my preference), I think there'd
need to be some testing of
the various code paths and pipelines to be sure that various values from
large, through extreme,
all do something sensible, and of course, no exceptions or crashes.
Various code paths means Text, TextFlow, linewrapping or not ...
complex text and simple text too.
Perhaps complex text should be tested anyway, although I've been
assuming that we are handling
tab spacing outside of that but didn't verify it.
-phil.
-- Kevin
On 10/17/2019 12:11 PM, Scott Palmer wrote:
You’re right. Something I was reading indicated that while there was
no default unit, ‘px’ was implicitly appended. I can’t find that
now. Go figure.
Everything I find now about CSS tab-size is in agreement with what
David wrote. It’s a number of spaces. With units it would be a
‘length’ but nothing supports that.
So I think it makes sense to add -fx-tab-size as a CSS property, only
supporting a number with no units.
Scott
On Oct 17, 2019, at 2:32 PM, Phil Race <philip.r...@oracle.com> wrote:
Really ? It defaults to pixels ? Is that just inherited as being the
default CSS unit ?
Is that FX's implementation
Hmm. A bit of reading about web CSS says that strictly anything
without an explicit unit should be ignored.
The only exception is zero, where it doesn't matter.
Eg see :
https://stackoverflow.com/questions/7907749/default-unit-for-font-size
Although I am sure there are more authoratative sources than that.
-phil.
On 10/17/19 11:22 AM, Scott Palmer wrote:
So do we go ahead and implement tabSize without the CSS support?
I’m not sure the CSS property should be added before unit issues
are worked out, as I think the units would be expected in the CSS
context. E.g. CSS implicitly adds ‘px’ if no unit is specified. If
our tabSize units are spaces, that breaks.
Scott
On Oct 17, 2019, at 2:16 PM, Kevin Rushforth
<kevin.rushfo...@oracle.com> wrote:
It might make sense to just add the tabSize property now, and
later consider adding a tabUnits property in the future if needed.
By default, having the units be "number of spaces in the current
font" is what makes the most sense, so before we could add
tabUnits we would need to extend it as you suggest. I'm not sure
it's needed, though, so that would be another reason not to do it
now.
It's probably best to have the type of tabSize be double rather
than int. We do this for most attributes to leave the door open
for fractional values. I don't know why anyone would want a tab
that was 5.7 spaces, but if we ever were to add a tabUnits
property, I could easily see wanting fractional values for some
units.
-- Kevin
On 10/17/2019 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://www.w3.org/TR/css-values-3/#lengths 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://developer.mozilla.org/en-US/docs/Web/CSS/tab-size
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://github.com/swpalmer/jfx/commit/cc6193451bf8a693093f3ded5dcbe47af2fcbe8f
<https://github.com/swpalmer/jfx/commit/cc6193451bf8a693093f3ded5dcbe47af2fcbe8f>
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://github.com/javafxports/openjdk-jfx/compare/develop...swpalmer:jdk-8130738?expand=1
<https://github.com/javafxports/openjdk-jfx/compare/develop...swpalmer:jdk-8130738?expand=1>
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://bugs.openjdk.java.net/browse/JDK-8130738
<https://bugs.openjdk.java.net/browse/JDK-8130738>>). 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
