Re: RFR: 8276179 removing unused font code - isInstalledFont

[Phil Race]

On Fri, 29 Oct 2021 15:10:35 GMT, Florian Kirmaier  
wrote:

> removing dead code.
> When looking into the font code, I've noticed that this code is no longer 
> used, so I thought I should make a PR with a minor cleanup.

Marked as reviewed by prr (Reviewer).

I'm quite confident the build will be a good test of whether it is unused so 
fine by me.
I am a little curious as to when it stopped being used and why .. but that 
isn't a blocker.

-

PR: https://git.openjdk.java.net/jfx/pull/658

Re: RFR: 8276179 removing unused font code - isInstalledFont

[Phil Race]

On Fri, 29 Oct 2021 15:10:35 GMT, Florian Kirmaier  
wrote:

> removing dead code.
> When looking into the font code, I've noticed that this code is no longer 
> used, so I thought I should make a PR with a minor cleanup.

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jfx/pull/658

Integrated: 8236689: macOS 10.15 Catalina: LCD text renders badly

[Phil Race]

On Wed, 13 Oct 2021 23:59:40 GMT, Phil Race  wrote:

> On an external (non-retina) monitor JavaFX LCD text on macOS is painful on 
> the eyes.
> Retina diminishes it rather than cures it.
> 
> The problem is a mix of a couple of things
> 1) CoreText no longer generates LCD glyphs (except perhaps if you change some 
> system settings at your own risk)
> 2) Prism's LCD shader assumes it got LCD glyphs and makes sub-pixel 
> positioning adjustments that turn greyscale
> glyphs into multi-coloured glyphs that weren't meant to be ...
> 
> The fix here is to just disable LCD by default on macOS as is already done 
> (eg) on iOS
> This ripples through to make everything use grey scale even if you asked for 
> the LCD (which you can't have)
> It also means if you REALLY want it (and perhaps are tweaking those magical 
> settings) you can have it back
> by just specifying -Dprism.lcdtext=on
> 
> Also it means the pieces of support for this on macos are still there if 
> Apple ever bring it back (unlikely).
> Not that much code would be removed anyway .. a fair amount of it is needed 
> for Windows and Linux.

This pull request has now been integrated.

Changeset: a118d333
Author:Phil Race 
URL:   
https://git.openjdk.java.net/jfx/commit/a118d33314a363336134d31c45b50329594e5a24
Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod

8236689: macOS 10.15 Catalina: LCD text renders badly

Reviewed-by: kcr, pbansal

-

PR: https://git.openjdk.java.net/jfx/pull/642

Re: RFR: 8236689: macOS 10.15 Catalina: LCD text renders badly

[Phil Race]

On Wed, 13 Oct 2021 23:59:40 GMT, Phil Race  wrote:

> On an external (non-retina) monitor JavaFX LCD text on macOS is painful on 
> the eyes.
> Retina diminishes it rather than cures it.
> 
> The problem is a mix of a couple of things
> 1) CoreText no longer generates LCD glyphs (except perhaps if you change some 
> system settings at your own risk)
> 2) Prism's LCD shader assumes it got LCD glyphs and makes sub-pixel 
> positioning adjustments that turn greyscale
> glyphs into multi-coloured glyphs that weren't meant to be ...
> 
> The fix here is to just disable LCD by default on macOS as is already done 
> (eg) on iOS
> This ripples through to make everything use grey scale even if you asked for 
> the LCD (which you can't have)
> It also means if you REALLY want it (and perhaps are tweaking those magical 
> settings) you can have it back
> by just specifying -Dprism.lcdtext=on
> 
> Also it means the pieces of support for this on macos are still there if 
> Apple ever bring it back (unlikely).
> Not that much code would be removed anyway .. a fair amount of it is needed 
> for Windows and Linux.

Any takers for the 2nd reviewer ?
BTW another reason for making this simple and easy to revert (after lots more 
exploriing than you'd think for what ended up as a small fix) is that this 
should be backported to the LTS releases ..

-

PR: https://git.openjdk.java.net/jfx/pull/642

RFR: 8236689: macOS 10.15 Catalina: LCD text renders badly

[Phil Race]

On an external (non-retina) monitor JavaFX LCD text on macOS is painful on the 
eyes.
Retina diminishes it rather than cures it.

The problem is a mix of a couple of things
1) CoreText no longer generates LCD glyphs (except perhaps if you change some 
system settings at your own risk)
2) Prism's LCD shader assumes it got LCD glyphs and makes sub-pixel positioning 
adjustments that turn greyscale
glyphs into multi-coloured glyphs that weren't meant to be ...

