JDK-8130738 TextFlow's tab width is static

[Phil Race]

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.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://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 <swpalmer at 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 <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://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
>