Re: RFR: 8214699: Node.getPseudoClassStates must return the same instance on every call

[Kevin Rushforth]

On Thu, 18 Jun 2020 16:30:42 GMT, Ambarish Rapte  wrote:

> Node.getPseudoClassStates() returns a new UnmodifiableObservableSet of 
> PseudoClassState on each call. So in order to
> listen to any changes in this set, user must call the method 
> Node.getPseudoClassStates() only once and keep a strong
> reference to the returned UnmodifiableObservableSet.
>  
> So the fix is that the method Node.getPseudoClassStates() should return the 
> same UnmodifiableObservableSet on every
> call. As the returned set is an UnmodifiableObservableSet, it will not have 
> any impact on it's usage.
>  
> Added a small unit test. and,
> Altered(minor) a test which was modified in
> past(https://github.com/openjdk/jfx/commit/0ac98695a1b11443c342fad4f009d6e03a052522)
> (https://github.com/openjdk/jfx/commit/62323e0a9c5817b33daa262d6914eba0e8d274ff)
>  along with this method and should be
> updated in view of this change.

The fix looks good, given that `FXCollections.unmodifiableObservableSet()` 
wraps an observable set listens to changes
to the backing set. Can you add a test to ensure that the listeners are getting 
called?

modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java line 168:

> 167: assertSame("getPseudoClassStates() should always return the same 
> instance",
> 168: node.getPseudoClassStates(), 
> node.getPseudoClassStates());
> 169: }

To make this more clear, I recommend to store the expected value in a local 
variable and then call assert.

-

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

Re: RFR: 8193800: TreeTableView selection changes on sorting [v5]

[Kevin Rushforth]

On Wed, 17 Jun 2020 12:28:41 GMT, Kevin Rushforth  wrote:

>> Ambarish Rapte has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove the un-required flag
>
> Marked as reviewed by kcr (Lead).

Pending a second reviewer.

@aghaisas or @kleopatra can one of you review it?

-

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

Re: RFR: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts [v3]

[Kevin Rushforth]

On Fri, 19 Jun 2020 15:49:43 GMT, Rony G. Flatscher 
 wrote:

> CSR-update: looks good!

I have moved the CSR to the "proposed" state in preparation for formal review.

-

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

Re: RFR: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts [v3]

[Kevin Rushforth]

On Fri, 19 Jun 2020 15:49:36 GMT, Rony G. Flatscher 
 wrote:

>> modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1769:
>> 
>>> 1768: try {
>>> 1769: if (isCompiled) {
>>> 1770:compiledScript.eval(localBindings);
>> 
>> I think there may be other places you need to set isCompiled (it isn't set 
>> in the first couple of methods where you
>> compile scripts). Can you check this?
>
> isCompiled gets set explicitly to false at object creation time. It only will 
> be changed to true, if the script was
> successfully compiled.

I was (mistakenly) thinking of the `processStartElement` and 
`processEndElement` methods of the `ScriptElement` class,
but that's a different class, and they use a local `compiledScript` variable. 
So this is fine.

-

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

Re: RFR: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts [v3]

[Rony G . Flatscher]

On Fri, 19 Jun 2020 00:04:37 GMT, Kevin Rushforth  wrote:

>> Rony G. Flatscher has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now
>> contains 27 commits:
>>  - Updates to meet Kevin's comment in PR 192 as of May 27th.
>>  - Merge remote-tracking branch 'upstream/master' into 
>> scriptCompilablePIcompileFallback
>>  - Reword the compile processing instructions (replace 'Hint' with 'Note:', 
>> adjust text to context), remove spurious empty
>>line in script example code.
>>  - Document the compile processing instruction for scripts.
>>  - Add missing language processing instruction.
>>  - Correct typo, replace tabs, remove trailing blanks.
>>  - Make sure we test the default behaviour to compile script by leaving out 
>> the compile PI.
>>  - Revert temporary rename of test method.
>>  - Correct ModuleLauncherTest (remove non-existing test), correct formatting.
>>  - Always supply the script's filename in the error message first to further 
>> ease spotting the location of script
>>exceptions.
>>  - ... and 17 more: 
>> https://git.openjdk.java.net/jfx/compare/6bd0e22d...7ef1bd68
>
> I reviewed the implementation, and I think this is close to being ready, but 
> I have a couple questions. I also still
> need to review the test and run it, but that will be later.
> I also noticed a few places with minor formatting issues -- mostly missing 
> spaces and extra blank lines added to some
> files, but also `else {` when it should be `} else {`. I'll wait until the 
> substantive part of the review is done to
> note them all (so if you can fix them ahead of time, that would be good).

@CSR-update: looks good!

> modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1769:
> 
>> 1768: try {
>> 1769: if (isCompiled) {
>> 1770:compiledScript.eval(localBindings);
> 
> I think there may be other places you need to set isCompiled (it isn't set in 
> the first couple of methods where you
> compile scripts). Can you check this?

isCompiled gets set explicitly to false at object creation time. It only will 
be changed to true, if the script was
successfully compiled.

-

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

Re: RFR: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts [v4]

[Rony G . Flatscher]

> This PR adds a "compile" process instruction to FXML files with the optional 
> PI data "true" (default) and "false". The
> PI data is turned into a boolean value using "Boolean.parseBoolean(String)".
> This makes it possible to inject the compile PI everywhere in a FXML file and 
> turn on and off compilation of scripts if
> the scripting engine implements the javax.script.Compilable interface. The PR 
> adds the ability for a fallback in case
> compilation of scripts fails, in which case a warning gets issued about this 
> fact and evaluation of the script will be
> done without compilation. Because of the fallback scripts get compiled with 
> this version by default.
> -
> ### Progress
> - [x] Change must not contain extraneous whitespace
> - [x] Commit message must refer to an issue
> - [ ] Change must be properly reviewed
> 
> ### Issue
>  * [JDK-8238080](https://bugs.openjdk.java.net/browse/JDK-8238080): 
> FXMLLoader: if script engines implement
>javax.script.Compilable compile scripts
> 
> 
> ### Download
> `$ git fetch https://git.openjdk.java.net/jfx pull/192/head:pull/192`
> `$ git checkout pull/192`

Rony G. Flatscher has updated the pull request incrementally with one 
additional commit since the last revision:

  Incorporating Kevin's review comments (final int, fix for loop test, correct 
formatting).

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/192/files
  - new: https://git.openjdk.java.net/jfx/pull/192/files/7ef1bd68..f2ea3d0e

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/192/webrev.03
 - incr: https://webrevs.openjdk.java.net/jfx/192/webrev.02-03

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

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

Re: backport requests for 11.0.8

[Kevin Rushforth]

+1

On 6/19/2020 7:32 AM, Johan Vos wrote:

Hi Kevin,

I request approval to backport the following fixes to OpenJFX 11 (11.0.8):

JDK-8238249 :
GetPrimitiveArrayCritical passed with hardcoded FALSE value
JDK-8212034 : Potential
memory leaks in jpegLoader.c in error case
JDK-8241370 : Crash in
JPEGImageLoader after fix for JDK-8212034
JDK-8232811 : Dialog's
preferred size no longer accommodates multi-line strings
JDK-8237782 : Only read
advances up to the minimum of the numHorMetrics or the available font data.
JDK-8237833 : Check glyph
size before adding to glyph texture cache.
JDK-8237944 : webview
native cl "-m32" unknown option for windows 32-bit build
JDK-8189092 :
ArrayIndexOutOfBoundsException on Linux in getCachedGlyph
JDK-8242106 : [macos]
Remove obsolete GlassView2D.m class
JDK-8236685 : [macOs]
Remove obsolete file dialog subclasses

All patches apply clean (if applied in this order).

- Johan

backport requests for 11.0.8

[Johan Vos]

Hi Kevin,

I request approval to backport the following fixes to OpenJFX 11 (11.0.8):

JDK-8238249 :
GetPrimitiveArrayCritical passed with hardcoded FALSE value
JDK-8212034 : Potential
memory leaks in jpegLoader.c in error case
JDK-8241370 : Crash in
JPEGImageLoader after fix for JDK-8212034
JDK-8232811 : Dialog's
preferred size no longer accommodates multi-line strings
JDK-8237782 : Only read
advances up to the minimum of the numHorMetrics or the available font data.
JDK-8237833 : Check glyph
size before adding to glyph texture cache.
JDK-8237944 : webview
native cl "-m32" unknown option for windows 32-bit build
JDK-8189092 :
ArrayIndexOutOfBoundsException on Linux in getCachedGlyph
JDK-8242106 : [macos]
Remove obsolete GlassView2D.m class
JDK-8236685 : [macOs]
Remove obsolete file dialog subclasses

All patches apply clean (if applied in this order).

- Johan

JavaFX HiDPI layout bugs

[Sam Hutchins]

Hi,

I've been doing some investigation into a layout bug in JavaFX on Windows with 
non-integer scaling values. I think it's related to JDK-8199592, and I've put a 
small example that will reproduce these layout bugs at the end of this email. 
The most obvious layout error is truncation of labels in many controls. I've 
used CheckBoxes in this example, but the truncation errors are apparent in any 
labeled control. I suspect the issue affects other controls too (and probably 
the entire layout), but in more subtle ways. With scaling values of 1.25 or 
1.5, the label truncation only happens in the dialog box. At 1.75 the issue is 
apparent in both windows.

I _think_ the issue is a combination of caching and invalid calculations. I 
stepped through the `layoutLabelInArea(x, y, w, h, alignment)` method in 
LabeledSkinBase and found that 3 layout passes take place.

In the first and second passes, all the layout calculations appear to be done 
without taking scaling into account.
The 3rd pass appears to be when a scene is involved, and the snap* methods 
start having an impact. It looks like some values are scaled appropriately, but 
others are still using the pre-scene cached values. In particular, the call to 
`leftLabelPadding()` returns 5 for the first 2 passes, but 5.6 in the 3rd (at 
1.25 scaling). The value of `w` doesn't change though, and that .6 increase in 
padding causes the label to be truncated. When you interact with the CheckBox 
to trigger a 4th layout, the value of `w` is increased by .6, and the layout 
appears to work as expected.

I've hacked some of the caching out of `Parent` by adding a call to 
clearSizeCache() to the start of the `prefWidth(height)` and 
`prefHeight(width)` methods, which forces JavaFX to re-compute everything on 
every call. This resolves the issues for simple layouts at 1.25 and 1.5 scaling 
(but not 1.75).

This doesn't resolve issues in more complicated layouts, however, as many 
subclasses of `Parent` have caching of their own. For example, controls in 
GridPanes will still exhibit this behaviour. I've hacked some caching out of 
that by removing the early return in the `computePrefHeights(widths)` and 
`computePrefWidths(heights)` methods, which resolves it for the GridPane case. 
I imagine there are many other instances where these cached values aren't 
correctly invalidated.

I think there's also a fundamental issue with the layout calculations with 
non-integer scaling, as the layout is _always_ wrong at 1.75. I suspect that 
the layout calculations are always wrong, but the error at lower scaling values 
isn't enough to cause visible issues after pixel snapping.

I'm not really sure where to go from here, as I'm not at all familiar with how 
and when JavaFX invalidates its layout calculations. If anyone can point me at 
some other threads to pull, I'd be grateful.

Thanks,

Sam


Code to reproduce:
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Alert;
import javafx.scene.control.Button;
import javafx.scene.control.ButtonType;
import javafx.scene.control.CheckBox;
import javafx.scene.layout.GridPane;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;
public class LayoutBug
{
public static void main(String[] args)
{
System.setProperty("glass.win.uiScale", "1.5");
Application.launch(MainWindow.class);
}
public static class MainWindow extends Application
{
public void start(final Stage primaryStage)
{
final var button = new Button("Show Dialog");
button.setOnAction(e ->
{
final var alert = new Alert(Alert.AlertType.NONE);
alert.initOwner(primaryStage);
alert.getButtonTypes().add(ButtonType.OK);
alert.getDialogPane().setContent(createTestUi());
alert.show();
});
final var root = createTestUi();
root.getChildren().add(button);
primaryStage.setScene(new Scene(root, 400, 600));
primaryStage.show();
}
private VBox createTestUi()
{
final var gridPane = new GridPane();
gridPane.addRow(0, new CheckBox("Checkbox in gridpane"));
return new VBox(10,
gridPane,
new CheckBox("Checkbox outside gridpane"));
}
}
}

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v12]

[Frederic Thevenet]

> Issue JDK-8088198, where an exception would be thrown when trying to capture 
> a snapshot whose final dimensions would be
> larger than the running platform's maximum supported texture size, was 
> addressed in openjfx14. The fix, based around
> the idea of capturing as many tiles of the maximum possible size and 
> re-compositing the final snapshot out of these, is
> currently only attempted after the original, non-tiled, strategy has already 
> failed. This was decided to avoid any risk
> of regressions, either in terms of performances and correctness, while still 
> offering some relief to the original
> issue.  This follow-on issue aims to propose a fix to the original issue, 
> that is able to correctly decide on the best
> snapshot strategy (tiled or not) to adopt before applying it and ensure best 
> performances possible when tiling is
> necessary while still introducing no regressions compared to the original 
> solution.

Frederic Thevenet has updated the pull request incrementally with one 
additional commit since the last revision:

  Crawling pixels in writableImage with parallel stream is a bad idea.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/112/files
  - new: https://git.openjdk.java.net/jfx/pull/112/files/4752d83e..a01aaa20

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/112/webrev.11
 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.10-11

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

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

Re: OpenJFX: Follow HiDPI Scaling Settings in Fedora Workstation 32 (Gnome)

[wurstsemmel]

Dear Kevin, Dear Phil,

thank you for your quick responses. I submitted a bug report (internal
review ID: 9065523). Within the description I listed JDK-8137050 and
additionally bug reports from Cryptomator which describe the issue also
in Ubuntu 20.04 LTS and pop!OS.

Kind regards and thanks,

wurstsemmel

Am Donnerstag, den 18.06.2020, 09:37 -0700 schrieb Kevin Rushforth:
> 
> On 6/18/2020 9:26 AM, Philip Race wrote:
> > Although we only support integer scaling on Linux you have 200%
> > which 
> > should work.
> > So sounds like it could be a breaking change in Fedora, where it 
> > passes around / defines
> > the scale in some new setting and doesn't set the old ones needed
> > for 
> > compatibility.
> 
> That could be.
> 
> > >  (either via gsettings or the GDI_SCALE env variable)
> > 
> > GDK_SCALE, not GDI_SCALE
> 
> Oops. :)  Of course.
> 
> -- Kevin
> 
> 
> > -phil
> > 
> > 
> > 
> > On 6/18/20, 5:12 AM, Kevin Rushforth wrote:
> > > We will need a new bug ID for this (we never reuse a bug ID for
> > > which 
> > > a commit has been pushed). You can file a new bug, mentioning 
> > > JDK-8137050 in the Description, at:
> > > 
> > > https://bugreport.java.com/
> > > 
> > > Question for other Linux users on this list: are any of you using
> > > a 
> > > Linux machine with Hi-DPI scaling? Is it detecting it properly or
> > > do 
> > > you have to set the scaling property (either via gsettings or
> > > the 
> > > GDI_SCALE env variable)  yourself?
> > > 
> > > -- Kevin
> > > 
> > > 
> > > On 6/18/2020 1:51 AM, wurstsem...@mailbox.org wrote:
> > > > Dear OpenJFX Developers,
> > > > 
> > > > on Fedora 32 Workstation (Gnome) the AppImage from Cryptomator
> > > > (cryptomator.org) does not follow the system-wide scale
> > > > settings from
> > > > Gnome Settings.
> > > > 
> > > > *System Setup
> > > > Linux: Fedora Workstation 32 (Gnome)
> > > > Hardware: Dell XPS 13 with HiDPI (200 % scaling)
> > > > Cryptomator version: 1.5.5-x86_64 AppImage
> > > > 
> > > > *Expected Behavior
> > > > App window should scale according to system-wide settings.
> > > > 
> > > > *Actual Behavior
> > > > App window scales at 100 %.
> > > > 
> > > > *Two workarounds exist:
> > > > $gsettings set org.gnome.desktop.interface scaling-factor 2
> > > > $GDK_SCALE=2 ./cryptomator-1.5.5-x86_64.AppImage
> > > > 
> > > > According to the developers from Cryptomator (see
> > > > https://github.com/cryptomator/cryptomator/issues/1242#issuecomment-643095195
> > > >  
> > > > 
> > > > ) this is a bug which was intended to be fixed with
> > > > https://bugs.openjdk.java.net/browse/JDK-8137050. However, the
> > > > bug
> > > > still occurs in OpenJFX 14.0.1 and 11.
> > > > 
> > > > I am happy to help if anything is unclear. Please re-open the
> > > > bug JDK-
> > > > 8137050 and make OpenJFX follow the system-wide scaling
> > > > settings in
> > > > Gnome.
> > > > 
> > > > Thank you!
> > > > 
> > > > wurstsemmel
> > > > 
> > > > 
> > > > 
> > > > 
> > > >