The fix here is to just disable LCD by default on macOS as is already done (eg) 
on iOS
This ripples through to make everything use grey scale even if you asked for 
the LCD (which you can't have)
It also means if you REALLY want it (and perhaps are tweaking those magical 
settings) you can have it back
by just specifying -Dprism.lcdtext=on

Also it means the pieces of support for this on macos are still there if Apple 
ever bring it back (unlikely).
Not that much code would be removed anyway .. a fair amount of it is needed for 
Windows and Linux.

-

Commit messages:
 - 8236689: macOS 10.15 Catalina: LCD text renders badly

Changes: https://git.openjdk.java.net/jfx/pull/642/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=642=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8236689
  Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jfx/pull/642.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/642/head:pull/642

PR: https://git.openjdk.java.net/jfx/pull/642

Re: [jfx17u] RFR: 8273732: Clarify review policies for clean backports in JavaFX update releases [v2]

[Phil Race]

On Wed, 15 Sep 2021 23:06:39 GMT, Kevin Rushforth  wrote:

>> Added a paragraph indicating that a review of a clean backport to an update 
>> release is optional, if the bug in question has been approved for inclusion 
>> into the release.
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Minor wording update

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jfx17u/pull/9

Re: RFR: 8269638: Property methods, setters, and getters in printing API should be final [v4]

[Phil Race]

On Fri, 16 Jul 2021 21:26:52 GMT, Kevin Rushforth  wrote:

> Yes, this null check is needed, since we specify that setting the `printer` 
> property to `null` will use the default printer.


oh yeah. So nothing to do here.

The only question is about the behaviour of
 public static final PrinterJob createPrinterJob(Printer printer)  {}
when called with null.

- Document the NPE
- Don't do anything 
-  Substitute the default printer and create the job and document this
- return null if there isn't one and document this.
-

-

PR: https://git.openjdk.java.net/jfx/pull/574

Re: RFR: 8269638: Property methods, setters, and getters in printing API should be final [v4]

[Phil Race]

On Fri, 16 Jul 2021 13:41:15 GMT, Kevin Rushforth  wrote:

> > looks good..
> > However I have a different question...I was looking at printerProperty and 
> > I saw In l182, we are not checking for getDefaultPrinter() returns null or 
> > not but in l120, we do...Is it not required in l182?
> 
> It might be a good follow-on bug to skip lines 185-6 if `getDefaultPrinter()` 
> is `null` (else it will NPE), but it's completely unrelated to this fix so 
> wouldn't be done as part of this PR.

I don't think there's a problem here. 
Else we'd have had a thousand bug reports by now.
Printer can never be null. Its instaniated by the private PrinterJob 
constructor.
So perhaps more to the point is are the lines right before that necessary ?


  if (value == null) {
value = Printer.getDefaultPrinter();
}


The public no-args factory method returns null if it can't find a printer and 
the one
where the app supplies the printer to the factory method that takes one it NPEs 
out in the internal constuctor
if you pass in the illegal value NULL as the printer.

There's a small issue there as the doc doesn't actually say that but anyone
who has tried it will have found out pretty quickly !

-

PR: https://git.openjdk.java.net/jfx/pull/574

Integrated: 8269638: Property methods, setters, and getters in printing API should be final

[Phil Race]

On Mon, 12 Jul 2021 18:50:34 GMT, Phil Race  wrote:

> - Make various setters and getters and properties final as needed
> - Move documentation to the property so the setters and getters inherit it, 
> with an exception for the special case of JobSettings.setPageRanges()
> - Override toString() on the properties in JobSettings so it doesn't delegate 
> to the JobSettings class.
> - Add a manual test program just so you can see what toString() does. No pass 
> or fail, just informative.
> 
> This will need a CSR but I won't create that until the review is done.

This pull request has now been integrated.

Changeset: 8b8cea23
Author:Phil Race 
URL:   
https://git.openjdk.java.net/jfx/commit/8b8cea23d1fd5c1c149e0143e2f6bf3312b5ab2e
Stats: 433 lines in 4 files changed: 265 ins; 134 del; 34 mod

8269638: Property methods, setters, and getters in printing API should be final

Reviewed-by: kcr, psadhukhan

-

PR: https://git.openjdk.java.net/jfx/pull/574

Re: RFR: 8269638: Property methods, setters, and getters in printing API should be final [v3]

[Phil Race]

On Thu, 15 Jul 2021 04:30:42 GMT, Prasanta Sadhukhan  
wrote:

>> Phil Race has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8269638: Property methods, setters, and getters in printing API should be 
>> final
>
> modules/javafx.graphics/src/main/java/javafx/print/PrinterJob.java line 226:
> 
>> 224:  * Setting a null value for printer will install the default 
>> printer.
>> 225:  * Setting the current printer has no effect.
>> 226:  * @return the Printer for this job
> 
> If we are using {@code Printer} preferred pattern above for new addition, 
> probably we should use here too, no?

done

> tests/manual/printing/JobSettingsInfo.java line 45:
> 
>> 43: import javafx.scene.Scene;
>> 44: import javafx.scene.control.TextArea;
>> 45: import javafx.scene.layout.*;
> 
> One more place wildcard overlooked...Since we changed one case above, 
> probably it can also be rectified..

done

-

PR: https://git.openjdk.java.net/jfx/pull/574

Re: RFR: 8269638: Property methods, setters, and getters in printing API should be final [v4]

[Phil Race]

> - Make various setters and getters and properties final as needed
> - Move documentation to the property so the setters and getters inherit it, 
> with an exception for the special case of JobSettings.setPageRanges()
> - Override toString() on the properties in JobSettings so it doesn't delegate 
> to the JobSettings class.
> - Add a manual test program just so you can see what toString() does. No pass 
> or fail, just informative.
> 
> This will need a CSR but I won't create that until the review is done.

Phil Race has updated the pull request incrementally with one additional commit 
since the last revision:

  8269638: Property methods, setters, and getters in printing API should be 
final

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/574/files
  - new: https://git.openjdk.java.net/jfx/pull/574/files/6a84d1d9..3da5e4df

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=574=03
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=574=02-03

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jfx/pull/574.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/574/head:pull/574

PR: https://git.openjdk.java.net/jfx/pull/574

Re: RFR: 8269638: Property methods, setters, and getters in printing API should be final [v3]

[Phil Race]

On Wed, 14 Jul 2021 20:44:47 GMT, Phil Race  wrote:

>> - Make various setters and getters and properties final as needed
>> - Move documentation to the property so the setters and getters inherit it, 
>> with an exception for the special case of JobSettings.setPageRanges()
>> - Override toString() on the properties in JobSettings so it doesn't 
>> delegate to the JobSettings class.
>> - Add a manual test program just so you can see what toString() does. No 
>> pass or fail, just informative.
>> 
>> This will need a CSR but I won't create that until the review is done.
>
> Phil Race has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8269638: Property methods, setters, and getters in printing API should be 
> final

uploaded new commit

-

PR: https://git.openjdk.java.net/jfx/pull/574

Re: RFR: 8269638: Property methods, setters, and getters in printing API should be final [v2]

[Phil Race]

On Wed, 14 Jul 2021 06:29:22 GMT, Prasanta Sadhukhan  
wrote:

>> Phil Race has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8269638: Property methods, setters, and getters in printing API should be 
>> final
>
> tests/manual/printing/JobSettingsInfo.java line 31:
> 
>> 29: import javafx.collections.FXCollections;
>> 30: import javafx.collections.ObservableList;
>> 31: import javafx.print.*;
> 
> Maybe we can do away with this wildcard and import explicit class.. Also, 
> down below one more place...

I have cleaned up wild card and unused imports.

-

PR: https://git.openjdk.java.net/jfx/pull/574

Re: RFR: 8269638: Property methods, setters, and getters in printing API should be final [v3]

[Phil Race]

> - Make various setters and getters and properties final as needed
> - Move documentation to the property so the setters and getters inherit it, 
> with an exception for the special case of JobSettings.setPageRanges()
> - Override toString() on the properties in JobSettings so it doesn't delegate 
> to the JobSettings class.
> - Add a manual test program just so you can see what toString() does. No pass 
> or fail, just informative.
> 
> This will need a CSR but I won't create that until the review is done.

Phil Race has updated the pull request incrementally with one additional commit 
since the last revision:

  8269638: Property methods, setters, and getters in printing API should be 
final

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/574/files
  - new: https://git.openjdk.java.net/jfx/pull/574/files/380ad63f..6a84d1d9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=574=02
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=574=01-02

  Stats: 22 lines in 1 file changed: 12 ins; 9 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/574.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/574/head:pull/574

PR: https://git.openjdk.java.net/jfx/pull/574

Re: RFR: 8269638: Property methods, setters, and getters in printing API should be final [v2]

[Phil Race]

On Mon, 12 Jul 2021 22:04:25 GMT, Phil Race  wrote:

>> - Make various setters and getters and properties final as needed
>> - Move documentation to the property so the setters and getters inherit it, 
>> with an exception for the special case of JobSettings.setPageRanges()
>> - Override toString() on the properties in JobSettings so it doesn't 
>> delegate to the JobSettings class.
>> - Add a manual test program just so you can see what toString() does. No 
>> pass or fail, just informative.
>> 
>> This will need a CSR but I won't create that until the review is done.
>
> Phil Race has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8269638: Property methods, setters, and getters in printing API should be 
> final

The CSR has been created : https://bugs.openjdk.java.net/browse/JDK-8270381

-

PR: https://git.openjdk.java.net/jfx/pull/574

Re: RFR: 8269638: Property methods, setters, and getters in printing API should be final

[Phil Race]

On Mon, 12 Jul 2021 18:50:34 GMT, Phil Race  wrote:

> - Make various setters and getters and properties final as needed
> - Move documentation to the property so the setters and getters inherit it, 
> with an exception for the special case of JobSettings.setPageRanges()
> - Override toString() on the properties in JobSettings so it doesn't delegate 
> to the JobSettings class.
> - Add a manual test program just so you can see what toString() does. No pass 
> or fail, just informative.
> 
> This will need a CSR but I won't create that until the review is done.

fixed all the above and uploaded new commit

-

PR: https://git.openjdk.java.net/jfx/pull/574

Re: RFR: 8269638: Property methods, setters, and getters in printing API should be final [v2]

[Phil Race]

On Mon, 12 Jul 2021 21:28:41 GMT, Kevin Rushforth  wrote:

>> Phil Race has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8269638: Property methods, setters, and getters in printing API should be 
>> final
>
> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 968:
> 
>> 966:  * and it is safest to stick to selecting a standard value that
>> 967:  * matches the requirement.
>> 968:  * 
> 
> Empty `` tag (you can simply remove this line).

done

> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 1049:
> 
>> 1047:  * and it is safest to stick to selecting a standard value that
>> 1048:  * matches the requirement.
>> 1049:  * 
> 
> Empty `` tag (you can simply remove this line).

done

-

PR: https://git.openjdk.java.net/jfx/pull/574

Re: RFR: 8269638: Property methods, setters, and getters in printing API should be final [v2]

[Phil Race]

> - Make various setters and getters and properties final as needed
> - Move documentation to the property so the setters and getters inherit it, 
> with an exception for the special case of JobSettings.setPageRanges()
> - Override toString() on the properties in JobSettings so it doesn't delegate 
> to the JobSettings class.
> - Add a manual test program just so you can see what toString() does. No pass 
> or fail, just informative.
> 
> This will need a CSR but I won't create that until the review is done.

Phil Race has updated the pull request incrementally with one additional commit 
since the last revision:

  8269638: Property methods, setters, and getters in printing API should be 
final

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/574/files
  - new: https://git.openjdk.java.net/jfx/pull/574/files/4fad1a0d..380ad63f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=574=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=574=00-01

  Stats: 33 lines in 2 files changed: 11 ins; 18 del; 4 mod
  Patch: https://git.openjdk.java.net/jfx/pull/574.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/574/head:pull/574

PR: https://git.openjdk.java.net/jfx/pull/574

RFR: 8269638: Property methods, setters, and getters in printing API should be final

[Phil Race]

- Make various setters and getters and properties final as needed
- Move documentation to the property so the setters and getters inherit it, 
with an exception for the special case of JobSettings.setPageRanges()
- Override toString() on the properties in JobSettings so it doesn't delegate 
to the JobSettings class.
- Add a manual test program just so you can see what toString() does. No pass 
or fail, just informative.

This will need a CSR but I won't create that until the review is done.

-

Commit messages:
 - 8269638: Property methods, setters, and getters in printing API should be 
final

Changes: https://git.openjdk.java.net/jfx/pull/574/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=574=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8269638
  Stats: 402 lines in 4 files changed: 253 ins; 118 del; 31 mod
  Patch: https://git.openjdk.java.net/jfx/pull/574.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/574/head:pull/574

PR: https://git.openjdk.java.net/jfx/pull/574

Integrated: 8223717: javafx printing: Support Specifying Print to File in the API

[Phil Race]

On Thu, 24 Jun 2021 22:06:37 GMT, Phil Race  wrote:

> This enhancement adds the String property outputFileProperty() to the 
> JobSettings class.
> The value should be a string that references a local file encoded as a URL.
> If this is non-null and set to a location that the user has permission to 
> write to,
> then the printer output will be spooled there instead of the printer, so long 
> as the platform printing system supports this.
> The user can of course also set a print-to-file destination in the platform 
> printer dialogs which may over-ride what the application set. But now the 
> application can also see what it was set to, and cancel or alter it if 
> necessary.
> 
> A simple manual test is provided, manual mainly because the few real printing 
> functional tests are all manual as they are only useful if run with a printer 
> configured.

This pull request has now been integrated.

Changeset: 386f6d7a
Author:Phil Race 
URL:   
https://git.openjdk.java.net/jfx/commit/386f6d7a56d9cfb334c210fccd29e1c5da58b591
Stats: 286 lines in 3 files changed: 284 ins; 0 del; 2 mod

8223717: javafx printing: Support Specifying Print to File in the API

Reviewed-by: kcr, psadhukhan

-

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8246104: Some complex text doesn't render correctly on macOS

[Phil Race]

On Fri, 2 Jul 2021 18:29:12 GMT, Phil Race  wrote:

>> They are in there. But with the current approach for CT glyph-parsing, I see 
>> no other way. The parsing is done in native code (e.g. 
>> OS.CTLineCreateWithAttributedString()) but we extract the required font from 
>> the returned runs, e.g. 
>> `String fontName = CTFontCopyAttributeDisplayName(actualFont);`
>> Next, the `fontName` is searched for, but if it is not in the "public" list 
>> of fonts, this fails. So if we don't make these dot fonts public, this 
>> approach doesn't work.
>
> When you say "public" you mean in the lists that the implementation searches 
> for a match ?
> This could get tricky. I don't have a complete answer off the top of my head.
> Could we at least filter the returned list of full and family names to 
> exclude them ?
> Then explicit searches would work but no one will be seeing them in a list of 
> fonts available to apps.

the filter would be macOS only and exlude fonts with a name beginning with "."

-

PR: https://git.openjdk.java.net/jfx/pull/547

Re: RFR: 8246104: Some complex text doesn't render correctly on macOS

[Phil Race]

On Fri, 2 Jul 2021 12:34:32 GMT, Johan Vos  wrote:

>> Another thought .. after doing this do these "." fonts appear in the list of 
>> fonts reported by
>> javafx.scene.text.Font.getFontNames() ?
>> 
>> I suspect they really should not ..
>
> They are in there. But with the current approach for CT glyph-parsing, I see 
> no other way. The parsing is done in native code (e.g. 
> OS.CTLineCreateWithAttributedString()) but we extract the required font from 
> the returned runs, e.g. 
> `String fontName = CTFontCopyAttributeDisplayName(actualFont);`
> Next, the `fontName` is searched for, but if it is not in the "public" list 
> of fonts, this fails. So if we don't make these dot fonts public, this 
> approach doesn't work.

When you say "public" you mean in the lists that the implementation searches 
for a match ?
This could get tricky. I don't have a complete answer off the top of my head.
Could we at least filter the returned list of full and family names to exclude 
them ?
Then explicit searches would work but no one will be seeing them in a list of 
fonts available to apps.

-

PR: https://git.openjdk.java.net/jfx/pull/547

Re: RFR: 8269593: Different fontname on macos [v2]

[Phil Race]

On Tue, 29 Jun 2021 14:20:30 GMT, Johan Vos  wrote:

>> Make sure the returned fullName of a created font matches the requested 
>> name. Since the name is used as a key/identifier in some cases, some 
>> internal code relies on this.
>> Added a test to check the case of "System Font Regular" on MacOS, which 
>> fails before and succeeds after the patch.
>
> Johan Vos has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   rename test

modules/javafx.graphics/src/main/java/com/sun/javafx/font/PrismFontFile.java 
line 612:

> 610: if (name != null) {
> 611: fullName = name;
> 612: }

So I suppose this is predicated on the assumption that the name being passed in 
is retrieved from the platform for *all* platforms (not just Mac) so we are 
sure it is a valid name ?
I'd be interested to see what the difference is in the full names we then 
display on each platform and whether locale matters .. the name got from the 
platform might be a locale name and that may not have been the intention here 
..  and is it possible "name" has already had toLowerCase() called on it ?

-

PR: https://git.openjdk.java.net/jfx/pull/553

Re: RFR: 8246104: Some complex text doesn't render correctly on macOS

[Phil Race]

On Thu, 1 Jul 2021 20:53:36 GMT, Phil Race  wrote:

>> [Mac only] register system fonts.
>> Fix for JDK-8246104
>> 
>> The list of available fonts returned by 
>> CTFontCollectionCreateFromAvailableFonts does not contain internal fonts (at 
>> least not by default, although this is not documented). By registering 
>> font(s) (files), those fonts become available in the returned list.
>> The CT Glyph processing might assign internal fonts to a glyph, and since we 
>> lookup the requested font in this list, we fail if the list doesn't contain 
>> the font.
>> This PR registers all fonts in the system library so that they become 
>> available. This is not creating additional Java objects or overhead, as it 
>> almost directly invokes `CTFontManagerRegisterFontsForURL` via 
>> `CTFontFile.registerFont(String fontfile)`
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/font/MacFontFinder.java 
> line 83:
> 
>> 81: Stream stream = 
>> Files.list(Paths.get("/System/Library/Fonts"));
>> 82: stream.forEach(f -> 
>> PrismFontFactory.getFontFactory().registerEmbeddedFont(f.toString()));
>> 83: } catch (IOException e) {
> 
> registerEmbeddedFont() is intended to be used for fonts embedded in an 
> application not for system fonts. 
> Also is there anywhere else we are hard-coding /System/Library/Fonts. Apple 
> move things around.
> Also as well as likely needing to use a different call to register, you 
> should make sure that the API
> is registering all the fonts in a collection.
> Finally (I think) what happens if you print ? The font info needs to be 
> passed over to J2D.

Another thought .. after doing this do these "." fonts appear in the list of 
fonts reported by
javafx.scene.text.Font.getFontNames() ?

I suspect they really should not ..

-

PR: https://git.openjdk.java.net/jfx/pull/547

Re: RFR: 8246104: Some complex text doesn't render correctly on macOS

[Phil Race]

On Sat, 26 Jun 2021 15:40:47 GMT, Johan Vos  wrote:

> [Mac only] register system fonts.
> Fix for JDK-8246104
> 
> The list of available fonts returned by 
> CTFontCollectionCreateFromAvailableFonts does not contain internal fonts (at 
> least not by default, although this is not documented). By registering 
> font(s) (files), those fonts become available in the returned list.
> The CT Glyph processing might assign internal fonts to a glyph, and since we 
> lookup the requested font in this list, we fail if the list doesn't contain 
> the font.
> This PR registers all fonts in the system library so that they become 
> available. This is not creating additional Java objects or overhead, as it 
> almost directly invokes `CTFontManagerRegisterFontsForURL` via 
> `CTFontFile.registerFont(String fontfile)`

modules/javafx.graphics/src/main/java/com/sun/javafx/font/MacFontFinder.java 
line 83:

> 81: Stream stream = 
> Files.list(Paths.get("/System/Library/Fonts"));
> 82: stream.forEach(f -> 
> PrismFontFactory.getFontFactory().registerEmbeddedFont(f.toString()));
> 83: } catch (IOException e) {

registerEmbeddedFont() is intended to be used for fonts embedded in an 
application not for system fonts. 
Also is there anywhere else we are hard-coding /System/Library/Fonts. Apple 
move things around.
Also as well as likely needing to use a different call to register, you should 
make sure that the API
is registering all the fonts in a collection.
Finally (I think) what happens if you print ? The font info needs to be passed 
over to J2D.

-

PR: https://git.openjdk.java.net/jfx/pull/547

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v5]

