On 10/17/19 11:16 AM, Kevin Rushforth 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.
In a fixed width font that would seem bizarre.
And I think fixed width fonts are the main case when you *want* tabs.
This is another reason why I don't think we should do pixels ...
-phil.
-- 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
