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 >