[Phil Race]

On Wed, 30 Jun 2021 22:30:47 GMT, Kevin Rushforth  wrote:

>> Phil Race has updated the pull request incrementally with three additional 
>> commits since the last revision:
>> 
>>  - 8223717: javafx printing: Support Specifying Print to File in the API
>>  - 8223717: javafx printing: Support Specifying Print to File in the API
>>  - 8223717: javafx printing: Support Specifying Print to File in the API
>
> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 481:
> 
>> 479:  * such as Postscript or PDF, and the application intends to 
>> distribute
>> 480:  * the result instead of printing it, or for some other reason the
>> 481:  * application does not want physical media (paper) emitted by the 
>> printer.
> 
> Very minor: maybe consider combining the first three paragraphs into a single 
> paragraph?

well .. I usually like a short paragraph that succinctly says what it does as 
the first paragraph

Anyway I've re-read all this and I prefer it as it is.

> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 486:
> 
>> 484:  * equivalent to null, which means output is sent to the printer.
>> 485:  * So in order to reset to print to the printer, clear the value of
>> 486:  * this property by setting it to null or an empty string.
> 
> This doesn't flow as well as it could. I think you only need to mention once 
> that `null` is the same as an empty string, and then you can just say "empty 
> string". Maybe something like this?
> 
> 
> The default value is an empty string, which means output is sent to the 
> printer. So in order to reset
> to print to the printer, clear the value of this property by setting it to an 
> empty string. A value
> of {@code null} is treated as an empty string.

But I don't say it twice. I say
>  The default value is an empty string, which is interpreted as unset, 
> equivalent to null,

and 
>  clear the value of this property by setting it to null or an empty string.

which is somewhat different and makes i t very clear that either will work .. 

So I think this is all fine and flows as intended.

> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 489:
> 
>> 487:  * 
>> 488:  * Additionally if the application displays a printer dialog which 
>> allows
>> 489:  * the user to specify a file destination including altering an 
>> application
> 
> Minor: I think there should be a comma between `destination` and `including`.

yep

> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 491:
> 
>> 489:  * the user to specify a file destination including altering an 
>> application
>> 490:  * specified file destination, the value of this property will 
>> reflect that
>> 491:  * user-specified choice, including clearing it to re-set to print 
>> to
> 
> `reset` is one word (no need to hyphenate).

it is reset elsewhere not sure why a hyphen is here.

> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 500:
> 
>> 498:  * a user writable file, when printing the results are 
>> platform-dependent, including
>> 499:  * replacement with a default output file location, printing to the 
>> printer instead,
>> 500:  * or a platform printing error.
> 
> This sentence is a bit awkward and hard to parse. Maybe break it into two 
> sentences? Perhaps something like this:
> 
> 
> If the specified name specifies a non-existent path, or does not specify a 
> user writable file, the
> results when printing are platform-dependent. Possible behavior might include 
> replacement with
> a default output file location, printing to the printer instead, or a 
> platform printing error.

ok split it

-

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v6]

[Phil Race]

> This enhancement adds the String property outputFileProperty() to the 
> JobSettings class.
> The value should be a string that references a local file encoded as a URL.
> If this is non-null and set to a location that the user has permission to 
> write to,
> then the printer output will be spooled there instead of the printer, so long 
> as the platform printing system supports this.
> The user can of course also set a print-to-file destination in the platform 
> printer dialogs which may over-ride what the application set. But now the 
> application can also see what it was set to, and cancel or alter it if 
> necessary.
> 
> A simple manual test is provided, manual mainly because the few real printing 
> functional tests are all manual as they are only useful if run with a printer 
> configured.

Phil Race has updated the pull request incrementally with one additional commit 
since the last revision:

  8223717: javafx printing: Support Specifying Print to File in the API

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/543/files
  - new: https://git.openjdk.java.net/jfx/pull/543/files/baab574d..1d86a803

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=543=05
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=543=04-05

  Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod
  Patch: https://git.openjdk.java.net/jfx/pull/543.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/543/head:pull/543

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v4]

[Phil Race]

On Mon, 28 Jun 2021 19:47:39 GMT, Phil Race  wrote:

>> This enhancement adds the String property outputFileProperty() to the 
>> JobSettings class.
>> The value should be a string that references a local file encoded as a URL.
>> If this is non-null and set to a location that the user has permission to 
>> write to,
>> then the printer output will be spooled there instead of the printer, so 
>> long as the platform printing system supports this.
>> The user can of course also set a print-to-file destination in the platform 
>> printer dialogs which may over-ride what the application set. But now the 
>> application can also see what it was set to, and cancel or alter it if 
>> necessary.
>> 
>> A simple manual test is provided, manual mainly because the few real 
>> printing functional tests are all manual as they are only useful if run with 
>> a printer configured.
>
> Phil Race has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8223717: javafx printing: Support Specifying Print to File in the API

CSR: https://bugs.openjdk.java.net/browse/JDK-8269642

-

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v5]

[Phil Race]

> This enhancement adds the String property outputFileProperty() to the 
> JobSettings class.
> The value should be a string that references a local file encoded as a URL.
> If this is non-null and set to a location that the user has permission to 
> write to,
> then the printer output will be spooled there instead of the printer, so long 
> as the platform printing system supports this.
> The user can of course also set a print-to-file destination in the platform 
> printer dialogs which may over-ride what the application set. But now the 
> application can also see what it was set to, and cancel or alter it if 
> necessary.
> 
> A simple manual test is provided, manual mainly because the few real printing 
> functional tests are all manual as they are only useful if run with a printer 
> configured.

Phil Race has updated the pull request incrementally with three additional 
commits since the last revision:

 - 8223717: javafx printing: Support Specifying Print to File in the API
 - 8223717: javafx printing: Support Specifying Print to File in the API
 - 8223717: javafx printing: Support Specifying Print to File in the API

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/543/files
  - new: https://git.openjdk.java.net/jfx/pull/543/files/34221a95..baab574d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=543=04
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=543=03-04

  Stats: 4 lines in 2 files changed: 1 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jfx/pull/543.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/543/head:pull/543

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]

[Phil Race]

On Fri, 25 Jun 2021 04:08:30 GMT, Prasanta Sadhukhan  
wrote:

>> Phil Race has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8223717: javafx printing: Support Specifying Print to File in the API
>
> modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinterJob.java
>  line 352:
> 
>> 350: try {
>> 351: 
>> settings.setOutputFile(dest.getURI().toURL().toString());
>> 352: } catch (MalformedURLException e) {
> 
> I guess in 2D RasterPrinterJob, if there is any exception, we try to form a 
> default file "out.prn"  
> [https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/print/RasterPrinterJob.java#L1137]
> and try to print there. Can't we do it here too?

I am going to just change to support file so this becomes a non-issue

> modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinterJob.java
>  line 356:
> 
>> 354: } else {
>> 355: settings.setOutputFile("");
>> 356: }
> 
> Actually in 2D , it seems if dest is null, we redirect printing to actual 
> printer instead of file. Not sure if that is what we need to do here too!!
> [https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/print/RasterPrinterJob.java#L1148]

that is what we are doing ..

> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 485:
> 
>> 483:  * 
>> 484:  * If the application does not have permission to write to the 
>> specified
>> 485:  * file, a {@code SecurityException} may be thrown when printing.
> 
> As I see in 2D atleast in Win32PrintService, if there is a SecurityException 
> for creating a Destination object from a URI, it retries again by creating a 
> new URL with default file "file:out.prn"
> Is it not needed here?
> [https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#L1181]

No. We are doing our own checking if a SM is running and disallow it.

-

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v2]

[Phil Race]

On Fri, 25 Jun 2021 21:10:16 GMT, Kevin Rushforth  wrote:

>> But the JobSettings class is final .. is it still necessary ?
>
> It's still a good idea to follow the pattern, so yes let's make the new 
> methods final. We can file a cleanup bug for the existing ones (and since the 
> class is final, we can do it without compatibility concerns).
> 
> Also, I built the docs, and it is better to not add any javadoc comments on 
> the setter and getter.

I'll file a clean up bug

-

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]

[Phil Race]

On Fri, 25 Jun 2021 21:13:31 GMT, Kevin Rushforth  wrote:

>> Phil Race has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8223717: javafx printing: Support Specifying Print to File in the API
>
> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 492:
> 
>> 490:  * a writable file it may be ignored, or a SecurityException may be 
>> thrown.
>> 491:  * The default value is an empty string, which is interpreted as 
>> unset,
>> 492:  * which means output is sent to the printer.
> 
> Can you add a `* @defaultValue empty string` javadoc tag?

will do  ..

> tests/manual/printing/PrintToFileTest.java line 134:
> 
>> 132: job.printPage(printNode);
>> 133: job.endJob();
>> 134: if (f.exists()) {
> 
> I have only tried this on Windows. This check is failing for me when I run 
> the test, and yet the file is created correctly. If I add a short sleep, it 
> works (so the printing is likely being done on a different thread).

interesting. I guess I will have to add a sleep as you did.

> tests/manual/printing/PrintToFileTest.java line 137:
> 
>> 135: System.out.println("created file");
>> 136: passed = true;
>> 137: f.delete();
> 
> Maybe leave the file? I found it useful to verify its contents.

ok

-

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v4]

[Phil Race]

On Thu, 24 Jun 2021 22:53:33 GMT, Phil Race  wrote:

>> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 490:
>> 
>>> 488:  * setting will be ignored.
>>> 489:  * If the URL specifies a non-existent path, or does not specify
>>> 490:  * a writable file it may be ignored, or a SecurityException may 
>>> be thrown.
>> 
>> I want to look at what we do for other properties like this. I'm not sure 
>> how feasible it is to throw an exception back to the application. And I 
>> wouldn't want to single out a security exception (what about a read-only 
>> file system?). I think that whatever the reason, if the file can't be 
>> written to, we should treat it consistently. Probably by ignoring the 
>> request. I presume that's what will happen if the print driver doesn't 
>> support print-to-file?
>
> Not easy to specify exactly. We are handing it off to 2D
> We aren't doing any up-front verification here, but I don't want to copy 2D 
> behaviour into the docs, even if I could be sure about that .. I may just 
> want to be more waffly about what happens.
> Could be printed to the printer, could be a failed job, could be an exception 
> of some kind if some nice piece of Java code recognises a problem up-front

So the SE will occur only if there is a SM and the code in 
J2DPrinterJob.checkPermission() rejects it.

-

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v4]

[Phil Race]

On Fri, 25 Jun 2021 21:08:31 GMT, Kevin Rushforth  wrote:

>> I agree with @kevinrushforth that if a String without protocol is passed, it 
>> should be treated as a file (absolute or relative to ?) 
>> I'm also not sure that the URL should be exposed here. I understand it's 
>> needed in the lower-level print API but you already do the conversion in the 
>> `syncOutputFile` method. Hence, since only the file protocol is supported, 
>> it might be easier for API users to pass the location of the file instead of 
>> a URL.
>> In case later other URL protocols are supported, a property `outputURL` 
>> might be introduced?
>
> I like the flexibility and consistency of defining it as a URL, as long as we 
> also interpret a url without a scheme as a file name. Borrowing language from 
> the Image docs, perhaps something like this?
> 
> 
> The URL string can either be a URL with a "file:" protocol that can be 
> resolved
> by @link java.net.URL} or a file path that can be resolved by {@link 
> java.io.File}.

I am changing it to be just a file path. We can extend it later if ever needed.
It does simplify the code and the potential for errors.

-

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]

[Phil Race]

On Fri, 25 Jun 2021 21:20:53 GMT, Kevin Rushforth  wrote:

>> modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinterJob.java
>>  line 839:
>> 
>>> 837: security.checkPrintJobAccess();
>>> 838: String file = settings.getOutputFile();
>>> 839: if (!file.isEmpty()) {
>> 
>> Don't we need to check for file!= null?
>
> The default value for the property is the empty string. But it does bring up 
> a good point that we should either check and throw NPE if `setOutputFile` is 
> called with `null` or we should map null to the empty string.

we do remap null to the empty string .. the code in 
JobSettings.outputFileProperty() does it.
So this will never be null

-

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v4]

[Phil Race]

> This enhancement adds the String property outputFileProperty() to the 
> JobSettings class.
> The value should be a string that references a local file encoded as a URL.
> If this is non-null and set to a location that the user has permission to 
> write to,
> then the printer output will be spooled there instead of the printer, so long 
> as the platform printing system supports this.
> The user can of course also set a print-to-file destination in the platform 
> printer dialogs which may over-ride what the application set. But now the 
> application can also see what it was set to, and cancel or alter it if 
> necessary.
> 
> A simple manual test is provided, manual mainly because the few real printing 
> functional tests are all manual as they are only useful if run with a printer 
> configured.

