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
