I finally had a moment to get back at this.
I've changed the name of the property from tabWidth to tabSize and
implemented the CSS property '-fx-tab-size'. The CSS documentation
will need to be updated. I didn't see where the CSS document is
located in the source tree.
While adding CSS support I noticed that in both Text.java and
TextFlow.java the indentation is wrong for the StyleableProperties
nested class (has one extra space). I wasn't sure how much I should
change in the scope of this feature. If I don't fix some indenting my
changes will look funny beside the lines of existing code. If I fix
all of the broken indenting I'll change a lot of lines that have
nothing to do with this feature. If I just properly indent the lines I
change or add, the indenting will be more chaotic than it is currently.
I have noticed a rendering anomaly with a tab size of zero (the value
I was initially clamping to) and rather than dealing with that I took
the easy way out and changed it to agree with the minimum of 1 that
Kevin indicated for the range.
I have not limited the maximum tab size. I know Phil has expressed
that MAXINT is not his preference, but I can't think of a good
criteria for what the maximum should be. I can imagine someone using
a tab separator to align columns of text that have much more than 16
characters for example. I did a quick test with the tab size set to
Integer.MAX_VALUE-1 and nothing blew up. Of course the text after the
tab was nowhere to be seen.
Regards,
Scott
On Thu, Oct 17, 2019 at 3:42 PM Phil Race <philip.r...@oracle.com
<mailto:philip.r...@oracle.com>> wrote:
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
<mailto: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
<mailto: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 <mailto: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 <mailto: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>
<mailto: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>
>>>>>>>>>>>> <mailto: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
>