Phil Race has updated the pull request incrementally with one additional commit 
since the last revision:

  8223717: javafx printing: Support Specifying Print to File in the API

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/543/files
  - new: https://git.openjdk.java.net/jfx/pull/543/files/8050d5f9..34221a95

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=543=03
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=543=02-03

  Stats: 85 lines in 3 files changed: 21 ins; 38 del; 26 mod
  Patch: https://git.openjdk.java.net/jfx/pull/543.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/543/head:pull/543

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]

[Phil Race]

On Thu, 24 Jun 2021 23:19:22 GMT, Kevin Rushforth  wrote:

>> We aren't ignoring it .. its just handed on, so its going to be the same as 
>> the other non-writable case .. file could not be created .. printing error 
>> .. 
>> we could do some up-front validation if that seems like the way we want to 
>> go. This could I guess also check if the file is writeable .. but since the 
>> user can select a non-writable file location in the dialog I don't think we 
>> should do that part. In which case I wonder about the URL validation too ..
>
> Consistency with other APIs would be the main reason. We could do something 
> similar to what we do for `Image`, and treat a string without a protocol as a 
> file (although not relative to the classpath as in the case of images), 
> turning it into a `file:` URL internally where needed.
> 
> @prrace what do you think?

> As a potential user of this API I have to ask, why would this be a URL 
> instead of a File or Path ? Particularly if only "file:" URLs will be valid. 
> I can't think of a scenario where it won't always be an extra inconvenience 
> to convert the path I have to a URL.

I will think about it but not promising yet.
 We only forsee (for now) a file ..  We use URL a lot and also the javax.print 
API in 2D uses URL .. and so 2D hands us a file URL as well as expecting one .. 
and FX printing depends heavily on that right now so there's no escaping URLs 
somewhere.

-

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v2]

[Phil Race]

On Thu, 24 Jun 2021 22:42:00 GMT, Kevin Rushforth  wrote:

>> Phil Race has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - 8223717: javafx printing: Support Specifying Print to File in the API
>>  - 8223717: javafx printing: Support Specifying Print to File in the API
>
> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 541:
> 
>> 539:  * @since 17
>> 540:  */
>> 541: public String getOutputFile() {
> 
> This should be final (as should the setter). I see that other getter and 
> setter methods in this class are also non-final, but we shouldn't propagate 
> this mistake.
> 
> Also, you don't need javadoc comments on the getters and setters, since it 
> will copy them from the property (and add appropriate links).

But the JobSettings class is final .. is it still necessary ?

-

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]

[Phil Race]

On Thu, 24 Jun 2021 22:53:57 GMT, Scott Palmer  wrote:

>> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 474:
>> 
>>> 472:  * encoded name of a filesystem file, to which the platform printer
>>> 473:  * driver should spool the rendered print data.
>>> 474:  * 
>> 
>> Can you add a sentence indicating that the URL needs to be a `file:` URL? 
>> And indicate what happens if it isn't (I presume it is ignored)?
>
> As a potential user of this API I have to ask, why would this be a URL 
> instead of a File or Path ?  Particularly if only "file:" URLs will be valid. 
>  I can't think of a scenario where it won't always be an extra inconvenience 
> to convert the path I have to a URL.

We aren't ignoring it .. its just handed on, so its going to be the same as the 
other non-writable case .. file could not be created .. printing error .. 
we could do some up-front validation if that seems like the way we want to go. 
This could I guess also check if the file is writeable .. but since the user 
can select a non-writable file location in the dialog I don't think we should 
do that part. In which case I wonder about the URL validation too ..

-

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]

[Phil Race]

> This enhancement adds the String property outputFileProperty() to the 
> JobSettings class.
> The value should be a string that references a local file encoded as a URL.
> If this is non-null and set to a location that the user has permission to 
> write to,
> then the printer output will be spooled there instead of the printer, so long 
> as the platform printing system supports this.
> The user can of course also set a print-to-file destination in the platform 
> printer dialogs which may over-ride what the application set. But now the 
> application can also see what it was set to, and cancel or alter it if 
> necessary.
> 
> A simple manual test is provided, manual mainly because the few real printing 
> functional tests are all manual as they are only useful if run with a printer 
> configured.

Phil Race has updated the pull request incrementally with one additional commit 
since the last revision:

  8223717: javafx printing: Support Specifying Print to File in the API

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/543/files
  - new: https://git.openjdk.java.net/jfx/pull/543/files/1c21e10f..8050d5f9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=543=02
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=543=01-02

  Stats: 4 lines in 1 file changed: 1 ins; 1 del; 2 mod
  Patch: https://git.openjdk.java.net/jfx/pull/543.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/543/head:pull/543

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]

[Phil Race]

On Thu, 24 Jun 2021 22:38:40 GMT, Kevin Rushforth  wrote:

>> Phil Race has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8223717: javafx printing: Support Specifying Print to File in the API
>
> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 490:
> 
>> 488:  * setting will be ignored.
>> 489:  * If the URL specifies a non-existent path, or does not specify
>> 490:  * a writable file it may be ignored, or a SecurityException may be 
>> thrown.
> 
> I want to look at what we do for other properties like this. I'm not sure how 
> feasible it is to throw an exception back to the application. And I wouldn't 
> want to single out a security exception (what about a read-only file 
> system?). I think that whatever the reason, if the file can't be written to, 
> we should treat it consistently. Probably by ignoring the request. I presume 
> that's what will happen if the print driver doesn't support print-to-file?

Not easy to specify exactly. We are handing it off to 2D
We aren't doing any up-front verification here, but I don't want to copy 2D 
behaviour into the docs, even if I could be sure about that .. I may just want 
to be more waffly about what happens.
Could be printed to the printer, could be a failed job, could be an exception 
of some kind if some nice piece of Java code recognises a problem up-front

-

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v2]

[Phil Race]

> This enhancement adds the String property outputFileProperty() to the 
> JobSettings class.
> The value should be a string that references a local file encoded as a URL.
> If this is non-null and set to a location that the user has permission to 
> write to,
> then the printer output will be spooled there instead of the printer, so long 
> as the platform printing system supports this.
> The user can of course also set a print-to-file destination in the platform 
> printer dialogs which may over-ride what the application set. But now the 
> application can also see what it was set to, and cancel or alter it if 
> necessary.
> 
> A simple manual test is provided, manual mainly because the few real printing 
> functional tests are all manual as they are only useful if run with a printer 
> configured.

Phil Race has updated the pull request incrementally with two additional 
commits since the last revision:

 - 8223717: javafx printing: Support Specifying Print to File in the API
 - 8223717: javafx printing: Support Specifying Print to File in the API

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/543/files
  - new: https://git.openjdk.java.net/jfx/pull/543/files/4edc1315..1c21e10f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=543=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=543=00-01

  Stats: 4 lines in 2 files changed: 0 ins; 1 del; 3 mod
  Patch: https://git.openjdk.java.net/jfx/pull/543.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/543/head:pull/543

PR: https://git.openjdk.java.net/jfx/pull/543

RFR: 8223717: javafx printing: Support Specifying Print to File in the API

[Phil Race]

This enhancement adds the String property outputFileProperty() to the 
JobSettings class.
The value should be a string that references a local file encoded as a URL.
If this is non-null and set to a location that the user has permission to write 
to,
then the printer output will be spooled there instead of the printer, so long 
as the platform printing system supports this.
The user can of course also set a print-to-file destination in the platform 
printer dialogs which may over-ride what the application set. But now the 
application can also see what it was set to, and cancel or alter it if 
necessary.

A simple manual test is provided, manual mainly because the few real printing 
functional tests are all manual as they are only useful if run with a printer 
configured.

-

Commit messages:
 - 8223717: javafx printing: Support Specifying Print to File in the API
 - -8223717: javafx printing: Support Specifying Print to File in the API

Changes: https://git.openjdk.java.net/jfx/pull/543/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=543=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8223717
  Stats: 303 lines in 3 files changed: 301 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jfx/pull/543.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/543/head:pull/543

PR: https://git.openjdk.java.net/jfx/pull/543

Re: RFR: 8262396: Update Mesa 3-D Headers to version 21.0.3

[Phil Race]

On Wed, 5 May 2021 13:45:39 GMT, Kevin Rushforth  wrote:

> Update OpenGL headers to latest versions from Mesa project. I ran a sanity 
> test on all three platforms. I also checked the diffs against the Java2D 
> patch in openjdk/jdk#3854 and they match, excluding the Java2D-specific 
> ifdefs.

Looks good. I also checked against the 2D version.
This may be as close as it gets to reviewing your own changes :-)

-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/486

Re: RFR: 8252099: JavaFX does not render Myanmar script correctly

[Phil Race]

On Tue, 9 Feb 2021 21:59:58 GMT, Jose Pereda  wrote:

> This PR allows rendering Myanmar script correctly, following up on 
> https://bugs.openjdk.java.net/browse/JDK-8223558.

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jfx/pull/399

Re: RFR: 8253356: JavaFX Terminology Refresh [v2]

[Phil Race]

On Wed, 16 Dec 2020 13:20:23 GMT, Kevin Rushforth  wrote:

