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> 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> 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> 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> > >>>>>>> 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 > > > >
