JDK-8130738 TextFlow's tab width is static

Thu Oct 17 18:51:36 UTC 2019

Hi,

Thanks for that. It also neatly solves the units problem.
I think it means we can just support the same and not go out of our way 
to make provision for pixels.

-phil.

On 10/17/19 11:46 AM, David Grieve wrote:
> FWIW, w3c spec for tab-size is https://drafts.csswg.org/css-text-3/#propdef-tab-size
> In this spec, the value of 'tab-size' is a <number> (or length -which no browsers support). For all intents and purposes, <number> indicates a number of spaces.
>
>> -----Original Message-----
>> From: openjfx-dev <openjfx-dev-bounces at openjdk.java.net> On Behalf Of
>> Phil Race
>> Sent: Thursday, October 17, 2019 2:25 PM
>> To: Scott Palmer <swpalmer at gmail.com>
>> Cc: openjfx-dev at openjdk.java.net Mailing <openjfx-dev at 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.race at 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&amp
>> ;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 <swpalmer at 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 <swpalmer at gmail.com
>> <mailto:swpalmer at 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.rushforth at oracle.com <mailto:kevin.rushforth at 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