>> Replace archaic/non-inclusive words in JavaFX with more neutral terms. See 
>> [JDK-8253315](https://bugs.openjdk.java.net/browse/JDK-8253315) for 
>> background information.
>> 
>> The following changes are made:
>> 
>> 1. Rename whitelist/blacklist to allowlist/rejectlist
>> 2. Rename `MasterTimer` to `PrimaryTimer` in animation and toolkit 
>> implementation
>> 3. Rename local variable master in a couple places
>> 4. Remove master from comments in a few other places
>> 
>> This PR doesn't remove uses of the word master where there is no connotation 
>> of slavery.
>> 
>> Note that it is out of scope of this PR to address similar issues in 
>> 3rd-party code, such as WebKit or GStreamer. We will pick up any relevant 
>> changes after they are available in the upstream sources.
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Respond to feedback: change masterWindow to mainWindow in iOS files

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jfx/pull/368

Re: PageOrientation partially ignored?

[Phil Race]

what printer, driver, and os?
did you call any methods to verify the values being passed are supported ?

-Phil.

> On Aug 17, 2020, at 9:01 PM, Tobias Oelgarte  
> wrote:
> 
> I just experimented with the JavaFX printing API and I'm confused about the 
> results.
> 
> If I request to print in landscape format the resulting page is still in 
> portrait mode, but rotated by 90 degrees and with mirrored borders/margins. 
> Is this intentional or should I file a bug report?
> 
> Simplified Example:
> 
> Printer printer = Printer.getDefaultPrinter();
> PageLayout layout = printer.createPageLayout(Paper.A4, 
> PageOrientation.LANDSCAPE, 80, 20, 10, 40);
> PrinterJob job = PrinterJob.createPrinterJob(printer);
> job.getJobSettings().setPageLayout(layout);
> boolean success = job.printPage(someNode);
> if (success) {
>job.endJob();
> }
> 
> What i would expect would be an A4 page in landscape orientation, with a top 
> border (long side) of 10, a bottom border of 40, a left border of 80 and a 
> right border of 20. The content/node will not be rotated.
> 
> But what i get is the following. The paper size is A4 as requested. The 
> orientation is portrait (not as requested). The top border (long side) is 40, 
> the bottom border is 10, the left border is 20, the right border is 80. The 
> content/node is rotated by 90 degrees to the left.
> 
> My guess is that the orientation is partially ignored, resulting in the 
> confusing margins.
> 
> -- 
> Tobias Oelgarte
> Mail: tobias.oelga...@gmail.com

Re: RFR: 8196079: Remove obsolete Pisces rasterizer

[Phil Race]

On Sat, 18 Jul 2020 14:57:08 GMT, Kevin Rushforth  wrote:

> This removes the obsolete OpenPiscesRasterizer (Java-based) and 
> NativePiscesRasterizer implementations. The Marlin
> rasterizer was added in FX 9 and was made the default in FX 10. Marlin both 
> outperforms Pisces and is more robust.
> There is no reason to keep the Pisces rasterizer(s) any more.  Note that the 
> SW pipeline still has a Pisces-based
> renderer for the actual rendering of primitives. This is separate from the 
> rasterizer and is not affected by this
> proposed fix.  I have tested this on Mac, Windows, and Linux.

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jfx/pull/268

Integrated: 8248908: Printer.createPageLayout() returns 0.75" margins instead of hardware margins

[Phil Race]

On Wed, 15 Jul 2020 23:54:59 GMT, Phil Race  wrote:

> For a Media that had no defined size - eg a "custom" paper - the code was 
> creating a mapping
> that over-rode the NA_LETTER size. This might cause multiple problems but 
> definitely meant that
> we ended up with default margins.

This pull request has now been integrated.

Changeset: f056d24b
Author:Phil Race 
URL:   https://git.openjdk.java.net/jfx/commit/f056d24b
Stats: 123 lines in 2 files changed: 2 ins; 111 del; 10 mod

8248908: Printer.createPageLayout() returns 0.75" margins instead of hardware 
margins

Reviewed-by: kcr

-

PR: https://git.openjdk.java.net/jfx/pull/266

Re: RFR: 8248908: Printer.createPageLayout() returns 0.75" margins instead of hardware margins [v2]

[Phil Race]

> For a Media that had no defined size - eg a "custom" paper - the code was 
> creating a mapping
> that over-rode the NA_LETTER size. This might cause multiple problems but 
> definitely meant that
> we ended up with default margins.

Phil Race has updated the pull request incrementally with one additional commit 
since the last revision:

  8248908: Printer.createPageLayout() returns 0.75" instead of hardware margins

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/266/files
  - new: https://git.openjdk.java.net/jfx/pull/266/files/d54cd40d..e517ca55

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/266/webrev.01
 - incr: https://webrevs.openjdk.java.net/jfx/266/webrev.00-01

  Stats: 4 lines in 1 file changed: 1 ins; 1 del; 2 mod
  Patch: https://git.openjdk.java.net/jfx/pull/266.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/266/head:pull/266

PR: https://git.openjdk.java.net/jfx/pull/266

RFR: 8248908: Printer.createPageLayout() returns 0.75"margins instead of hardware margins

[Phil Race]

For a Media that had no defined size - eg a "custom" paper - the code was 
creating a mapping
that over-rode the NA_LETTER size. This might cause multiple problems but 
definitely meant that
we ended up with default margins.

-

Commit messages:
 - 8248908: Printer.createPageLayout() returns 0.75"margins instead of hardware 
margins

Changes: https://git.openjdk.java.net/jfx/pull/266/files
 Webrev: https://webrevs.openjdk.java.net/jfx/266/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8248908
  Stats: 129 lines in 2 files changed: 117 ins; 8 del; 4 mod
  Patch: https://git.openjdk.java.net/jfx/pull/266.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/266/head:pull/266

PR: https://git.openjdk.java.net/jfx/pull/266

Re: RFR: 8208169: can not print selected pages of web page [v5]

[Phil Race]

On Tue, 16 Jun 2020 07:59:02 GMT, Bhawesh Choudhary 
 wrote:

>> Print function of WebEngine.java ignores page range setting and prints given 
>> number of pages starting from first page,
>> which is the root cause of this issue. To fix it, put check for page ranges 
>> and if it available, use it for printing
>> pages otherwise print all pages as usual.
>
> Bhawesh Choudhary has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Formatting changes

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jfx/pull/222

Re: [Rev 03] RFR: 8208169: can not print selected pages of web page

[Phil Race]

On Mon, 15 Jun 2020 05:22:48 GMT, Bhawesh Choudhary 
 wrote:

>> Print function of WebEngine.java ignores page range setting and prints given 
>> number of pages starting from first page,
>> which is the root cause of this issue. To fix it, put check for page ranges 
>> and if it available, use it for printing
>> pages otherwise print all pages as usual.
>
> Bhawesh Choudhary has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   cleanup unused variable

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jfx/pull/222

Re: [Rev 05] RFR: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars

[Phil Race]

On Mon, 15 Jun 2020 16:04:05 GMT, Johan Vos  wrote:

>> This addresses https://bugs.openjdk.java.net/browse/JDK-8246348
>
> Johan Vos has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove trailing whitespace

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jfx/pull/249

Re: [Rev 01] RFR: 8208169: can not print selected pages of web page

[Phil Race]

On Mon, 1 Jun 2020 15:37:09 GMT, Bhawesh Choudhary 
 wrote:

>> Print function of WebEngine.java ignores page range setting and prints given 
>> number of pages starting from first page,
>> which is the root cause of this issue. To fix it, put check for page ranges 
>> and if it available, use it for printing
>> pages otherwise print all pages as usual.
>
> Bhawesh Choudhary has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   added manual test

tests/manual/printing/PrintPageRangeTest.java line 78:

> 77:
> 78: static final String initialURL = 
> "https://en.wikipedia.org/wiki/Java_version_history;;
> 79:

Have you tested this when using a system proxy ? And do the webview tests in 
general use random webpages from the
internet ? Why can't we generate some simple local content that we KNOW is 4 
pages long. You don't even know that for
sure about this page.

-

PR: https://git.openjdk.java.net/jfx/pull/222

Re: [Rev 02] RFR: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars

[Phil Race]

On Fri, 12 Jun 2020 15:01:35 GMT, Johan Vos  wrote:

>> This addresses https://bugs.openjdk.java.net/browse/JDK-8246348
>
> Johan Vos has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   use LinkedHashMap instead of HashMap
>   Only store the pointer to the utf8 string if its creation was successful

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jfx/pull/249

Re: [Rev 02] RFR: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars

[Phil Race]

On Fri, 12 Jun 2020 13:05:03 GMT, Johan Vos  wrote:

>> I agree. the text array is now the subset for the run, so effectively 
>> run.getStart() should be zero,
>> so there should be no need for that call. This probably looks OK when there 
>> is one run not otherwise.
>
> You're both correct. I simplified it.

Looks good now.

-

PR: https://git.openjdk.java.net/jfx/pull/249

Re: [Rev 02] RFR: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars

[Phil Race]

On Fri, 12 Jun 2020 14:49:50 GMT, Johan Vos  wrote:

>> Oh, right. It will be a Long 0, not null. If you store it you will still 
>> have the problem I mentioned with dispose
>> unless you add back in the `str != 0` check. And you would need a check for 
>> `str != 0` in the layout method so that the
>> second time it doesn't treat it as a valid pointer. It might be safer to not 
>> store it if 0, which matches the current
>> behavior?
>
> That makes sense indeed. For the dispose call, a valid pointer is required.

I wasn't completely sure what the idea was, which is why I phrased it as a 
question :-)

-

PR: https://git.openjdk.java.net/jfx/pull/249

Re: RFR: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars

[Phil Race]

On Thu, 11 Jun 2020 19:20:05 GMT, Kevin Rushforth  wrote:

>> I did a quick test, and setting `start = str` fixes the spurious assertions 
>> and intermittent crash.
>
> I should add that this is without any attempt to filter out `0` chars. Both 
> `UnicodeTextTest` and  `UnicodeTextTest2`
> run correctly with no crashes and no assertions when I comment out the 
> (ineffective) loop checking for 0 and set `start
> = str` leaving everything else as you have it in the current PR. Loading 
> https://gluonhq.com/ in WebView works, too.

I agree. the text array is now the subset for the run, so effectively 
run.getStart() should be zero,
so there should be no need for that call. This probably looks OK when there is 
one run not otherwise.

-

PR: https://git.openjdk.java.net/jfx/pull/249

Re: RFR: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars

[Phil Race]

On Tue, 9 Jun 2020 19:33:01 GMT, Johan Vos  wrote:

> This addresses https://bugs.openjdk.java.net/browse/JDK-8246348

modules/javafx.graphics/src/main/java/com/sun/javafx/font/freetype/PangoGlyphLayout.java
 line 140:

> 139: runUtf8.put(run, str);
> 140: if (check(str, "Failed allocating UTF-8 buffer.", context, 
> desc, attrList)) {
> 141: return;

Did you really mean to store the run in the map before checking if str is null ?

-

PR: https://git.openjdk.java.net/jfx/pull/249

Re: RFR: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars

[Phil Race]

On Tue, 9 Jun 2020 19:33:01 GMT, Johan Vos  wrote:

> This addresses https://bugs.openjdk.java.net/browse/JDK-8246348

Instead of a space you could use a ZWNJ U+FEFF. Because that is also the 
endian-ness
mark, Unicode actually prefers you now to use U+2060 but you need to make sure 
these
are processed correctly by pango. Unlike a space they should have no rendering 
effect - not even an advance.

-

PR: https://git.openjdk.java.net/jfx/pull/249

Re: Font rendering issue on macOS - cut off characters

[Phil Race]

PS the spacing is very uneven. That does not look right.

-Phil.

> On May 24, 2020, at 5:16 PM, Phil Race  wrote:
> 
> That looks normal.
> 
> -Phil.
> 
>> On May 24, 2020, at 4:27 PM, Rob Nikander  wrote:
>> 
>> 
>> 
>>> On May 24, 2020, at 5:46 PM, Philip Race  wrote:
>>> 
>>> That's likely LCD sub-pixel text. Its normal to see it if you squint. Not 
>>> new.
>> 
>> I’m doubting that this is normal, but let me try to share a few screenshots 
>> so we can be sure. (All pasted into one PNG for easy comparison):
>> 
>> https://drive.google.com/file/d/1pHtehqm8AP0H_dkV3NdczRdqNSA5gq2o/view?usp=sharing
>> 
>> (I hope that if you view those on a retina display, you will still see the 
>> blockier appearance of my non-retina laptop screen. )
>> 
>> You can see the shaved “o” characters there, but I’m just talking about the 
>> colors now. Is that normal? To me the JavaFX text looks significantly worse 
>> than the native app text [1]. In the native app, I don’t see any color, even 
>> looking real close. The JavaFX seems like it went overboard on something. 
>> 
>> Rob
>> 
>> [1] It’s a screenshot from Pages, but they all look the same.
>> 
>>> macOS has been moving away from it but people with non-retina displays have 
>>> lamented its passing.
>>> 
>>> https://osxdaily.com/2018/09/26/fix-blurry-thin-fonts-text-macos-mojave/
>>> 
>>> FX still requests LCD tho'. Although it is not clear how long macOS will 
>>> continue to provide it.
>> 

Re: Font rendering issue on macOS - cut off characters

[Phil Race]

That looks normal.

-Phil.

> On May 24, 2020, at 4:27 PM, Rob Nikander  wrote:
> 
> 
> 
>> On May 24, 2020, at 5:46 PM, Philip Race  wrote:
>> 
>> That's likely LCD sub-pixel text. Its normal to see it if you squint. Not 
>> new.
> 
> I’m doubting that this is normal, but let me try to share a few screenshots 
> so we can be sure. (All pasted into one PNG for easy comparison):
> 
> https://drive.google.com/file/d/1pHtehqm8AP0H_dkV3NdczRdqNSA5gq2o/view?usp=sharing
> 
> (I hope that if you view those on a retina display, you will still see the 
> blockier appearance of my non-retina laptop screen. )
> 
> You can see the shaved “o” characters there, but I’m just talking about the 
> colors now. Is that normal? To me the JavaFX text looks significantly worse 
> than the native app text [1]. In the native app, I don’t see any color, even 
> looking real close. The JavaFX seems like it went overboard on something. 
> 
> Rob
> 
> [1] It’s a screenshot from Pages, but they all look the same.
> 
>> macOS has been moving away from it but people with non-retina displays have 
>> lamented its passing.
>> 
>> https://osxdaily.com/2018/09/26/fix-blurry-thin-fonts-text-macos-mojave/
>> 
>> FX still requests LCD tho'. Although it is not clear how long macOS will 
>> continue to provide it.
> 

Re: RFR: 8208169: can not print selected pages of web page

[Phil Race]

On Fri, 15 May 2020 16:36:29 GMT, Bhawesh Choudhary 
 wrote:

> Print function of WebEngine.java ignores page range setting and prints given 
> number of pages starting from first page,
> which is the root cause of this issue. To fix it, put check for page ranges 
> and if it available, use it for printing
> pages otherwise print all pages as usual.

looks ok. Approval waiting on a test.

-

PR: https://git.openjdk.java.net/jfx/pull/222

Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight

[Phil Race]

On Fri, 24 Apr 2020 01:50:11 GMT, Kevin Rushforth  wrote:

>> I think most of those are good suggestions going forward. As for the 
>> performance drop, the only place we've seen it so
>> far is on graphics accelerators that are a few years old by now. Integrated 
>> graphics chipsets (such as Intel HD) either
>> old or new seem largely unaffected by the shader changes. What we are 
>> missing is performance metrics from newer
>> graphics accelerators on Mac and Windows.  Even with the performance drop on 
>> older graphics devices, I'm leaning
>> towards not having the shaders to be shaders to be doubled, since this is an 
>> artificial stress test with huge quads. If
>> we could get performance data from a couple more recent graphics 
>> accelerators that would be best.
>
> Here is a slightly modified test program. It fixes a compilation error in the 
> previous, and also adds a system property
> to set the number of quads:
> It creates 200 quads by default. If you need to increase this or decrease it 
> to get something in the ~ 10 fps range you
> can do that with `-DnumQuads=`.
> [pointlighttest.zip](https://github.com/openjdk/jfx/files/4526179/pointlighttest.zip)

@kevinrushforth
Member
kevinrushforth commented Apr 18, 2020

I think most of those are good suggestions going forward. As for the 
performance drop, the only place we've seen it so
far is on graphics accelerators that are a few years old by now.

So 50% drop on a 2015 macbook pro is OK ? Do we have numbers on recent macbook 
pros ?

-

PR: https://git.openjdk.java.net/jfx/pull/43

Re: RFR: 8191758: Match WebKit's font weight rendering with JavaFX

[Phil Race]

On Thu, 16 Apr 2020 12:40:24 GMT, Kevin Rushforth  wrote:

>> As per JavaFx 700 font weight is considered to be bold but webkit is using 
>> 600 font weight for text to become bold. to
>> fix issue, use boldWeightValue() function which uses 700 font weight rather 
>> than isFontWeightBold() which compare
>> against 600 font weight.
>
> Can you add a unit test to go along with this fix?

Per the opentype spec, 700 is bold. 600 is semi-bold
https://docs.microsoft.com/en-us/typography/opentype/spec/os2#usweightclass

CSS agrees : https://developer.mozilla.org/en-US/docs/Web/CSS/font-weight

So are you saying webkit has been using bold at a lower weight than these specs 
suggest ?
I see the logic all comes from 
Source/WebCore/platform/graphics/FontSelectionAlgorithm.h

I suppose the existing code thinks that if we have reached what that file calls 
the bold threshold of 600 then we
should use bold. It isn't necessarily "wrong" but I think I agree that it is 
more important to be consistent with the
rest of Java FX ... which I believe is the point of this change ?

-

PR: https://git.openjdk.java.net/jfx/pull/180

Re: RFR: 8236685: [macOs] Remove obsolete file dialog subclasses

[Phil Race]

On Wed, 4 Mar 2020 20:27:46 GMT, Kevin Rushforth  wrote:

> This is a follow-on to 
> [JDK-8234474](https://bugs.openjdk.java.net/browse/JDK-8234474).
> 
> This fix removes the custom subclasses of NSSavePanel and NSOpenPanel that 
> are optionally used by the glass implementation of file open, directory open, 
> and file save. These subclasses were originally added to provide support for 
> keyboard shortcuts for Copy (CMD-C), Cut (CMD-X), and Paste (CMD-V) 
> operations that the standard Apple dialogs do not support directly; 
> applications can add their own support, but JavaFX is a library not an 
> application.
> 
> Note that as of macOS 10.15, all file dialogs on Mac are run in a spearate 
> process. Attempts to subclass NSSavePanel and NSOpenPanel are no longer 
> supported. They are either ineffective (silently ignored) or else crash the 
> application. As a result of the crashes that were happening in some 
> environments, the fix for 
> [JDK-8234474](https://bugs.openjdk.java.net/browse/JDK-8234474) uses the 
> NSSavePanel and NSOpenPanel base classes directly when running on macOS 10.15 
> or later. The proposed fix for this follow-on issue, 
> [JDK-8236685](https://bugs.openjdk.java.net/browse/JDK-8236685), will use 
> NSSavePanel and NSOpenPanel directly on all versions of macOS.
> 
> Note that Copy, Cut, and Paste functionality are available with the menu 
> before and after this fix. This only impacts the keyboard shortcuts.

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jfx/pull/135

Re: [Integrated] RFR: 8237782: Only read advances up to the minimum of the numHorMetrics or…

[Phil Race]

Changeset: 95bf2c00
Author:Phil Race 
Date:  2020-01-31 18:22:52 +
URL:   https://git.openjdk.java.net/jfx/commit/95bf2c00

8237782: Only read advances up to the minimum of the numHorMetrics or the 
available font data.

Reviewed-by: kcr

! modules/javafx.graphics/src/main/java/com/sun/javafx/font/PrismFontFile.java

Re: [Integrated] RFR: 8237833: Check glyph size before adding to glyph texture cache

[Phil Race]

Changeset: d05e8fc4
Author:Phil Race 
Date:  2020-01-31 18:21:51 +
URL:   https://git.openjdk.java.net/jfx/commit/d05e8fc4

8237833: Check glyph size before adding to glyph texture cache

Reviewed-by: kcr

! modules/javafx.graphics/src/main/java/com/sun/prism/impl/GlyphCache.java

Re: RFR: 8231513: JavaFX cause Keystroke Receiving prompt on MacOS 10.15 (Catalina)

[Phil Race]

On Thu, 30 Jan 2020 23:07:53 GMT, Kevin Rushforth  wrote:

> This is a fix for 
> [JDK-8231513](https://bugs.openjdk.java.net/browse/JDK-8231513) to disable 
> the use of `CGEventTap` when running on macOS 10.15 or later.
> 
> The effect of this bug is that a scary dialog is shown for all users the 
> first time they run a JavaFX application and move the mouse is moved into the 
> JavaFX window. It also is reported to block apps from being accepted in the 
> Apple store.
> 
> This bug is caused by a change in macOS 10.15 to require additional 
> permissions for using CGEventTap, which JavaFX uses to track touch events.
> 
> The suggested replacement API, 
> `NSEvent::addLocalMonitorForEventsMatchingMask`, works just differently 
> enough (it tracks events delivered to a specific view, whereas the current 
> code is implemented using a global monitor and a global set of touch points), 
> that it would be too risky to change it this late in the release.
> 
> For openjfx14, I am proposing to disable touch events completely if running 
> on macOS 10.15 (or later). This will disable the tracking of native touch 
> events, but those events are not used by default on macOS anyway. For Mac 
> systems with a trackpad we instead rely on macOS to do the gesture 
> recognition by default, and this fix does not intefere with that 
> functionality.
> 
> I have verified that this avoids the dialog on macOS 10.15 and that the 
> HelloGestures program still runs correctly and still recognizes trackpad 
> gestures such as zoom and rotate. I also verified that the changes don't 
> affect macOS 10.14 or earlier (the Event Tap code is still enabled on those 
> older OS versions).
> 
> See [this 
> thread](https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024876.html)
>  on openjfx-dev for more discussion.

Looks OK to me.

-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/102

RFR: 8237833: Check glyph size before adding to glyph texture cache

[Phil Race]

Check if the glyph will fit before trying to cache it.

-

Commits:
 - fbfb2473: 8237833: Check glyph size before adding to glyph texture cache

Changes: https://git.openjdk.java.net/jfx/pull/96/files
 Webrev: https://webrevs.openjdk.java.net/jfx/96/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8237833
  Stats: 9 lines in 1 file changed: 7 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jfx/pull/96.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/96/head:pull/96

PR: https://git.openjdk.java.net/jfx/pull/96

RFR: 8237782: Only read advances up to the minimum of the numHorMetrics or…

[Phil Race]

… the available font data.

-

Commits:
 - 059ec788: 8237782: Only read advances up to the minimum of the numHorMetrics 
or the available font data.

Changes: https://git.openjdk.java.net/jfx/pull/95/files
 Webrev: https://webrevs.openjdk.java.net/jfx/95/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8237782
  Stats: 18 lines in 1 file changed: 18 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/95.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/95/head:pull/95

PR: https://git.openjdk.java.net/jfx/pull/95

Re: RFR: 8237823: Mark TextTest.testTabSize as unstable

[Phil Race]

On Fri, 24 Jan 2020 15:12:50 GMT, Kevin Rushforth  wrote:

> Fix for [JDK-8237823](https://bugs.openjdk.java.net/browse/JDK-8237823).
> 
> The javafx.graphics unit test `TextTest.testTabSize` fails intermittently -- 
> see [JDK-8236728](https://bugs.openjdk.java.net/browse/JDK-8236728).
> 
> This PR marks `TextTest.testTabSize` as unstable, meaning it will not be run 
> by default. It will be run only when running gradle with the 
> `-PUNSTABLE_TEST=true` flag.
> 
> As noted in the bug report, I see this about 20% of the time in our nightly 
> and CI builds. Given that we are getting late in the cycle for openjfx14, we 
> need to mark the test as unstable until a fix can be found.
> 
> NOTE: this is targeted to jfx14.

Marked as reviewed by prr (Reviewer).

-

PR: https://git.openjdk.java.net/jfx/pull/94

Re: [Rev 01] RFR: 8234474: [macos 10.15] Crash in file dialog in sandbox mode

[Phil Race]

On Mon, 6 Jan 2020 21:01:25 GMT, Kevin Rushforth  wrote:

>> This PR is a fix for 
>> [JDK-8234474](https://bugs.openjdk.java.net/browse/JDK-8234474), a crash in 
>> the code that shows a file open or save dialog.
>> 
>> In order to provide additional support for Copy (CMD-C), Cut (CMD-X), and 
>> Paste (CMD-V), the Glass implementation for displaying a file open or save 
>> dialog subclasses NSSavePanel or NSOpenPanel to add this support. When the 
>> application is running in sandboxed mode, the dialogs are shown 
>> out-of-process by the "powerbox". In this mode, attempting to use our 
>> subclass results in a security exception. Previously, we added code to 
>> detect whether we were running in a sandbox as a fix for 
>> [JDK-8092977](https://bugs.openjdk.java.net/browse/JDK-8092977); we now use 
>> NSSavePanel or NSOpenPanel directly when in sandboxed mode.
>> 
>> Starting with macOS 10.15 (Catalina) Apple always displays file dialogs 
>> out-of-process via powerbox, so our use of a subclass is ineffective. 
>> Further, we have reports of some cases where we crash even though our 
>> sandbox detection code doesn't indicate that we are running in a sandbox.
>> 
>> Since there is no point in trying to use our subclasses on macOS 10.15 or 
>> later, I propose to fix this bug by changing the logic so that we use 
>> NSSavePanel or NSOpenPanel directly in either of the following conditions:
>> 
>> 1) the app is running in sandbox mode
>> OR
>> 2) The platform is macOS 10.15 or later
> 
> The pull request has been updated with 1 additional commit.



-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/70

Re: [Rev 01] RFR: 8234474: [macos 10.15] Crash in file dialog in sandbox mode

[Phil Race]

On Mon, 6 Jan 2020 18:11:29 GMT, Kevin Rushforth  wrote:

>> Well then you'll just have to come back to it. If its not critical I'd 
>> remove it now so that FX 15 (or is this for 14 ?) behaves consistently
> 
> This is targeted for JavaFX 14, which is another reason to go with a more 
> targeted (safer) fix, since we are almost at RDP1. For JavaFX 15 it does seem 
> best to just remove the functionality entirely.

Ok, fair enough for 14 ...

-

PR: https://git.openjdk.java.net/jfx/pull/70

Re: [Rev 01] RFR: 8234474: [macos 10.15] Crash in file dialog in sandbox mode

[Phil Race]

On Mon, 6 Jan 2020 16:24:33 GMT, Ambarish Rapte  wrote:

>> The pull request has been updated with 1 additional commit.
> 
> Looks good to me, Verified that all tests pass on Mojave 10.14.6

Based on my reading of https://bugs.openjdk.java.net/browse/JDK-8092977, this 
means the support for edit operations in these dialogs is on its way out ... 
now supportable only in non-sandboxed apps on the older macOS releases. Is 
there a serious practical consequence of this, or was the editing just a 
convenience ? There must have been some resason we went to the trouble. However 
based on this being "on its way out" why try to prolong it ? Just drop the 
subclasses now.

-

PR: https://git.openjdk.java.net/jfx/pull/70

Re: [Rev 04] RFR: 8130738: Add tabSize property to Text and TextFlow

[Phil Race]

On Sat, 21 Dec 2019 18:35:26 GMT, Scott Palmer  wrote:

>> I'm not sure if I'me supposed to try to integrate now that I've made that 10 
>> -> 0 change, or if the new change resets the need for review... Also, note 
>> that the link in the bot msg for "project specific requirements" is giving 
>> me a 404 error.
> 
> Link problem appears to just be a missing slash: 
> https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md

> Link problem appears to just be a missing slash: 
> https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md

Seems this was fixed yesterday : https://github.com/openjdk/skara/pull/344

-

PR: https://git.openjdk.java.net/jfx/pull/32

Re: [Rev 05] RFR: 8130738: Add tabSize property to Text and TextFlow

[Phil Race]

On Fri, 20 Dec 2019 22:18:29 GMT, Scott Palmer  wrote:

>> Added tabSize property to Text and TextFlow and -fx-tab-size CSS attribute 
>> to both.  TextFlow's tab size overrides that of contained Text nodes.
> 
> The pull request has been updated with 1 additional commit.



-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/32

Re: Skara - bot sending can-be-integrated message prematurely?

[Phil Race]

It would have to allow anyone who has reviewer status to add that comment.
Not just the author since if they knew we would have less need for it.

-Phil.

> On Dec 18, 2019, at 11:31 AM, Kevin Rushforth  
> wrote:
> 
> That's an interesting idea. It would, of course, need to disallow reducing 
> the number below the minimum specified in .jcheck/conf (e.g., we wouldn't 
> allow "/reviewers 0").
> 
> -- Kevin
> 
> 
>> On 12/18/2019 10:36 AM, Nir Lisker wrote:
>>> The client libraries in the OpenJDK do as a default rule, excusing simple 
>>> fixes.
>> 
>> Then maybe it would be helpful to have a "/reviewers n" command that will 
>> tell the bot how many reviewers are needed. It's less convoluted than the 
>> CSR tracking and basically replaces the comment a reviewer would make anyway 
>> to inform the PR author how many reviewers they should wait for. Extending 
>> the bot to look for n reviewers instead of the current 1 should not be far 
>> fetched.
>> 
>>  
>>> 
>> 
>>> On Wed, Dec 18, 2019 at 4:02 AM Philip Race  wrote:
>>> 
>>> 
>>> On 12/16/19, 4:14 PM, Nir Lisker wrote:
>>> > Do other projects also have multi-reviewers requirements?
>>> 
>>> The client libraries in the OpenJDK do as a default rule, excusing 
>>> simple fixes.
>>> 
>>> >
>>> > I would think that at least for a CSR, which is OpenJDK-wide, a request
>>> > could be put in with the Skara to track it. A Reviewer (or Committer?)
>>> > could invoke a /csr command which will require the PR author to provide a
>>> > link to the CSR, and check that it was approved as part of the patch
>>> > approval process.
>>> 
>>> I think that if there is a CSR sub-task in JBS skara can key off whether 
>>> that is approved.
>>> This does mean skara needs to probe JBS but SFAIK its doing that a 
>>> hundred times
>>> a minute anyway.
>>> 
>>> -phil.
>>> 
>>> >
>>> > Not sure how complicated it would be to implement.
>>> >
>>> > - Nir
>>> >
>>> > On Mon, Dec 16, 2019 at 5:39 PM Kevin 
>>> > Rushforth
>>> > wrote:
>>> >
>>> >> That's a good question about whether we can ask for a slight rewording
>>> >> of the Skara bot message to indicate that there might be other things to
>>> >> check before "/integrate". I'll raise that question with the Skara team.
>>> >>
>>> >> As an aside, I noticed the other day that you typed "/integrate" after a
>>> >> single review, but in that case, there was no clear indication from Ajit
>>> >> (the first reviewer and the sponsor) that a second review was required,
>>> >> so I didn't comment on it.
>>> >>
>>> >> -- Kevin
>>> >>
>>> >>
>>> >> On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:
>>> >>> Kevin,
>>> >>>
>>> >>> thanks for the clarification :) My bad that I didn't re-read the
>>> >>> contrib.md. But then, who does? The lazy like myself do it
>>> >>> occasionally only (down to once and then forget about it)
>>> >>>
>>> >>> Maybe the bot message can be improved? With some indication that its
>>> >>> (the bot's) knowledge about review requirements is limited, so would
>>> >>> require a careful check by the contributor before actually post the
>>> >>> /integrate comment? Actually, I think I goofed the other day, was
>>> >>> safed only by Ajit who waited for the 2nd review before his /sponsor.
>>> >>>
>>> >>> -- Jeanette
>>> >>>
>>> >>> Zitat von Kevin Rushforth:
>>> >>>
>>>  I added a comment to the two PRs in question, but it bears discussion
>>>  here.
>>> 
>>>  The Skara bot can't know whether all criteria have been met. It
>>>  can't, for example, know whether there are outstanding comments from
>>>  some reviewers that need to be addressed. Nor does it know which PRs
>>>  need two reviewers (or sometimes a third if there is a specific
>>>  person we would like to review it), which ones need a CSR, etc.
>>> 
>>>  So having it state authoritatively that the PR is ready to integrate
>>>  is a bit misleading. This is documented in the Code Review section of
>>>  the CONTRIBUTING [1] doc:
>>> 
>>> > NOTE: while the Skara tooling will indicate that the PR is ready to
>>> > integrate once the first reviewer with a "Reviewer" role in the
>>> > project has approved it, this may or may not be sufficient depending
>>> > on the type of fix. For example, you must wait for a second approval
>>> > for enhancements or high-impact bug fixes.
>>>  If anyone can think of a way to improve this and make it more clear,
>>>  that would be helpful.
>>> 
>>>  -- Kevin
>>> 
>>>  [1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md
>>> 
>>> 
>>>  On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:
>>> > Looks like it assumes a pull request as properly reviewed as soon as
>>> > it gets a single approve - independent on how many reviewers are
>>> > required, see f.i.
>>> >
>>> > https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
>>> > https://github.com/openjdk/jfx/pull/6#issuecomment-566028296
>>> 

Re: [Rev 03] RFR: 8130738: Add tabSize property to Text and TextFlow

[Phil Race]

On Thu, 12 Dec 2019 22:02:53 GMT, Kevin Rushforth  wrote:

>> modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1273:
>> 
>>> 1272: /**
>>> 1273:  * The size of a tab stop in spaces.
>>> 1274:  * Values less than 1 are treated as 1.
>> 
>> "tab stop" seems to be an uncommon term. Better are "horizontal tab" (used 
>> in the 
>> [JLS](https://docs.oracle.com/javase/specs/jls/se13/html/jls-3.html#jls-3.10.6)),
>>  "tab character" (used in 
>> [`Pattern`](https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/util/regex/Pattern.html),
>>  "horizontal tabulation" or the like.
> 
> The terms "tab character" or "horizontal tab" refer to the ASCII tab 
> character itself. Since a tab character isn't a fixed number of spaces, 
> changing it to "size of a tab character" could be misleading. I'd be fine 
> with another alternative, though, if someone could come up with a better one.

We are referring to the character here aren't we ? ie the actual character and 
the rest of it is about how it renders.
Paraphrasing the java doc it says :
If you display  it will display as N instances of 

-

PR: https://git.openjdk.java.net/jfx/pull/32

Re: [Integrated] RFR: 8234916: [macos 10.15] Garbled text running with native-image

[Phil Race]

Changeset: 4ddf5542
Author:Jose Pereda 
Committer: Phil Race 
Date:  2019-12-05 20:13:53 +
URL:   https://git.openjdk.java.net/jfx/commit/4ddf5542

8234916: [macos 10.15] Garbled text running with native-image

Reviewed-by: prr

! modules/javafx.graphics/src/main/native-font/coretext.c

Re: [Approved] RFR: 8234916: [macos 10.15] Garbled text running with native-image

[Phil Race]

On Wed, 27 Nov 2019 17:06:13 GMT, Jose Pereda  wrote:

> Running on MacOS Catalina, when doing static builds of `libjavafx_font.a` and 
> linking against this in a JavaFX app compiled with GraalVM native-image, if 
> default fonts are used, the rendered text is garbled, and a warning message 
> is printed: 
> 
> CoreText note: Client requested name ".SFUI-Regular", it will get 
> TimesNewRomanPSMT rather than the intended font. All system UI font access 
> should be through proper APIs such as CTFontCreateUIFontForLanguage() or 
> +[UIFont systemFontOfSize:]. 
> CoreText note: Set a breakpoint on CTFontLogSystemFontNameRequest to debug. 
> 
> On Mac OS, when a map with all the regular fonts is created, we also add two 
> system fonts: [System 
> regular](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-font/MacFontFinder.c#L178),
>  and [System 
> bold](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-font/MacFontFinder.c#L187).
>  These will be the default fonts to be used if the project doesn't specify a 
> font.
> 
> On runtime, when a font is 
> [created](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTFontStrike.java#L90),
>  `CTFontCreateWithName` is 
> [used](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-font/coretext.c#L391),
>  which according to Apple guidelines should be used only for the regular 
> fonts (those that are not system fonts). In fact the above warning doesn't 
> happen when creating any of those fonts.
> 
> Following the warning message, one of the options to create system fonts is 
> to use `CTFontCreateUIFontForLanguage`, which according to the documentation 
> in `CTFont.h`: 
>> Returns the special UI font for the given language and UI type.
> 
> and matches what we already do in `MacFontFinder` to create such fonts. (The 
> other option will require the use of UIKit.)
> 
> 
> This PR modifies `CTFontCreateWithName` in `coretext.c` to detect if the font 
> is a system font in the first place, else use the same existing mechanism.
> 
> As an aside, the constants `kCTFontSystemFontType` and 
> `kCTFontEmphasizedSystemFontType` are deprecated, and this PR uses now the 
> new ones, but in both cases their int value is the same (2 and 3, in that 
> order). As a follow-up PR, we could replace these deprecated constants.
> 
> 
> 
> Commits:
>  - 693dfa78: Use CTFontCreateUIFontForLanguage for system fonts
> 
> Changes: https://git.openjdk.java.net/jfx/pull/55/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/55/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8234916
>   Stats: 9 lines in 1 file changed: 8 ins; 0 del; 1 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/55.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/55/head:pull/55

Approved by prr (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/55

Re: [Rev 02] RFR: 8130738: TextFlow's tab width is static

[Phil Race]

On Tue, 26 Nov 2019 17:26:33 GMT, Scott Palmer  wrote:

> The pull request has been updated with a complete new set of changes 
> (possibly due to a rebase).
> 
> 
> 
> Commits:
>  - 254c40de: Merge remote-tracking branch 'upstream/master'
>  - a670c4f8: 8130738: TextFlow's tab width is static
>  - 68d221c7: 8130738: TextFlow's tab width is static
> 
> Changes: https://git.openjdk.java.net/jfx/pull/32/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/32/webrev.02
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8130738
>   Stats: 325 lines in 8 files changed: 261 ins; 0 del; 64 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/32.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/32/head:pull/32

Why do we have a ton of extraneous white space changes in the Stylesheet 
Handling sections of Text.java and TextFlow.java ?



PR: https://git.openjdk.java.net/jfx/pull/32

Re: [Approved] RFR: 8234704: Fix attribution in libxslt.md

[Phil Race]

On Sat, 23 Nov 2019 16:40:11 GMT, Kevin Rushforth  wrote:

> We discovered three minor differences between the `libxslt.md` file and the 
> libxslt-1.1.34 source bundle:
> 
> 1. The copyright years didn't match the source
> 
> 2. The following line was missing:
> Licence for libxslt except libexslt
> 
> 3. The terms currently lists `BLFS` rather than `DANIEL VEILLARD` in one 
> place.
> 
> Note to reviewers:
> 
> I pushed this in two commits as an aid to reviewing it (as with any other PR, 
> it will be squashed during integration). The first commit just fixes the 
> above three issues without any reformatting of the lines. The second commit 
> has no changes in the content itself, but re-wraps the lines to match the 
> source.
> 
> 
> 
> Commits:
>  - 07be5f50: Reformat to match format of original source -- no changes in 
> content
>  - 68adb259: 8234704: Fix attribution in libxslt.md
> 
> Changes: https://git.openjdk.java.net/jfx/pull/52/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/52/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8234704
>   Stats: 20 lines in 1 file changed: 2 ins; 0 del; 18 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/52.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/52/head:pull/52

Approved by prr (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/52

Re: RFR: 8234704: Fix attribution in libxslt.md

[Phil Race]

On Sat, 23 Nov 2019 16:40:11 GMT, Kevin Rushforth  wrote:

> We discovered three minor differences between the `libxslt.md` file and the 
> libxslt-1.1.34 source bundle:
> 
> 1. The copyright years didn't match the source
> 
> 2. The following line was missing:
> Licence for libxslt except libexslt
> 
> 3. The terms currently lists `BLFS` rather than `DANIEL VEILLARD` in one 
> place.
> 
> Note to reviewers:
> 
> I pushed this in two commits as an aid to reviewing it (as with any other PR, 
> it will be squashed during integration). The first commit just fixes the 
> above three issues without any reformatting of the lines. The second commit 
> has no changes in the content itself, but re-wraps the lines to match the 
> source.
> 
> 
> 
> Commits:
>  - 07be5f50: Reformat to match format of original source -- no changes in 
> content
>  - 68adb259: 8234704: Fix attribution in libxslt.md
> 
> Changes: https://git.openjdk.java.net/jfx/pull/52/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/52/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8234704
>   Stats: 20 lines in 1 file changed: 2 ins; 0 del; 18 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/52.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/52/head:pull/52

"THE DANIEL VEILLARD"

The one and only ? Was THE supposed to be here ?



Changes requested by prr (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/52

Re: [Approved] RFR: 8232064: Switch FX build to use JDK 13.0.1 as boot JDK

[Phil Race]

On Fri, 22 Nov 2019 21:13:05 GMT, Kevin Rushforth  wrote:

> Now that we have switched to using gradle 6 we can switch to using JDK 13 as 
> the boot JDK for JavaFX 14 builds. The latest JDK 13 update release is JDK 
> 13.0.1.
> 
> This will not change the minimum JDK version needed to build or run JavaFX, 
> which remains at 11. We will continue to generate class files using `-source 
> 11 -target 11`.
> 
> 
> 
> Commits:
>  - 741414f1: 8232064: Switch FX build to use JDK 13.0.1 as boot JDK
> 
> Changes: https://git.openjdk.java.net/jfx/pull/51/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/51/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8232064
>   Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/51.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/51/head:pull/51

Approved by prr (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/51

Re: [Approved] RFR: 8233421: Upgrade to Visual Studio 2017 version 15.9.16

[Phil Race]

On Thu, 14 Nov 2019 23:31:34 GMT, Kevin Rushforth  wrote:

> [JDK-8233421](https://bugs.openjdk.java.net/browse/JDK-8233421)
> 
> This bumps the windows compiler version to VS2017 version 15.9.16 to match 
> JDK 14. I have run a full build and test, including WebKit and media.
> 
> 
> 
> Commits:
>  - aab19fa8: 8233421: Upgrade to Visual Studio 2017 version 15.9.16
> 
> Changes: https://git.openjdk.java.net/jfx/pull/40/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/40/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8233421
>   Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/40.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/40/head:pull/40

Approved by prr (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/40

Re: [Approved] RFR: 8232210: Update Mesa 3-D Headers to version 19.2.1

[Phil Race]

On Tue, 29 Oct 2019 23:05:44 GMT, Kevin Rushforth  wrote:

> This PR updates the header files we use the build the OpenGL ES2 pipeline to 
> Mesa 19.2.1. See [this review 
> thread](https://mail.openjdk.java.net/pipermail/2d-dev/2019-October/010372.html)
>  for the equivalent change that is under review for Java2D.
> 
> The updates to the `gl.h` and `glx.h` files are large, since we are many, 
> many years behind.
> 
> The `*ext.h` header files were updated fairly recently, so those diffs are 
> not large.
> 
> Previously we used to get the `*ext.h` headers from Khronos, but now we get 
> all the headers from the Mesa project.
> 
> This reduces the number of upstream sources we need to monitor.
> 
> I note that with this update, the `glxext.h` and `wglext.h` files are 
> slightly older in the Mesa bundle than in Khronos, but the differences are 
> not relevant to FX.
> 
> I did a full build and test on Mac and Linux and a sanity build (with 
> `-PINCLUDE_ES2=true`) on Windows. I also verified that the build artifacts 
> are unchanged.
> 
> As with the Java2D change, the licensing terms are the same as before, but 
> since we no longer get files directly from Khronos, the `opengl_fx.md` file 
> is gone and the `mesa3d.md` is updated as required to mention these files.
> 
> 
> 
> Commits:
>  - 7a520adc: 8232210: Update Mesa 3-D Headers to version 19.2.1
> 
> Changes: https://git.openjdk.java.net/jfx/pull/26/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/26/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8232210
>   Stats: 1515 lines in 8 files changed: 1076 ins; 269 del; 170 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/26.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/26/head:pull/26

Approved by prr (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/26

Re: [Approved] RFR: 8232210: Update Mesa 3-D Headers to version 19.2.1

[Phil Race]

On Tue, 29 Oct 2019 23:05:44 GMT, Kevin Rushforth  wrote:

> This PR updates the header files we use the build the OpenGL ES2 pipeline to 
> Mesa 19.2.1. See [this review 
> thread](https://mail.openjdk.java.net/pipermail/2d-dev/2019-October/010372.html)
>  for the equivalent change that is under review for Java2D.
> 
> The updates to the `gl.h` and `glx.h` files are large, since we are many, 
> many years behind.
> 
> The `*ext.h` header files were updated fairly recently, so those diffs are 
> not large.
> 
> Previously we used to get the `*ext.h` headers from Khronos, but now we get 
> all the headers from the Mesa project.
> 
> This reduces the number of upstream sources we need to monitor.
> 
> I note that with this update, the `glxext.h` and `wglext.h` files are 
> slightly older in the Mesa bundle than in Khronos, but the differences are 
> not relevant to FX.
> 
> I did a full build and test on Mac and Linux and a sanity build (with 
> `-PINCLUDE_ES2=true`) on Windows. I also verified that the build artifacts 
> are unchanged.
> 
> As with the Java2D change, the licensing terms are the same as before, but 
> since we no longer get files directly from Khronos, the `opengl_fx.md` file 
> is gone and the `mesa3d.md` is updated as required to mention these files.
> 
> 
> 
> Commits:
>  - 7a520adc: 8232210: Update Mesa 3-D Headers to version 19.2.1
> 
> Changes: https://git.openjdk.java.net/jfx/pull/26/files
>  Webrev: https://webrevs.openjdk.java.net/jfx/26/webrev.00
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8232210
>   Stats: 1515 lines in 8 files changed: 1076 ins; 269 del; 170 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/26.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/26/head:pull/26

Approved by prr (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/26

Re: RFR: 8232210: Update Mesa 3-D Headers to version 19.2.1

[Phil Race]

On Wed, 30 Oct 2019 00:43:08 GMT, Sergey Bylokhov  wrote:

> On Tue, 29 Oct 2019 23:05:46 GMT, Kevin Rushforth  wrote:
> 
>> On Tue, 29 Oct 2019 23:05:44 GMT, Kevin Rushforth  wrote:
>> 
>>> This PR updates the header files we use the build the OpenGL ES2 pipeline 
>>> to Mesa 19.2.1. See [this review 
>>> thread](https://mail.openjdk.java.net/pipermail/2d-dev/2019-October/010372.html)
>>>  for the equivalent change that is under review for Java2D.
>>> 
>>> The updates to the `gl.h` and `glx.h` files are large, since we are many, 
>>> many years behind.
>>> 
>>> The `*ext.h` header files were updated fairly recently, so those diffs are 
>>> not large.
>>> 
>>> Previously we used to get the `*ext.h` headers from Khronos, but now we get 
>>> all the headers from the Mesa project.
>>> 
>>> This reduces the number of upstream sources we need to monitor.
>>> 
>>> I note that with this update, the `glxext.h` and `wglext.h` files are 
>>> slightly older in the Mesa bundle than in Khronos, but the differences are 
>>> not relevant to FX.
>>> 
>>> I did a full build and test on Mac and Linux and a sanity build (with 
>>> `-PINCLUDE_ES2=true`) on Windows. I also verified that the build artifacts 
>>> are unchanged.
>>> 
>>> As with the Java2D change, the licensing terms are the same as before, but 
>>> since we no longer get files directly from Khronos, the `opengl_fx.md` file 
>>> is gone and the `mesa3d.md` is updated as required to mention these files.
>>> 
>>> 
>>> 
>>> Commits:
>>>  - 7a520adc: 8232210: Update Mesa 3-D Headers to version 19.2.1
>>> 
>>> Changes: https://git.openjdk.java.net/jfx/pull/26/files
>>>  Webrev: https://webrevs.openjdk.java.net/jfx/26/webrev.00
>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8232210
>>>   Stats: 1515 lines in 8 files changed: 1076 ins; 269 del; 170 mod
>>>   Patch: https://git.openjdk.java.net/jfx/pull/26.diff
>>>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/26/head:pull/26
>> 
>> Reviewers: @prrace, @arapte, @johanvos
> 
> Not sure but should not the license be GPL+CP in some of these files?

> Not sure but should not the license be GPL+CP in some of these files?

It is not necessary.

PR: https://git.openjdk.java.net/jfx/pull/26

Re: RFR: 8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph

[Phil Race]

On Sat, 26 Oct 2019 11:33:15 GMT, Laurent Bourgès 
 wrote:

> On Fri, 25 Oct 2019 20:18:48 GMT, Phil Race  wrote:
> 
>> I have added an evaluation in 
>> https://bugs.openjdk.java.net/browse/JDK-8207839
>> Please read that for more detail, but basically we are not properly 
>> preventing or
>> handling cases where fontconfig causes us to overflow the byte storage used
>> for a font "slot".
>> 
>> 
>> 
>> Commits:
>>  - b6c19466: Fix whitespace
>>  - e00ef992: 8189092: ArrayIndexOutOfBoundsException on Linux in 
>> getCachedGlyph
>> 
>> Changes: https://git.openjdk.java.net/jfx/pull/24/files
>>  Webrev: https://webrevs.openjdk.java.net/jfx/24/webrev.00
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8189092
>>   Stats: 15 lines in 3 files changed: 12 ins; 0 del; 3 mod
>>   Patch: https://git.openjdk.java.net/jfx/pull/24.diff
>>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/24/head:pull/24
> 
> I had a quick look by curiosity as it is not my field. Explanations really 
> make sense, thanks Phil.
> Bit masks & shifts look good and definitely it will make that code safer.
> However I wonder how to deal with more than 255 fonts (1 byte limit) as it 
> may happen on some systems, even not reallistic.
> Thanks

> However I wonder how to deal with more than 255 fonts (1 byte limit) as it 
> may happen on some systems, even not reallistic.

That would have to be > 255 fonts *per fontconfig font*, and we have additional 
filters
to eliminate fonts that don't add value (at least we do now with fixing the bug 
in that
code in fontpath_linux.c).
We could rework things to use 16 bit for font slot, although that would be a 
bigger change
and require more testing and really this is never going to be needed on Windows 
or Mac,
just some very small number of Linux systems that are not default configs ..

PR: https://git.openjdk.java.net/jfx/pull/24

RFR: 8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph

[Phil Race]

I have added an evaluation in https://bugs.openjdk.java.net/browse/JDK-8207839
Please read that for more detail, but basically we are not properly preventing 
or
handling cases where fontconfig causes us to overflow the byte storage used
for a font "slot".



Commits:
 - b6c19466: Fix whitespace
 - e00ef992: 8189092: ArrayIndexOutOfBoundsException on Linux in getCachedGlyph

Changes: https://git.openjdk.java.net/jfx/pull/24/files
 Webrev: https://webrevs.openjdk.java.net/jfx/24/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8189092
  Stats: 15 lines in 3 files changed: 12 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jfx/pull/24.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/24/head:pull/24

PR: https://git.openjdk.java.net/jfx/pull/24

Re: JDK-8130738 TextFlow's tab width is static

[Phil Race]

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

Re: JDK-8130738 TextFlow's tab width is static

[Phil Race]

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  (or length -which no browsers 
support). For all intents and purposes,  indicates a number of spaces.


-Original Message-
From: openjfx-dev  On Behalf Of
Phil Race
Sent: Thursday, October 17, 2019 2:25 PM
To: Scott Palmer 
Cc: openjfx-dev@openjdk.java.net Mailing 
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%23lengthsdata=02%7C01%7CDavid.Griev
e%40microsoft.com%7Cec03031ecb6c484da76008d7533002ce%7C72f988bf8
6f141a
f91ab2d7cd011db47%7C1%7C0%7C637069337978947508sdata=li0kW
L1C1hNRA

In0C5ehMIHz6WD%2B%2F3F1xgRH%2Fl9Piv4%3Dreserved=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  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 pix

Re: JDK-8130738 TextFlow's tab width is static

[Phil Race]

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

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

Re: JDK-8130738 TextFlow's tab width is static

[Phil Race]

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

Re: JDK-8130738 TextFlow's tab width is static

[Phil Race]

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

Re: JDK-8130738 TextFlow's tab width is static

[Phil Race]

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 


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

My current work is here 
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 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 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 >).  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

Re: [11][13] RFR: Request to backport October 2019 CPU changes

[Phil Race]

Approved.

-phil.

On 10/15/19 9:44 AM, Kevin Rushforth wrote:
> I request approval to backport the changes from the just-released 
April 2019 CPU to 11-dev and 13-dev.


Typo: that should be October 2019 CPU

On 10/15/2019 9:43 AM, Kevin Rushforth wrote:

Hi Johan,

I request approval to backport the changes from the just-released 
April 2019 CPU to 11-dev and 13-dev.


https://cr.openjdk.java.net/~kcr/cpu-1910-sync/11-dev/webrev/
https://cr.openjdk.java.net/~kcr/cpu-1910-sync/13-dev/webrev/

Each is a straight backport (patch applies cleanly) of the one FX fix 
that went into the October CPU.


Thanks.

-- Kevin