Re: RFR: 8240692: Cleanup of the javafx property objects

[Nir Lisker]

On Fri, 6 Mar 2020 23:12:57 GMT, Kevin Rushforth  wrote:

>> A simple readability cleanup for the files that were changed in #113.
>> 
>> Note that the boolean property has an `instanceof` check that doesn't exist 
>> in the other properties.
>
> I note that this also changes the wrapper property objects from anonymous 
> subclasses of XPropertyBase to SimpleXProperty. This is more than 
> just a readability cleanup. It's probably fine for this case, but that's why 
> I want a second reviewer.

> I note that this also changes the wrapper property objects from anonymous 
> subclasses of XPropertyBase to SimpleXProperty. This is more than 
> just a readability cleanup. It's probably fine for this case, but that's why 
> I want a second reviewer.

Isn't SimpleXProperty exactly made for XPropertyBase with the built-in 
overrides for the bean and the name? When is this substitution not fine?

-

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

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

[Nir Lisker]

On Wed, 5 Feb 2020 13:48:45 GMT, Kevin Rushforth  wrote:

>> We have a few performance tests in apps/performance, but I don't know how up 
>> to date they are. They might give you a
>> starting point on how to measure FPS, but really the harder part is going to 
>> be coming up with a workload -- a scene
>> with a number of Shape3Ds with large triangles (so that they are fill-rate 
>> limited) and 1-3 lights, etc -- that you can
>> use to measure rendering performance without measuring overhead. Typically 
>> you want a scene that is rendering
>> continuously in the < 30 fps range, and more like 10 fps to minimize the 
>> overhead even more.  Before we figure out
>> whether we need to double the number of shaders (which was one of the ideas 
>> that I had as well), we need to know how
>> much of a problem it is. Is it < 2% performance drop on typical cases on a 
>> variety of machines or it is a 25% drop (or
>> more likely somewhere in between). If the perf drop is negligible, then it 
>> isn't worth doubling the shaders. If it is
>> significant, then we probably need to.  If we do need to double the shaders, 
>> I wouldn't do it based on the maxRange
>> being infinite, rather I would do it based on whether attenuation is being 
>> used. That way the existing shaders would be
>> unchanged, while the new shader would deal with both attenuation and the 
>> maxRange test.  Hopefully, there won't be
>> enough of a perf hit to require doubling the shaders, but we need to be able 
>> to measure it.  For functional testing, in
>> addition to the simple API tests, we want to make sure that the basic 
>> rendering is working with and without
>> attenuation. Some sort of visual test where you verify that attenuation is / 
>> isn't happening as well as testing the
>> cutoff. I wouldn't get too fancy with these...just a sanity test.
>
> Looks like the `jcheck` bot removed the `rfr` label because the CSR isn't 
> complete. An incomplete CSR should be treated
> the same way as an insufficient number of reviewers. I filed
> [SKARA-262](https://bugs.openjdk.java.net/browse/SKARA-262) to track this.

@kevinrushforth Can you have a look at the test app? I would like to get this 
moving so we would have time to get the
rest of the lighting enhancements into 15.

-

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

Re: Monocle as a replacement

[Nir Lisker]

I added a note about removing Dprism.order=sw.

On Wed, Mar 25, 2020 at 7:27 PM Tres Finocchiaro 
wrote:

> With the help from a paid fx support channel, we're making headway with
> this.  The main help was disabling the recommended software option.  At
> time of writing this, I recommend this warning/detail is added to the wiki:
> https://wiki.openjdk.java.net/display/OpenJFX/Monocle
>
> Quoting:
>
>
> > *If you are running the desktop build of JavaFX or OpenJFX then your only
> > monocle option is Headless. Desktop JavaFX does not support the
> > javafx.platform system property, but you can select Monocle
> > with:-Dglass.platform=Monocle -Dmonocle.platform=Headless
> -Dprism.order=sw*
>
>
> In or case, removing "*-Dprism.order=sw"* was critical to prevent crashes
> on MacOS and Windows.  We've yet to test on Linux.
>
> Last, since Monocle introduces a virtual desktop size (and we're using this
> desktop for screen captures), we'll be tweaking the
> "*-Dheadless.geometry=*"
> parameter depending on available heap size.  These details are best tracked
> downstream for those interested https://github.com/qzind/tray/pull/586.
>
> Unless there are immediate questions, this will be the last update I
> provide to the mailing list, all further updates will be specific to the
> downstream implementation.
>
> As an aside, we may decide to sponsor the fixing of the underlying crashes
> as well (for example, d2d may not be available on Windows 2016 Core?) but
> at the time of writing this, we're adopting workarounds to make it viable
> on a standard Desktop.
>
> - tres.finocchi...@gmail.com
>
>
> On Fri, Feb 21, 2020 at 5:13 PM Tres Finocchiaro <
> tres.finocchi...@gmail.com>
> wrote:
>
> > Danny,
> >
> > Thanks this information is very valuable.
> >
> > By using the provided patch, I too am able to re-use the Monocle
> framework
> > without suffering this bug.
> >
> > For those visiting this thread (e.g. through archive) at a later time,
> > it's being tracked downstream here:
> > https://github.com/qzind/tray/issues/576
> >
> > So applying it I was able to discover I was running into two separate
> > issues...
> >
> >- The first was the unpredictable buffer behavior you've shared, which
> >seems to be resolved that problem.  I used the recommended code, just
> in a
> >copy of the JavaFX 11 class instead.  Thanks!
> >
> >- The second is a nuance of reusing the WebView and Stage using
> >Monocle, the stage/webview height calculation starts to grow out of
> control
> >(watching the DOM height -- we calculate the natural height and then
> use
> >that for snapshot).  In my case it was growing 300, 900, 2900, 8600
> >eventually crashing somewhere in buffer allocation.  Hard-coding the
> sizing
> >didn't suffer the same fate because it bypasses our attempts to
> calculate
> >the height using JavaScript.  Oddly, creating a fresh WebView each
> time
> >didn't correct the issue either.  I believe the underlying issue stems
> >somewhere from the stage height never resetting back, but attempts to
> do so
> >manually cause other issues, so I'll see if there's a viable
> workaround by
> >doing more renders using Monocle.
> >
> > We already have an open item with Gluon regarding WebView stage sizing,
> so
> > this may be a race condition rearing its ugly head through a different
> > symptom.  We'll continue to work on it separately.
> >
> > Danny, thanks again for the link to the patch.  We'll continue our
> testing.
> >
> > - tres.finocchi...@gmail.com
> >
> >
> > On Wed, Feb 19, 2020 at 2:46 AM Danny Gonzalez <
> > danny.gonza...@screamingfrog.co.uk> wrote:
> >
> >> Hi Tres,
> >>
> >> We also are suffering from this crash when running our TestFX unit
> tests,
> >> particularly on Java 11.
> >> It is due to a concurrency issue between the JavaFX thread and the
> >> QuantumRenderer thread and there is an OpenJDK bug here:
> >> https://bugs.openjdk.java.net/browse/JDK-8201567
> >>
> >> Quoting from this bug report:
> >> “The QuantumRenderer calls the getPixels() method while trying to find a
> >> buffer that's not in use, yet in doing so it can inadvertently modify a
> >> buffer that's in use."
> >>
> >> There is also a related TestFX Bug:
> >> https://github.com/TestFX/Monocle/issues/56
> >>
> >> There is a fix for this issue In the first comment of the JDK-8201567,
> in
> >> a link to GitLab: https://gitlab.com/openjfxepd/jfxpatch/commit/
> >> <
> https://gitlab.com/openjfxepd/jfxpatch/commit/f7c341775e5258e790a049f3fdce4a956ef665c7
> >
> >>
> >> We have used this patch in our local OpenJFX build.
> >>
> >> This has never been made into a pull request however but I believe it
> >> should.
> >>
> >> Danny
> >>
> >> On 17 Feb 2020, at 19:12, Tres Finocchiaro 
> >> wrote:
> >>
> >> Hi,
> >>
> >> I'm the developer of a printing plugin which leverages JavaFX for a few
> >> HTML functions.
> >>
> >> One of our functions would greatly benefit from being "headless (or more
> >> 

Re: JavaFX Swipe Events Glitch On Mobile

[Nir Lisker]

Is this a bug caused by JavaFX or Gluon?

On Fri, Mar 20, 2020 at 9:24 PM Debayan Sutradhar <
debayansutradh...@gmail.com> wrote:

> Swipe Events like swipeup and swipedown don't get registered and their
> related set methods don't get called. OnTouch works rarely.
>
> JavaFX Version : 14
> Gluon Client Plugin : 0.0.18
>
> Regards,
> Debayan
>

RFR: 8241582: Infinite animation does not start from the end when started with a negative rate

[Nir Lisker]

### Cause

`Animation#jumpTo(Duration)` does not handle `Duration.INDEFINITE` properly. 
This causes
`InfiniteClipEnvelope#jumpTo(long)` to receive an erroneous value and start the 
animation not from the end, depending
on the modulo calculation.

### Fix

For infinite cycles, use the `cycleDuration` as the destination since that is 
the effective end point.

### Tests

* Added an `testJumpTo_IndefiniteCycles` test that fails without the patch and 
passes after it.
* Added a `testJumpTo_IndefiniteCycleDuration` that passes both before and 
after, just to make sure that this type of
  infinite duration is correct.
* Removed a test in `SequentialTransition` that fails with this patch, but it 
passed before it only because the modulo
  value happened to land in the right place. Changing the duration of one of 
the child animation can cause it to fail, so
  the test is faulty anyway at this stage.

### Future work

Playing backwards will still not work correctly, but at least now it start from 
the correct value. This is the first of
a series of fixes under the umbrella issue 
[JDK-8210238](https://bugs.openjdk.java.net/browse/JDK-8210238).

-

Commit messages:
 - Comment out erroneous test
 - Fix IndefiniteCycleDuration test
 - Fixed single loop test
 - Initial commit

Changes: https://git.openjdk.java.net/jfx/pull/169/files
 Webrev: https://webrevs.openjdk.java.net/jfx/169/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8241582
  Stats: 28 lines in 3 files changed: 19 ins; 1 del; 8 mod
  Patch: https://git.openjdk.java.net/jfx/pull/169.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/169/head:pull/169

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

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

[Nir Lisker]

On Wed, 15 Apr 2020 20:59:40 GMT, Kevin Rushforth  wrote:

>> Here are the results on Phil's machine, which is a Mac Book Pro with a 
>> graphics accelerator (Nvidia, I think).
>> 
>> Without the patch:
>> 2000 quads average 8.805 fps
>> 
>> With the patch:
>> 2000 quads average 4.719 fps
>> 
>> Almost a 2x performance hit.
>
> Conclusion: The new shaders that support attenuation don't seem to have much 
> of a performance impact on machines with
> an Intel HD, but on systems with a graphics accelerator, it is a significant 
> slowdown.
> So we are left with the two choices of doubling the number of shaders (that 
> is, a set of shaders with attenuation and a
> set without) or living with the performance hit (which will only be a problem 
> on machines with a dedicated graphics
> accelerator for highly fill-limited scenes). The only way we can justify a 2x 
> drop in performance is if we are fairly
> certain that this is a corner case, and thus unlikely to hit real 
> applications.  If we do end up deciding to replicate
> the shaders, I don't think it is all that much work. I'm more worried about 
> how well it would scale to subsequent
> improvements, although we could easily decide that for, say, spotlights 
> attenuation is so common that you wouldn't
> create a version that doesn't do that.  In the D3D HLSL shaders, ifdefs are 
> used, so the work would be to restore the
> original code and add the new code under an ifdef. Then double the number of 
> lines of gradle (at that point, I'd do it
> in a for-each loop), then modify the logic that loads the shaders to pick the 
> right one.  For GLSL, the different parts
> of the shader are in different files, so it's a matter of creating new 
> versions of each of the three lighting shaders
> that handle attenuation and choosing the right one at runtime.

I discussed this with a graphics engineer. He said that a couple of branches do 
not have any real performance impact
even on modern mobile devices, and that, e.g., on iOS 7 using half floats 
instead of floats was improving shader
execution dramatically. Desktops with NVIDIA or AMD and even Intel modern cards 
can process dozens of branches with no
significant performance degradation.

He suggested actually to have all the light types in a single shader file 
(looking ahead here). He also suggested not
to permute on shaders based on the number of lights and just pass in a uniform 
for that number and loop over it. The
permutations on the bump, specular and self illuminations components are 
correct (not sure we are not doing that for
the diffuse component). If we add later shadows, which is not on my near to-do 
list, then we should permute there.

It also depends on our target hardware. If we take into account hardware from, 
say, 2005 then maybe branching will
cause significant performance loss, but that hinders our ability to increase 
performance for newer hardware. What is
the policy here?

I have a Win10 laptop with a GeForce 610M that I will test this weekend to see 
if the mobile NVidia cards have some
issue.

-

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

Re: Testing JavaFX with Java14 preview features

[Nir Lisker]

Thanks, yes, testing on JavaFX itself.
I made these changes. I'm getting "error: invalid source release: 14" when
trying to build. These are the settings that are output during the task:

Gradle Distribution: Gradle wrapper from target build
Gradle Version: 6.3
Java Home: C:\Program Files\Java\jdk-14
JVM Arguments: None
Program Arguments: None
Build Scans Enabled: false
Offline Mode Enabled: false
Gradle Tasks: build

This is the full error message:

error: invalid source release: 14
Usage: javac  
use --help for a list of possible options

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':base:compileJava'.
> Compilation failed with exit code 2; see the compiler error output for
details.

On Tue, Apr 14, 2020 at 2:00 AM Kevin Rushforth 
wrote:

> I guess you mean modifying the FX build in your local repo so that you
> can test the use of JDK 14 preview features in FX itself? (if you were
> just trying to use it from your app you wouldn't need any build changes
> in FX). At a minimum you would need to add the "--enable-preview" flag
> to compile.options.compilerArgs, and change "sourceCompatibility" from
> "11" to "14". Not sure if anything more is needed. I've never tried it.
>
> -- Kevin
>
>
> On 4/13/2020 1:49 PM, Nir Lisker wrote:
> > Hi,
> >
> > I would like to test the preview features in Java 14 on JavaFX. What
> > changes should I make in the build files to get it working?
> >
> > - Nir
>
>

Re: Testing JavaFX with Java14 preview features

[Nir Lisker]

It contains:
-source
14
-target
14

Looks like I'll have to do some digging.

Thanks

On Tue, Apr 14, 2020 at 2:38 AM Kevin Rushforth 
wrote:

> Not sure, but you might check here:
>
> modules/javafx.base/build/tmp/compileJava/java-compiler-args.txt
>
> That's the list of args that gradle generates to pass to javac (using
> @.../java-compiler-args.txt)
>
> -- Kevin
>
> On 4/13/2020 4:10 PM, Nir Lisker wrote:
>
> Thanks, yes, testing on JavaFX itself.
> I made these changes. I'm getting "error: invalid source release: 14" when
> trying to build. These are the settings that are output during the task:
>
> Gradle Distribution: Gradle wrapper from target build
> Gradle Version: 6.3
> Java Home: C:\Program Files\Java\jdk-14
> JVM Arguments: None
> Program Arguments: None
> Build Scans Enabled: false
> Offline Mode Enabled: false
> Gradle Tasks: build
>
> This is the full error message:
>
> error: invalid source release: 14
> Usage: javac  
> use --help for a list of possible options
>
> FAILURE: Build failed with an exception.
>
> * What went wrong:
> Execution failed for task ':base:compileJava'.
> > Compilation failed with exit code 2; see the compiler error output for
> details.
>
> On Tue, Apr 14, 2020 at 2:00 AM Kevin Rushforth <
> kevin.rushfo...@oracle.com> wrote:
>
>> I guess you mean modifying the FX build in your local repo so that you
>> can test the use of JDK 14 preview features in FX itself? (if you were
>> just trying to use it from your app you wouldn't need any build changes
>> in FX). At a minimum you would need to add the "--enable-preview" flag
>> to compile.options.compilerArgs, and change "sourceCompatibility" from
>> "11" to "14". Not sure if anything more is needed. I've never tried it.
>>
>> -- Kevin
>>
>>
>> On 4/13/2020 1:49 PM, Nir Lisker wrote:
>> > Hi,
>> >
>> > I would like to test the preview features in Java 14 on JavaFX. What
>> > changes should I make in the build files to get it working?
>> >
>> > - Nir
>>
>>
>

Testing JavaFX with Java14 preview features

[Nir Lisker]

Hi,

I would like to test the preview features in Java 14 on JavaFX. What
changes should I make in the build files to get it working?

- Nir

Re: Testing JavaFX with Java14 preview features

[Nir Lisker]

I managed to get it working by adding  "--enable-preview" in several
places, I'm not sure if they are all needed. There will be a test branch in
my fork with the working build.grade.

On Tue, Apr 14, 2020 at 4:35 AM Nir Lisker  wrote:

> It contains:
> -source
> 14
> -target
> 14
>
> Looks like I'll have to do some digging.
>
> Thanks
>
> On Tue, Apr 14, 2020 at 2:38 AM Kevin Rushforth <
> kevin.rushfo...@oracle.com> wrote:
>
>> Not sure, but you might check here:
>>
>> modules/javafx.base/build/tmp/compileJava/java-compiler-args.txt
>>
>> That's the list of args that gradle generates to pass to javac (using
>> @.../java-compiler-args.txt)
>>
>> -- Kevin
>>
>> On 4/13/2020 4:10 PM, Nir Lisker wrote:
>>
>> Thanks, yes, testing on JavaFX itself.
>> I made these changes. I'm getting "error: invalid source release: 14"
>> when trying to build. These are the settings that are output during the
>> task:
>>
>> Gradle Distribution: Gradle wrapper from target build
>> Gradle Version: 6.3
>> Java Home: C:\Program Files\Java\jdk-14
>> JVM Arguments: None
>> Program Arguments: None
>> Build Scans Enabled: false
>> Offline Mode Enabled: false
>> Gradle Tasks: build
>>
>> This is the full error message:
>>
>> error: invalid source release: 14
>> Usage: javac  
>> use --help for a list of possible options
>>
>> FAILURE: Build failed with an exception.
>>
>> * What went wrong:
>> Execution failed for task ':base:compileJava'.
>> > Compilation failed with exit code 2; see the compiler error output for
>> details.
>>
>> On Tue, Apr 14, 2020 at 2:00 AM Kevin Rushforth <
>> kevin.rushfo...@oracle.com> wrote:
>>
>>> I guess you mean modifying the FX build in your local repo so that you
>>> can test the use of JDK 14 preview features in FX itself? (if you were
>>> just trying to use it from your app you wouldn't need any build changes
>>> in FX). At a minimum you would need to add the "--enable-preview" flag
>>> to compile.options.compilerArgs, and change "sourceCompatibility" from
>>> "11" to "14". Not sure if anything more is needed. I've never tried it.
>>>
>>> -- Kevin
>>>
>>>
>>> On 4/13/2020 1:49 PM, Nir Lisker wrote:
>>> > Hi,
>>> >
>>> > I would like to test the preview features in Java 14 on JavaFX. What
>>> > changes should I make in the build files to get it working?
>>> >
>>> > - Nir
>>>
>>>
>>

Gradle task for running tests without Webkit

[Nir Lisker]

Hi,

For those who didn't build Webkit, running tests is done with `-x
:web:test`. I think it makes sense to just add a test task that does
exactly that, for convenience.

What do you think?

- Nir

Re: RFR: 8240692: Cleanup of the javafx property objects

[Nir Lisker]

On Fri, 13 Mar 2020 15:02:48 GMT, Kevin Rushforth  wrote:

> So this begs the question: Do we still want to do this? Is the cleanup worth 
> the extra memory used?

I'm not sure. This raises the opposite question: are there any places where we 
can benefit from introducing anonymous
classes instead of the current implementation?

In my own code, I prefer the readability and conciseness over performance. 
However, since JavaFX is a library, we might
want to go with the performance route. I don't know how important the memory 
consumption is for users in this case or
the other similar cases.

-

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

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

[Nir Lisker]

On Fri, 3 Jan 2020 09:26:43 GMT, Ambarish Rapte  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Attenuation and range changed internally to floats from doubles
>
> I have added few comments, but have not run tests and sample yet.

@arapte Can you please test the performance changes too?

-

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

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Nir Lisker]

On Thu, 27 Feb 2020 21:29:27 GMT, Martin Polakovic 
 wrote:

>> If there are many columns, the current TableView will stall scrolling. 
>> Resolving this performance issue requires column
>> virtualization. Virtualization mode is enabled when the row height is fixed 
>> by the following method.
>> `tableView.setFixedCellSize(height)`
>> 
>> This proposal includes a fix because the current code does not correctly 
>> implement column virtualization.
>> 
>> The improvement of this proposal can be seen in the following test program.
>> 
>>  Java
>> import javafx.animation.AnimationTimer;
>> import javafx.application.Application;
>> import javafx.beans.property.SimpleStringProperty;
>> import javafx.collections.ObservableList;
>> import javafx.scene.Scene;
>> import javafx.scene.control.TableColumn;
>> import javafx.scene.control.TableView;
>> import javafx.scene.layout.BorderPane;
>> import javafx.stage.Stage;
>> 
>> public class BigTableViewTest2 extends Application {
>> 
>>  private static final boolean USE_WIDTH_FIXED_SIZE = true;
>>  private static final boolean USE_HEIGHT_FIXED_SIZE = true;
>> //   private static final int COL_COUNT=300;
>>  private static final int COL_COUNT=600;
>> //   private static final int COL_COUNT=1000;
>>  private static final int ROW_COUNT=1000;
>> 
>>  @Override
>>  public void start(Stage primaryStage) throws Exception {
>>  final TableView tableView = new TableView<>();
>> 
>>  final ObservableList> columns = 
>> tableView.getColumns();
>>  for(int i=0; i>  TableColumn column = new 
>> TableColumn<>("Col"+i);
>>  final int colIndex=i;
>>  column.setCellValueFactory((cell)->new 
>> SimpleStringProperty(cell.getValue()[colIndex]));
>>  columns.add(column);
>>  if(USE_WIDTH_FIXED_SIZE) {
>>  column.setPrefWidth(60);
>>  column.setMaxWidth(60);
>>  column.setMinWidth(60);
>>  }
>>  }
>> 
>>  ObservableList items = tableView.getItems();
>>  for(int i=0; i>  String[] rec = new String[COL_COUNT];
>>  for(int j=0; j>  rec[j] = i+":"+j;
>>  }
>>  items.add(rec);
>>  }
>>  BorderPane root = new BorderPane(tableView);
>>  if(USE_HEIGHT_FIXED_SIZE) {
>>  tableView.setFixedCellSize(24);
>>  }
>> 
>>  Scene scene = new Scene(root, 800, 800);
>>  primaryStage.setScene(scene);
>>  primaryStage.show();
>>  prepareTimeline(scene);
>> 
>>  }
>> 
>>  public static void main(String[] args) {
>>  Application.launch(args);
>>  }
>> 
>> private void prepareTimeline(Scene scene) {
>> new AnimationTimer() {
>> @Override public void handle(long now) {
>> double fps =  
>> com.sun.javafx.perf.PerformanceTracker.getSceneTracker(scene).getInstantFPS();
>> ((Stage)scene.getWindow()).setTitle("FPS:"+(int)fps);
>> }
>> }.start();
>> }
>> 
>> }
>
> I took the liberty of pointing out some formatting errors

The title of your PR should match the title of the JBS issue. Moreover, #108 
already tackles this issue with a
different approach and 2 PRs on the same issue can't be intetgrated. Since I'm 
not familiar with this code, I can't
advise on who's solution is more suitable. I suggest you discuss both of the 
approaches on the mailing list. I saw many
JBS issues around this performance issue, probably one of them fits (or a new 
one can be created).

-

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

Re: RFR: 8240542: Switch FX build to use JDK 14 as boot JDK

[Nir Lisker]

On Fri, 27 Mar 2020 17:10:09 GMT, Johan Vos  wrote:

>> Now that we have switched to using gradle 6.3 we can switch to using JDK 14 
>> as the boot JDK for JavaFX 15 builds.
>> 
>> 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`.
>
> Marked as reviewed by jvos (Reviewer).

> We will continue to generate class files using `-source 11 -target 11`

Isn't `-release 11` the preferred way?

-

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

Re: WeakXXListener - when not to use?

[Nir Lisker]

BTW, Tomas Mikula wrote about this on
http://tomasmikula.github.io/blog/2015/02/10/the-trouble-with-weak-listeners.html
.
There is a comment at the end that is worth a read too.


On Thu, Mar 26, 2020 at 1:53 PM Jeanette Winzenburg 
wrote:

>
> Zitat von Kevin Rushforth :
>
> Thanks for your input!
>
> Glad we didn't miss the "minimum bar" height - with the java doc being
> really clear on that :)
>
> What I still don't quite get is the concern about "too early" and "not
> cleaning up" - maybe I misunderstand the point entirely
>
> >
> > As for whether the above is sufficient, it depends on what the
> > listener does (what its purpose is).In this simple example, it seems
> > unlikely that removing the listener when the instance of SomeClass
> > goes out of scope will cause any problems. It's worth looking at
> > what "doSomethingUseful" does to see if unregisters anything that
> > ought to be unregistered (and now maybe won't be if the listener
> > goes away early).
> >
>
> if not doing that "doSomethingUseful" would cause a - more - terrible
> misbehavior than a memory leak, would that mean that the
> listening/update implementation in that specific case would have to be
> re-thought? F.i. in the case of the ButtonSkin listening to control's
> scene is changing global state which might be broken if it's not
> reverted to not having a default/cancel registered? (what a horrible
> sentence, sry ;)
>
> Hmm ..
>
>
>
>

Re: CFV: New OpenJFX Committer: Arun Joseph

[Nir Lisker]

Vote: YES

On Thu, Mar 26, 2020 at 1:46 PM Kevin Rushforth 
wrote:

> Vote: YES
>
> On 3/26/2020 4:44 AM, Kevin Rushforth wrote:
> > I hereby nominate Arun Joseph [1] to OpenJFX Committer.
>
>

RFR: 8242523: Update the animation and clip envelope classes

[Nir Lisker]

Mostly refactoring in preparation of the upcoming fixes. The changes might look 
like a lot, but it's mostly rearranging
of methods. Summery of changes:

### Animation
* Added `isNearZero` and `areNearEqual` methods that deal with `EPSILON` checks.
* Added `isStopped`, `isRunning` and `isPaused` convenience methods.
* Added `runHandler` method to deal with running the handler.
* Moved methods to be grouped closer to where they are used rather than by 
visibility.
* Removed the static import for `TickCalculation`.
* Various small subjective readability changes.
* Behavioral changes: switching `autoReverse` and `onFinished` properties from 
"Simple" to "Base" properties; and lazily
  initializing the `cuePoints` map.

### Clip Envelopes
* Added `MultiLoopClipEnvelope` as an intermediate parent for infinite and 
finite clip envelopes.
* Rearranged methods order to be consistent.
* Replaced the `checkBounds` method with a new overload of `Utils.clamp`.
* Renamed `pos` to `cyclePos`.
* Extracted common methods: `changedDirection` and `ticksRateChange`
* Added internal documentation.

Also corrected a typo in `TicksCalculation` and added an explanation for an 
animation test.

-

Commit messages:
 - Removed whitespace
 - Merge branch 'master' into 
8242523_Update_the_Animation_and_ClipEnvelope_classes
 - Removing cycleNumber for now
 - Fix typo in TicksCalculation & add an explanation for an animation test
 - Initial push of 8242523
 - Comment out erroneous test
 - Fix IndefiniteCycleDuration test
 - Fixed single loop test
 - Initial commit

Changes: https://git.openjdk.java.net/jfx/pull/196/files
 Webrev: https://webrevs.openjdk.java.net/jfx/196/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8242523
  Stats: 648 lines in 9 files changed: 336 ins; 174 del; 138 mod
  Patch: https://git.openjdk.java.net/jfx/pull/196.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/196/head:pull/196

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

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

[Nir Lisker]

>
> Will there also be any performance drop in case you just use the default
> parameters for the lighting?


That's what we're testing.

The default, which corresponds to the current lighting, should not need
> any additional computations
> and thus no performance drop.


But there is no way to know if you're using the default values unless you
check for it, and that already is a cost.
The only way to guarantee that there is no performance drop is to add a
boolean that enables/disables these changes, and that will require
generating double the shaders - for the "on" case and for the "off" case.
We are trying to avoid this. These changes should be negligible according
to "current knowledge", but we have to test. For some reason we saw a
drop in one of the hardware configurations that was tested.

On Sat, Apr 25, 2020 at 8:33 PM Michael Paus  wrote:

> Am 25.04.20 um 19:09 schrieb Kevin Rushforth:
> > On Fri, 24 Apr 2020 05:06:56 GMT, Phil Race  wrote:
> >
> >>> 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 ?
> > If this were an even remotely representative use case, then no, the
> performance hit would not be OK. The test was
> > designed as an artificial "worst-case" stress test: a single mesh with a
> large number of very large (window-sized)
> > quads stacked on top of each other. Any real-world use case won't do
> this.
> >
> > We should make sure that we aren't seeing any significant performance
> drop when rendering spheres (at a couple
> > different tessellation levels) or boxes.
> >
> > -
> >
> > PR: https://git.openjdk.java.net/jfx/pull/43
>
> Will there also be any performance drop in case you just use the default
> parameters for the lighting?
> The default, which corresponds to the current lighting, should not need
> any additional computations
> and thus no performance drop.
>
>

Re: RFR: 8241582: Infinite animation does not start from the end when started with a negative rate

[Nir Lisker]

On Wed, 22 Apr 2020 00:08:05 GMT, Kevin Rushforth  wrote:

>> ### Cause
>> 
>> `Animation#jumpTo(Duration)` does not handle `Duration.INDEFINITE` properly. 
>> This causes
>> `InfiniteClipEnvelope#jumpTo(long)` to receive an erroneous value and start 
>> the animation not from the end, depending
>> on the modulo calculation.  ### Fix
>> 
>> For infinite cycles, use the `cycleDuration` as the destination since that 
>> is the effective end point.
>> 
>> ### Tests
>> 
>> * Added an `testJumpTo_IndefiniteCycles` test that fails without the patch 
>> and passes after it.
>> * Added a `testJumpTo_IndefiniteCycleDuration` that passes both before and 
>> after, just to make sure that this type of
>>   infinite duration is correct.
>> * Removed a test in `SequentialTransition` that fails with this patch, but 
>> it passed before it only because the modulo
>>   value happened to land in the right place. Changing the duration of one of 
>> the child animation can cause it to fail, so
>>   the test is faulty anyway at this stage.
>> 
>> ### Future work
>> 
>> Playing backwards will still not work correctly, but at least now it start 
>> from the correct value. This is the first of
>> a series of fixes under the umbrella issue 
>> [JDK-8210238](https://bugs.openjdk.java.net/browse/JDK-8210238).
>
> modules/javafx.graphics/src/test/java/test/javafx/animation/AnimationTest.java
>  line 271:
> 
>> 270: animation.jumpTo("end");
>> 271: assertEquals(Duration.millis(Long.MAX_VALUE / 6), 
>> animation.getCurrentTime());
>> 272: }
> 
> Why `/ 6` ?

This is a good question and I think I should have explained it in a comment 
because it also points to a mini-flaw. The
short answer is that the 6 comes from the `TicksCalculation` class, which 
defines `TICKS_PER_SECOND = 6000` and
`TICKS_PER_MILI = TICKS_PER_SECOND / 1000.0` which is 6.

The test works as follows:

This is the "ticks from duration" calculation for jumping to the end 
(`Duration.INDEFINITE`), demonstrated by
mathematical substitutions: double millis = Duration.INDEFINITE.toMillis() = 
Double.POSITIVE_INFINITY
long ticks = TickCalculation.fromMillis(millis) = Math.round(TICKS_PER_MILI * 
millis)
= Math.round(6 * Double.POSITIVE_INFINITY) = 
Math.round(Double.POSITIVE_INFINITY)
= Long.MAX_VALUE (as per the spec. of Math.round)
Notice that we lose precision when multiplying `POSITIVE_INFINITY`.

Then getting the current time calculates "duration from ticks":

animation.getCurrentTime() = TickCalculation.toDuration(currentTicks)
= ticks / TICKS_PER_MILI = Long.MAX_VALUE / 6

So the division carries out normally (there's no `Long.POSITIVE_INFINITY`), but 
the multiplication does not.

Maybe we shouldn't convert between the `double`-based `Duration` and the `long` 
ticks so much, but I think that this is
somewhat negligible.

My next patch is refactoring in preparation of the next, heavier, changes, so I 
will add this explanation on the test.

-

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

Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes

[Nir Lisker]

On Mon, 27 Apr 2020 13:19:49 GMT, Kevin Rushforth  wrote:

>> The title of this PR should match exactly the title of the JBS bug. So:
>> 
>> 8243115: Spurious invalidations due to bug in IntegerBinding and other 
>> classes
>
> @arapte can you also review this?

I will review this too anyway.

-

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

Community request to test 3D performance

[Nir Lisker]

Hi all,

My PR [1] for adding attenuation for PointLight is pending tests from
setups with recent NVidia or AMD GPUs. If anyone has such a setup, it would
greatly help to get tests results from it.

Thanks,
Nir

[1] https://github.com/openjdk/jfx/pull/43

Re: RE; Community request to test 3D performance

[Nir Lisker]

Isn't it this one?
https://github.com/openjdk/jfx/pull/43#issuecomment-599080407

On Fri, Apr 24, 2020 at 1:02 AM Kevin Rushforth 
wrote:

> Thanks, David.
>
> I presume you will be running Windows? Ideally we would get results on
> Windows and Mac.
>
> Nir: I can upload the modified version of the benchmark, unless you have
> a working version.
>
> -- Kevin
>
>
> On 4/23/2020 2:41 PM, Nir Lisker wrote:
> > I think so. The test is relatively simple, so it should be worth it.
> Thanks.
> >
> > On Fri, Apr 24, 2020 at 12:04 AM David Grieve <
> david.gri...@microsoft.com>
> > wrote:
> >
> >> I have an NVIDIA Quadro P400. Will that help?
> >>
> >> -Original Message-
> >> From: openjfx-dev  On Behalf Of
> Nir
> >> Lisker
> >> Sent: Thursday, April 23, 2020 3:55 PM
> >> To: openjfx-dev@openjdk.java.net Mailing 
> >> Subject: [EXTERNAL] Community request to test 3D performance
> >>
> >> Hi all,
> >>
> >> My PR [1] for adding attenuation for PointLight is pending tests from
> >> setups with recent NVidia or AMD GPUs. If anyone has such a setup, it
> would
> >> greatly help to get tests results from it.
> >>
> >> Thanks,
> >> Nir
> >>
> >> [1]
> >>
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenjdk%2Fjfx%2Fpull%2F43data=02%7C01%7CDavid.Grieve%40microsoft.com%7C6fb8b89f9d724c990dd608d7e7c0c848%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637232687495430880sdata=OCgL7t6vDim%2F9Lek4htImF8dMZmzAbETAzmoj91T9SE%3Dreserved=0
> >>
>
>

Re: RE; Community request to test 3D performance

[Nir Lisker]

I think so. The test is relatively simple, so it should be worth it. Thanks.

On Fri, Apr 24, 2020 at 12:04 AM David Grieve 
wrote:

> I have an NVIDIA Quadro P400. Will that help?
>
> -Original Message-
> From: openjfx-dev  On Behalf Of Nir
> Lisker
> Sent: Thursday, April 23, 2020 3:55 PM
> To: openjfx-dev@openjdk.java.net Mailing 
> Subject: [EXTERNAL] Community request to test 3D performance
>
> Hi all,
>
> My PR [1] for adding attenuation for PointLight is pending tests from
> setups with recent NVidia or AMD GPUs. If anyone has such a setup, it would
> greatly help to get tests results from it.
>
> Thanks,
> Nir
>
> [1]
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenjdk%2Fjfx%2Fpull%2F43data=02%7C01%7CDavid.Grieve%40microsoft.com%7C6fb8b89f9d724c990dd608d7e7c0c848%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637232687495430880sdata=OCgL7t6vDim%2F9Lek4htImF8dMZmzAbETAzmoj91T9SE%3Dreserved=0
>

Re: RFR: 8242523: Update the animation and clip envelope classes

[Nir Lisker]

On Tue, 5 May 2020 05:47:30 GMT, Ambarish Rapte  wrote:

>> Mostly refactoring in preparation of the upcoming fixes. The changes might 
>> look like a lot, but it's mostly rearranging
>> of methods. Summery of changes:
>> ### Animation
>> * Added `isNearZero` and `areNearEqual` methods that deal with `EPSILON` 
>> checks.
>> * Added `isStopped`, `isRunning` and `isPaused` convenience methods.
>> * Added `runHandler` method to deal with running the handler.
>> * Moved methods to be grouped closer to where they are used rather than by 
>> visibility.
>> * Removed the static import for `TickCalculation`.
>> * Various small subjective readability changes.
>> * Behavioral changes: switching `autoReverse` and `onFinished` properties 
>> from "Simple" to "Base" properties; and lazily
>>   initializing the `cuePoints` map.
>> 
>> ### Clip Envelopes
>> * Added `MultiLoopClipEnvelope` as an intermediate parent for infinite and 
>> finite clip envelopes.
>> * Rearranged methods order to be consistent.
>> * Replaced the `checkBounds` method with a new overload of `Utils.clamp`.
>> * Renamed `pos` to `cyclePos`.
>> * Extracted common methods: `changedDirection` and `ticksRateChange`
>> * Added internal documentation.
>> 
>> Also corrected a typo in `TicksCalculation` and added an explanation for an 
>> animation test.
>
> modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/MultiLoopClipEnvelope.java
>  line 48:
> 
>> 47:
>> 48: protected boolean autoReverse() {
>> 49: return autoReverse;
> 
> I would suggest the name to be `isAutoReverese`

That would be the usual naming convention, yes, but I find that  this can be 
more fluent to read. Perhaps I'm
influenced by the upcoming `record`s feature. Can change.

-

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

Re: RFR: 8242523: Update the animation and clip envelope classes

[Nir Lisker]

On Wed, 6 May 2020 14:18:06 GMT, Ambarish Rapte  wrote:

>> Mostly refactoring in preparation of the upcoming fixes. The changes might 
>> look like a lot, but it's mostly rearranging
>> of methods. Summery of changes:
>> ### Animation
>> * Added `isNearZero` and `areNearEqual` methods that deal with `EPSILON` 
>> checks.
>> * Added `isStopped`, `isRunning` and `isPaused` convenience methods.
>> * Added `runHandler` method to deal with running the handler.
>> * Moved methods to be grouped closer to where they are used rather than by 
>> visibility.
>> * Removed the static import for `TickCalculation`.
>> * Various small subjective readability changes.
>> * Behavioral changes: switching `autoReverse` and `onFinished` properties 
>> from "Simple" to "Base" properties; and lazily
>>   initializing the `cuePoints` map.
>> 
>> ### Clip Envelopes
>> * Added `MultiLoopClipEnvelope` as an intermediate parent for infinite and 
>> finite clip envelopes.
>> * Rearranged methods order to be consistent.
>> * Replaced the `checkBounds` method with a new overload of `Utils.clamp`.
>> * Renamed `pos` to `cyclePos`.
>> * Extracted common methods: `changedDirection` and `ticksRateChange`
>> * Added internal documentation.
>> 
>> Also corrected a typo in `TicksCalculation` and added an explanation for an 
>> animation test.
>
> modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 
> 142:
> 
>> 141: }
>> 142:
>> 143: /*
> 
> Both these methods seem like they belong to Utils.java. If moved to 
> Utils.java then the all the calls
> `(Math.abs(currentRate - rate) < EPSILON)` can be changed to use these 
> methods from Utils.java. If we move these
> methods then, Utils.java also needs to declare  `static final double EPSILON 
> = 1e-12;`  and the EPSILON here can be
> initialized as `private static final double EPSILON = Utils.EPSILON;`

I agree that these methods are better suited there, but I'm not sure the same 
epsilon value will be suitable for other
places that will want to use these. That value is somewhat arbitrary anyway I 
think.

-

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

Re: RFR: 8242523: Update the animation and clip envelope classes

[Nir Lisker]

On Tue, 5 May 2020 11:19:55 GMT, Ambarish Rapte  wrote:

>> Mostly refactoring in preparation of the upcoming fixes. The changes might 
>> look like a lot, but it's mostly rearranging
>> of methods. Summery of changes:
>> ### Animation
>> * Added `isNearZero` and `areNearEqual` methods that deal with `EPSILON` 
>> checks.
>> * Added `isStopped`, `isRunning` and `isPaused` convenience methods.
>> * Added `runHandler` method to deal with running the handler.
>> * Moved methods to be grouped closer to where they are used rather than by 
>> visibility.
>> * Removed the static import for `TickCalculation`.
>> * Various small subjective readability changes.
>> * Behavioral changes: switching `autoReverse` and `onFinished` properties 
>> from "Simple" to "Base" properties; and lazily
>>   initializing the `cuePoints` map.
>> 
>> ### Clip Envelopes
>> * Added `MultiLoopClipEnvelope` as an intermediate parent for infinite and 
>> finite clip envelopes.
>> * Rearranged methods order to be consistent.
>> * Replaced the `checkBounds` method with a new overload of `Utils.clamp`.
>> * Renamed `pos` to `cyclePos`.
>> * Extracted common methods: `changedDirection` and `ticksRateChange`
>> * Added internal documentation.
>> 
>> Also corrected a typo in `TicksCalculation` and added an explanation for an 
>> animation test.
>
> modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/ClipEnvelope.java
>  line 46:
> 
>> 45:  */
>> 46: public abstract class ClipEnvelope {
>> 47:
> 
> I think the removal of line 46 was unintended change.

I think that there is no empty line between the docs and the declaration, but 
can revert.

-

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

Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes

[Nir Lisker]

On Tue, 12 May 2020 17:58:09 GMT, John Hendrikx  wrote:

> I'm fine with doing a fix, but I need to know which one. Avoiding NPE's and 
> silently doing nothing is IMHO not very
> desirable as this will give the user of the API no feedback that something 
> went wrong.
> So I would prefer to fix this by documenting that these cases will result in 
> a NPE.
> 
> The `bind` method has a similar issue -- it doesn't check its array elements 
> for `null`, and will throw a NPE when
> attempting to add a listener to `null`. Again, I would just document the NPE 
> so what is clearly a mistake doesn't go
> unnoticed.

I think that checking the array elements doesn't help much because by the time 
you can check them they will already be
used, and that will throw an NPE implicitly.

`bind` is no-op for `null` or 0-length arrays, but should have probably throw 
an NPE on the `null` case. The 0-length
check saves creating the `observer`, so I think it makes sense. `unbind` should 
probably only throw an NPE on `null`
arrays, but that happens implicitly anyway, so maybe there is no change needed 
unless it's for clarity because we can
add a custom error message.

-

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

Re: RFR: 8242523: Update the animation and clip envelope classes

[Nir Lisker]

On Mon, 11 May 2020 04:30:28 GMT, Ambarish Rapte  wrote:

>> Mostly refactoring in preparation of the upcoming fixes. The changes might 
>> look like a lot, but it's mostly rearranging
>> of methods. Summery of changes:
>> ### Animation
>> * Added `isNearZero` and `areNearEqual` methods that deal with `EPSILON` 
>> checks.
>> * Added `isStopped`, `isRunning` and `isPaused` convenience methods.
>> * Added `runHandler` method to deal with running the handler.
>> * Moved methods to be grouped closer to where they are used rather than by 
>> visibility.
>> * Removed the static import for `TickCalculation`.
>> * Various small subjective readability changes.
>> * Behavioral changes: switching `autoReverse` and `onFinished` properties 
>> from "Simple" to "Base" properties; and lazily
>>   initializing the `cuePoints` map.
>> 
>> ### Clip Envelopes
>> * Added `MultiLoopClipEnvelope` as an intermediate parent for infinite and 
>> finite clip envelopes.
>> * Rearranged methods order to be consistent.
>> * Replaced the `checkBounds` method with a new overload of `Utils.clamp`.
>> * Renamed `pos` to `cyclePos`.
>> * Extracted common methods: `changedDirection` and `ticksRateChange`
>> * Added internal documentation.
>> 
>> Also corrected a typo in `TicksCalculation` and added an explanation for an 
>> animation test.
>
> modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/MultiLoopClipEnvelope.java
>  line 59:
> 
>> 58: return Math.round((ticks - deltaTicks) * Math.abs(newRate / 
>> rate));
>> 59:  }
>> 60:
> 
> This is similar to `ClipEnvelope.ticksRateChange()` with a minor difference. 
> Can this be removed from here and
> `ClipEnvelope.ticksRateChange()` be reused ?

I think that in the next patches there will be another change here anyway that 
will make this more sensible. I just
tried to split the final patch into more manageable chunks so some changes need 
future vision.

-

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

Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes

[Nir Lisker]

On Tue, 28 Apr 2020 00:00:28 GMT, Kevin Rushforth  wrote:

>> I will review this too anyway.
>
>> I will review this too anyway.
> 
> Thank you. That will be helpful.

As I started my review I noticed that `unbind` does not null-check its argument 
`dependencies` like `bind` does and it
can lead to NPEs. If it is out of scope for this PR to fix this, a new issue 
should be filed.

-

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

Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes

[Nir Lisker]

On Mon, 11 May 2020 22:27:27 GMT, Nir Lisker  wrote:

>>> I will review this too anyway.
>> 
>> Thank you. That will be helpful.
>
> As I started my review I noticed that `unbind` does not null-check its 
> argument `dependencies` like `bind` does and it
> can lead to NPEs. If it is out of scope for this PR to fix this, a new issue 
> should be filed.

The fix looks correct and the tests pass. I just wonder why the change to 
reflection-based construction with
`bindingMockClassConstructor`?

-

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

Re: Mac: Supported MacOS JDKs

[Nir Lisker]

if this is confirmed I can update the page.

On Tue, Apr 14, 2020 at 1:18 PM Florian Kirmaier 
wrote:

> Hi everyone,
>
> it seems to me, that the newest JDK for Mac (MacOSX10.15.sdk) doesn't work
> to build JavaFX.
> It works for me with 10.14 but not with 10.15.
> Can anyone confirm this?
> It would be great to mention this in the build instructions here:
> https://wiki.openjdk.java.net/display/OpenJFX/Building+OpenJFX
>
> Greetings Florian Kirmaier
>

Re: Proposed IntegerSpinner buggy behavior correction - JDK-8242553

[Nir Lisker]

I would say that for doubles, the minimum step size is the one given by ulp
= Math.ulp(double). Then the double case should behave like the
integer case where the ulp is 1 I think.
So, for the angle case of [0, 360), if we are at 360 - ulp, incrementing by
N * ulp will get us to (N-1) * ulp.

On Wed, Apr 22, 2020 at 3:41 PM Kevin Rushforth 
wrote:

> Hi Ajit,
>
> Thanks for writing this up. I think we are clear on what should happen
> for IntegerSpinnerValueFactory and ListSpinnerValueFactory. To answer
> your two questions, I'll take the second one first:
>
> > 2. Do we really care about negative values in no of steps OR
> > amountToStepBy?
> > I am asking this as we already have separate increment() and
> > decrement() methods. Should we bar such negative values? That’s
> > another behavior change though :)
>
> Once you switch to a proper modulo function, the easy answer is that
> negative values will "just work". A negative amountToStepBy might even
> make sense in some use cases (think of an increasing debt value), so
> there is no reason not to support it. A negative number of steps doesn't
> really make sense, since if you want to call increment(-N) you could
> equivalently call decrement(N), but again, it will "just work" as
> expected once the wrapping function is fixed. If we were designing from
> scratch I'd specify that nsteps must be >= 0, but it isn't worth it to
> make the change now (which would require a CSR) to prohibit it.
>
> > 1. Need to decide what would be better way to fix
> > DoubleSpinnerValueFactory wrapAround behavior?
>
> It might be helpful to compare it to the Integer flavor. By definition,
> IntegerSpinnerValueFactory represents a set of discrete values: min and
> max are two distinct values, and there is a well-defined ordering when
> wrapping. If you wrap, it should be a continuous (in both positive and
> negative directions) list of values like so:
>
> min, min+1, min+2, ... max-2, max-1, max, min, min+1, min+2, ...
>
> The formula is simple. After incrementing or decrementing by N *
> amountToStepBy, do the following if wrapAround is true:
>
>  if (val < min || val > max) val = (val - min) % (max - min + 1) + min;
>
> The above works regardless of step size because integers are naturally
> discrete values. It is well-behaved and natural to consider max and min
> being 1 apart from each other when wrapping.
>
> By contrast, it's harder to know what the right answer is for
> DoubleSpinnerValueFactory. It's quite possible that in some cases, the
> app expects the values of DoubleValue factory to also be discrete when
> it comes to wrapping, meaning that min and max are distinct values such
> that if your step was 1.0, you might expect to go from max-1.0 to max to
> min to min+1.0 (although even that definition has problems for
> non-integer values of amountToStepBy, but it's (mostly) solvable). It's
> also quite likely for other cases -- especially ones that would lend
> themselves to wrapAround mode in graphical apps -- where it isn't a set
> of discrete values, but rather is such that min and max represent the
> same value (i.e., the same end result). The simplest example is an angle
> in degrees where 0.0 and 360.0 mean the same thing when used as a
> rotation angle. In this latter use case, stepping from 359.0 to 360.0 to
> 0.0 to 1.0 would cause a discontinuous pause in motion if you mapped the
> output to a rotation angle.
>
> I'm not sure what the right answer is, but the latter seems like a valid
> use case and I would guess at least as common as a "set of discrete
> values" for doubles. Another thing to consider is that even in what I
> will call "discrete" mode (no I'm not really proposing that we add a
> property to control it), you have to take the step size into account;
> otherwise when the step size is < 0.5 you will get stuck at max when
> incrementing by 1 amountToStepBy, unless you have special case logic for
> that.
>
> Any thoughts from other developers?
>
> -- Kevin
>

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

[Nir Lisker]

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

Nir Lisker has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now
contains 11 commits:

 - Merge branch 'master' into 8217472_Add_attenuation_for_PointLight
 - Attenuation and range changed internally to floats from doubles
 - Fixed shader compilation errors for 2 and 3 lights in es2
 - Addressing review comments
 - Fixed whitespaces
 - Correction for indexes
 - Docs and year update
 - Merge remote-tracking branch 
'nlisker/8217472_Add_attenuation_for_PointLight' into
   8217472_Add_attenuation_for_PointLight
 - GL pipeline
 - Separate range from attenuation
 - ... and 1 more: https://git.openjdk.java.net/jfx/compare/4ec163df...2e1223ed

-

Changes: https://git.openjdk.java.net/jfx/pull/43/files
 Webrev: https://webrevs.openjdk.java.net/jfx/43/webrev.04
  Stats: 501 lines in 27 files changed: 391 ins; 13 del; 97 mod
  Patch: https://git.openjdk.java.net/jfx/pull/43.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/43/head:pull/43

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

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

[Nir Lisker]

On Sat, 25 Apr 2020 17:07:21 GMT, Kevin Rushforth  wrote:

> We should make sure that we aren't seeing any significant performance drop 
> when rendering spheres (at a couple
> different tessellation levels) or boxes.

I missed this. Do you mean that the test should create a mesh of a sphere 
instead of a flat surface?

-

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

Re: RFR: 8242523: Update the animation and clip envelope classes

[Nir Lisker]

On Thu, 7 May 2020 08:50:23 GMT, Ambarish Rapte  wrote:

>> Mostly refactoring in preparation of the upcoming fixes. The changes might 
>> look like a lot, but it's mostly rearranging
>> of methods. Summery of changes:
>> ### Animation
>> * Added `isNearZero` and `areNearEqual` methods that deal with `EPSILON` 
>> checks.
>> * Added `isStopped`, `isRunning` and `isPaused` convenience methods.
>> * Added `runHandler` method to deal with running the handler.
>> * Moved methods to be grouped closer to where they are used rather than by 
>> visibility.
>> * Removed the static import for `TickCalculation`.
>> * Various small subjective readability changes.
>> * Behavioral changes: switching `autoReverse` and `onFinished` properties 
>> from "Simple" to "Base" properties; and lazily
>>   initializing the `cuePoints` map.
>> 
>> ### Clip Envelopes
>> * Added `MultiLoopClipEnvelope` as an intermediate parent for infinite and 
>> finite clip envelopes.
>> * Rearranged methods order to be consistent.
>> * Replaced the `checkBounds` method with a new overload of `Utils.clamp`.
>> * Renamed `pos` to `cyclePos`.
>> * Extracted common methods: `changedDirection` and `ticksRateChange`
>> * Added internal documentation.
>> 
>> Also corrected a typo in `TicksCalculation` and added an explanation for an 
>> animation test.
>
> modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/SingleLoopClipEnvelope.java
>  line 102:
> 
>> 101: long ticksChange = Math.round(currentTick * currentRate);
>> 102: ticks = Utils.clamp(0, deltaTicks + ticksChange, 
>> cycleTicks);
>> 103: AnimationAccessor.getDefault().playTo(animation, ticks, 
>> cycleTicks);
> 
> This could remain unchanged. The `ticksChange` value is not really used 
> elsewhere here.

This change was made with extracting similar code from the other clip envelopes 
in mind, though there would need to be
some future discussion about it. I think it also makes the code more readable 
anyway.

-

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

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

[Nir Lisker]

On Tue, 17 Mar 2020 11:43:16 GMT, Frederic Thevenet 
 wrote:

>> 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:
> 
>   Revert changes in import statements

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
 line 1483:

> 1482: IntBuffer buffer, ResourceFactory 
> rf, QuantumImage tileImg, QuantumImage
> targetImg) { 1483: com.sun.prism.RTTexture rt = 
> tileImg.getRT(w, h, rf);
> 1484: if (rt == null) {

Any reason why the fully qualified name is needed?

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
 line 1502:

> 1501: private void renderWholeImage(int x, int y, int w, int h, 
> ResourceFactory rf, QuantumImage pImage) {
> 1502: com.sun.prism.RTTexture rt = pImage.getRT(w, h, rf);
> 1503: if (rt == null) {

Same questions for `RTTexture`

-

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

Re: Raspberry Pi 4 hw support

[Nir Lisker]

I thought that Johan Vos's team did some work on Raspberry, maybe he can
comment on this.

On Mon, Mar 9, 2020 at 9:50 AM Debayan Sutradhar <
debayansutradh...@gmail.com> wrote:

> I have a large embedded project for raspberry pis (https://stream-pi.com)
> and recently its discord server crosses 60 members and growing every month.
> Unfortunately it seems that Hardware acceleration doesnt work on VideoCore
> VI.
>
> If es2 pipeline is used, the program exits after the output "* failed to
> add service - already in use?"
>
> Hence for now we are using sw pipeline to use javafx apps. That makes them
> very slow and touch gestures don't work as expect.
>
> Regards,
> Debayan Sutradhar
>

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

[Nir Lisker]

On Tue, 17 Mar 2020 02:50:28 GMT, Nir Lisker  wrote:

>> Updated test case: 
>> [attenTest2.zip](https://github.com/openjdk/jfx/files/4332937/attenTest2.zip)
>
> On Win 10 with an AMD RX 470 4GB I get the following:
> 
> Without the patch:
> 200 quads average 113 fps
> 5000 quads average 11.5 fps
> 
> With the patch:
> 200 quads average 106 fps fps
> 5000 quads average 8.5 fps
> 
> Will test on Ubuntu later.

On Ubuntu 18 with an AMD RX 470 4GB I get the following:

Without the patch:
200 quads average 107 fps
5000 quads average 11.5 fps

With the patch:
200 quads average 107 fps fps
5000 quads average 11 fps

-

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

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

[Nir Lisker]

On Tue, 17 Mar 2020 11:01:55 GMT, Ambarish Rapte  wrote:

>> Frederic Thevenet has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fixed code style and typo following review.
>
> The PR looks good to me, Please revert the changes in import.

Now that the tiling is done in the `QuantumRenderer` level, I'll bring back
[JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082). Can't this 
tiling be used to fix that?

-

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

Re: [Rev 01] RFR: 8240692: Cleanup of the javafx property objects

[Nir Lisker]

> A simple readability cleanup for the files that were changed in #113.
> 
> Note that the boolean property has an `instanceof` check that doesn't exist 
> in the other properties.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Revert changes of anonymous classes usage

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/141/files
  - new: https://git.openjdk.java.net/jfx/pull/141/files/15f50f17..6876a037

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

  Stats: 111 lines in 5 files changed: 100 ins; 1 del; 10 mod
  Patch: https://git.openjdk.java.net/jfx/pull/141.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/141/head:pull/141

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

Behavior of jumpTo

[Nir Lisker]

Hi,

As I'm continuing my work on the animations front, I came across an
ambiguous behavior of JumpTo. The specifications don't say anything about
it, but in FiniteClipEnvelop, if the rate is negative, it jumps to the
specified time *counted from the end*. This seems intentional because it's
checked explicitly:

pos = ticks % cycleTicks;
if (rate < 0) {
pos = cycleTicks - pos;
}
if ((pos == 0) && (ticks > 0)) {
pos = cycleTicks;
}

SingleLoopClipEnvelope does not behave this way. I submitted [1] to track
this. This also causes an issue with jumping when rate=0, however, there
are other issues with rate=0, for example [2], and it's hard to separate
them.

What should be the proper behavior of jumpTo with respect to the rate? I
think that they shouldn't be coupled: jumping should always be calculated
from the start. The application can always calculate (totalTime -
requiredTime) and send that to jumpTo.

- Nir

[1] https://bugs.openjdk.java.net/browse/JDK-8237973
[2] https://bugs.openjdk.java.net/browse/JDK-8237757

Re: Build instructions for Ubuntu 20.04

[Nir Lisker]

Updated

On Tue, Mar 24, 2020 at 3:26 AM Thiago Milczarek Sayao <
thiago.sa...@clamed.com.br> wrote:

> Hi,
>
> Can someone  update
> https://wiki.openjdk.java.net/display/OpenJFX/Building+OpenJFX for Ubuntu
> 20.04?
>
>
> 1) libavformat-ffmpeg57 -> libavformat58
>
> 2) Need to add libxxf86vm-dev
>
>
> Thanks
>
>

Re: [Rev 02] RFR: 8240692: Cleanup of the javafx property objects

[Nir Lisker]

> A simple readability cleanup for the files that were changed in #113.
> 
> Note that the boolean property has an `instanceof` check that doesn't exist 
> in the other properties.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Re-added newline at the end

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/141/files
  - new: https://git.openjdk.java.net/jfx/pull/141/files/6876a037..93f50005

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/141/webrev.02
 - incr: https://webrevs.openjdk.java.net/jfx/141/webrev.01-02

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

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

Release notes presentation

[Nir Lisker]

Hi,

I posted the release notes for OpenJFX 14 on Reddit. The top comment [1]
complained about the presentation. Maybe on openjfx.io the Release Notes
link could show a more "friendly" summary with a link to the detailed
table. I suggest:

1. Including only changes that matter to the users of the library with
separate sections for new API and for (non-internal) bug fixes.
2. Highlighting significant changes.

Some example bug fixes to include:
- TableView, ListView: unexpected scrolling behaviour on up/down keys
- DragAndDrop no longer works with GTK3
- Window order is not correct when Modality.WINDOW_MODAL

Don't include:
- [WebView] Sub-resource integrity check fails on Windows and Linux
- javafx.web build fails on XCode 10.2
- Cherry pick GTK WebKit ___ changes

Enhancements to include:
- Add support for e-paper displays
- Add exclusion scope for LightBase
- Point2D and Point3D should implement Interpolatable

Don't include:
-  Color, Point2D and Point3D's fields should be made final

Thoughts?

- Nir

[1]
https://www.reddit.com/r/java/comments/fegcv9/release_notes_for_javafx_14/fjphqfn?utm_source=share_medium=web2x

Re: [Integrated] RFR: 8240689: Remove the JavaBeanXxxPropertyBuilders constructors

[Nir Lisker]

Changeset: e3026b9c
Author:Nir Lisker 
Date:  2020-03-09 16:38:20 +
URL:   https://git.openjdk.java.net/jfx/commit/e3026b9c

8240688: Remove the JavaBeanXxxPropertyBuilders constructors

Reviewed-by: kcr, arapte

! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/JavaBeanBooleanPropertyBuilder.java
! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/JavaBeanDoublePropertyBuilder.java
! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/JavaBeanFloatPropertyBuilder.java
! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/JavaBeanIntegerPropertyBuilder.java
! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/JavaBeanLongPropertyBuilder.java
! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/JavaBeanObjectPropertyBuilder.java
! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/JavaBeanStringPropertyBuilder.java
! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/ReadOnlyJavaBeanBooleanPropertyBuilder.java
! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/ReadOnlyJavaBeanDoublePropertyBuilder.java
! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/ReadOnlyJavaBeanFloatPropertyBuilder.java
! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/ReadOnlyJavaBeanIntegerPropertyBuilder.java
! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/ReadOnlyJavaBeanLongPropertyBuilder.java
! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/ReadOnlyJavaBeanObjectPropertyBuilder.java
! 
modules/javafx.base/src/main/java/javafx/beans/property/adapter/ReadOnlyJavaBeanStringPropertyBuilder.java

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

[Nir Lisker]

On Fri, 6 Mar 2020 10:46:44 GMT, Frederic Thevenet 
 wrote:

>> 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.
>
> The pull request has been updated with 1 additional commit.

modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1296:

> 1295: int width = Math.max(xMax - xMin, 1);
> 1296: int height = Math.max(yMax - yMin, 1);
> 1297: if (wimg == null) {

The changes here do not need to be reverted, this is still extra calculation.

-

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

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

[Nir Lisker]

On Sat, 14 Mar 2020 15:31:18 GMT, Kevin Rushforth  wrote:

>> I'll attach the above modified testcase that I ran. I ran it on a relatively 
>> new Windows 10 laptop and a rather ancient
>> MacBook Pro. I had to drastically reduce the number of quads on the Mac, but 
>> the results are similar: no significant
>> difference between the current code and the proposed patch for point lights 
>> (without attenuation).  I'd like to see
>> results on a recent machine with a graphics accelerator (either NVIDIA or 
>> AMD/ATI) to see if the new shader hurts
>> performance there, but I suspect it will be fine.
>
> Updated test case: 
> [attenTest2.zip](https://github.com/openjdk/jfx/files/4332937/attenTest2.zip)

On Win 10 with an AMD RX 470 4GB I get the following:

Without the patch:
200 quads average 113 fps
5000 quads average 11.5 fps

With the patch:
200 quads average 106 fps fps
5000 quads average 8.5 fps

Will test on Ubuntu later.

-

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

Re: RFR: 8217472: Add attenuation for PointLight [v5]

[Nir Lisker]

On Tue, 1 Sep 2020 17:00:10 GMT, Kevin Rushforth  wrote:

> gradle -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests MyTest

What format is `MyTest`? Is it some relative path?

-

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

RFR: 8252547: Correct transformations docs in Node

[Nir Lisker]

Correction to the order of transforms specified in the docs of `Node`.

-

Commit messages:
 - Remove whitespace
 - Remove whitespace
 - Initial commit

Changes: https://git.openjdk.java.net/jfx/pull/293/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=293=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252547
  Stats: 37 lines in 1 file changed: 13 ins; 12 del; 12 mod
  Patch: https://git.openjdk.java.net/jfx/pull/293.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/293/head:pull/293

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

Re: RFR: 8217472: Add attenuation for PointLight [v5]

[Nir Lisker]

On Tue, 1 Sep 2020 23:09:39 GMT, Kevin Rushforth  wrote:

>>> gradle -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests MyTest
>> 
>> What format is `MyTest`? Is it some relative path?
>
> It can either be a fully qualified class name (with `.` as separator) or the 
> unqualified name of the test class. The
> class name must end with exactly `Test`. So for example, try it with 
> `Snapshot1Test`.

I wrote the following test:

import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;

import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;

import javafx.application.Application;
import javafx.application.Platform;
import javafx.scene.Group;
import javafx.scene.PointLight;
import javafx.scene.Scene;
import javafx.scene.paint.Color;
import javafx.scene.shape.Box;
import javafx.stage.Stage;
import javafx.stage.WindowEvent;

public class PointLightAttenuationTest {

private static CountDownLatch startupLatch;
private static Stage stage;
private static PointLight light = new PointLight(Color.BLUE);
private static Box box = new Box(100, 100, 1);

@BeforeClass
public static void initFX() throws Exception {
startupLatch = new CountDownLatch(1);
new Thread(() -> Application.launch(TestApp.class, 
(String[])null)).start();
assertTrue("Timeout waiting for FX runtime to start", 
startupLatch.await(15, TimeUnit.SECONDS));
}

public class TestApp extends Application {

@Override
public void start(Stage mainStage) {
stage = mainStage;
light.setTranslateZ(-50);
var root = new Group(light, box);
var scene = new Scene(root);
stage.setScene(scene);
stage.setFullScreen(true);
stage.addEventHandler(WindowEvent.WINDOW_SHOWN, e -> 
Platform.runLater(startupLatch::countDown));
stage.show();
}
}

@Test
public void testAttenuation() {
var image = box.snapshot(null, null);
var nonAttenColor = image.getPixelReader().getColor(1, 1);

light.setLinearAttenuation(2);
image = box.snapshot(null, null);
var attenColor = image.getPixelReader().getColor(1, 1);

System.out.println(nonAttenColor);
System.out.println(attenColor);
if (nonAttenColor.getBlue() > attenColor.getBlue()) {
throw new AssertionError("Attenuation color should be less than 
non-attenuated");
}
}

@AfterClass
public static void teardown() {
Platform.runLater(() -> {
stage.hide();
Platform.exit();
});
}
}
But when executing it with

./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests 
PointLightAttenuationTest

I get the error

test.javafx.scene.lighting3D.PointLightAttenuationTest > classMethod FAILED
java.lang.AssertionError: Timeout waiting for FX runtime to start
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at 
test.javafx.scene.lighting3D.PointLightAttenuationTest.initFX(PointLightAttenuationTest.java:59)

So for some reason the Application doesn't start, it seems. I ran 
`ShapeViewOrderLeakTest` and `RestoreSceneSizeTest`
which look the same, and those work. Any idea?

-

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

Re: RFR: 8217472: Add attenuation for PointLight [v5]

[Nir Lisker]

On Fri, 7 Aug 2020 22:37:15 GMT, Kevin Rushforth  wrote:

>> Given that we don't have any automated tests for lighting (we have a couple 
>> that verify that we can take a snapshot and
>> get the same result, but that isn't testing the lighting itself), probably 
>> the most we can expect is a simple test of a
>> large quad with a light fairly close to the object, such that the difference 
>> with / without attenuation is noticeable.
>> Then you could sample the center and near the corners of the object for both 
>> the attenuated and unattenuated cases.
>
> The performance tests need a standard GPLv2+classpath copyright header. I 
> haven't tested them yet, but will do that
> next week.

I've written a simple draft for the automated test:

PointLight light = new PointLight(Color.BLUE);
light.setTranslateZ(-50);
var box = new Box(100, 100, 1);
var root = new Group(light, box);
var scene = new Scene(root);
var image = box.snapshot(null, null);
var nonAttenColor = image.getPixelReader().getColor(1, 1);
light.setLinearAttenuation(2);
image = box.snapshot(null, null);
var attenColor = image.getPixelReader().getColor(1, 1);
assertTrue("Attenuation color should be less than non-attenuated", 
nonAttenColor.getBlue() > attenColor.getBlue());

However, I'm getting the error:

java.lang.UnsupportedOperationException
at 
javafx.graphics/test.com.sun.javafx.pgstub.StubToolkit.renderToImage(StubToolkit.java:719)
at javafx.graphics/javafx.scene.Scene.doSnapshotTile(Scene.java:1383)
at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1319)
at javafx.graphics/javafx.scene.Node.doSnapshot(Node.java:2136)
at javafx.graphics/javafx.scene.Node.snapshot(Node.java:2214)
Where is the test that takes the snapshot? I am missing something in my setup.

-

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

Re: RFR: 8252547: Correct transformations docs in Node [v2]

[Nir Lisker]

> Correction to the order of transforms specified in the docs of `Node`.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Addressed review comments

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/293/files
  - new: https://git.openjdk.java.net/jfx/pull/293/files/d56f8d02..e09d20f7

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

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

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

Re: RFR: 8252547: Correct transformations docs in Node [v3]

[Nir Lisker]

On Fri, 11 Sep 2020 21:43:46 GMT, Kevin Rushforth  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Added  tags
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 3423:
> 
>> 3421:  *
>> 3422:  * @return the {@code boundsInParent} property for this {@code 
>> Node}
>> 3423:  * @see {@linkplain Node Bounding Rectangles} section in the class 
>> docs
> 
> This causes a javadoc error:
> 
>> Task :javadoc
> modules\javafx.graphics\src\main\java\javafx\scene\Node.java:3423: error: 
> unexpected content
>  * @see {@linkplain Node Bounding Rectangles} section in the class docs
>^
> 1 error
> 
>> Task :javadoc FAILED

`@see` doesn't show for properties because of the special handling (same goes 
for `@return`). I opted to create HTML
links to the headers instead. I did it to all for them even if they are not all 
used.

-

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

Re: RFR: 8252546: Move ObservableValue's equality check and lazy evaluation descriptions to @implSpec [v2]

[Nir Lisker]

> Moving implementation details about lazy evaluation and equality checking to 
> `@implSpec`.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Addressed review comments

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/292/files
  - new: https://git.openjdk.java.net/jfx/pull/292/files/808ba3ce..2a0b1f3b

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

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

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

Re: RFR: 8252546: Move ObservableValue's equality check and lazy evaluation descriptions to @implSpec [v3]

[Nir Lisker]

On Sat, 12 Sep 2020 14:35:49 GMT, Kevin Rushforth  wrote:

>> Marked as reviewed by kcr (Lead).
>
> I see that [SKARA-548](https://bugs.openjdk.java.net/browse/SKARA-548) is 
> still an issue, and that the PR was marked
> ready without the CSR being approved.

Yeah I saw. The CSR has been submitted. I think that the bot isn't picking up 
on it even.

-

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

Re: RFR: 8252547: Correct transformations docs in Node [v3]

[Nir Lisker]

> Correction to the order of transforms specified in the docs of `Node`.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Added  tags

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/293/files
  - new: https://git.openjdk.java.net/jfx/pull/293/files/e09d20f7..0bf63c0b

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

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

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

Re: RFR: 8252547: Correct transformations docs in Node [v4]

[Nir Lisker]

> Correction to the order of transforms specified in the docs of `Node`.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Replaced link

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/293/files
  - new: https://git.openjdk.java.net/jfx/pull/293/files/0bf63c0b..453435ef

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

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

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

Re: RFR: 8252546: Move ObservableValue's equality check and lazy evaluation descriptions to @implSpec [v3]

[Nir Lisker]

> Moving implementation details about lazy evaluation and equality checking to 
> `@implSpec`.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Addressed review comments and added 

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/292/files
  - new: https://git.openjdk.java.net/jfx/pull/292/files/2a0b1f3b..6529dc4e

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

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

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

Re: RFR: 8169501: GIF animation is too fast

[Nir Lisker]

On Tue, 15 Sep 2020 06:41:29 GMT, Bhawesh Choudhary  
wrote:

>> This is pending response to comments above.
>
> 10ms![10ms](https://user-images.githubusercontent.com/4208131/93172831-3c9fcb80-f749-11ea-93ee-46b58ecff4c3.gif)
> 
> 11ms![11ms](https://user-images.githubusercontent.com/4208131/93172833-3dd0f880-f749-11ea-8fa7-5cf2f3cfdcdc.gif)
> 12ms![12ms](https://user-images.githubusercontent.com/4208131/93172834-3e698f00-f749-11ea-92ae-24b3087758d2.gif)
> 15ms![15ms](https://user-images.githubusercontent.com/4208131/93172836-3e698f00-f749-11ea-9b9b-15c5f21dd5af.gif)
> __
> 19ms![19ms](https://user-images.githubusercontent.com/4208131/93172837-3f022580-f749-11ea-84be-7adc712bf230.gif)
> 20ms![20ms](https://user-images.githubusercontent.com/4208131/93172839-3f9abc00-f749-11ea-8d5d-98b4ae131546.gif)
> 21ms![21ms](https://user-images.githubusercontent.com/4208131/93172841-3f9abc00-f749-11ea-9b50-0cb5aa56b525.gif)
> 40ms![40ms](https://user-images.githubusercontent.com/4208131/93172843-40335280-f749-11ea-8572-5bcfae11e28f.gif)
> 
>  __
> 75ms![75ms](https://user-images.githubusercontent.com/4208131/93172846-40cbe900-f749-11ea-8cc2-e20d2ce74947.gif)
> 
> Original![gif](https://user-images.githubusercontent.com/4208131/93172848-41647f80-f749-11ea-88a9-429fa956e428.gif)
> Without the fix, gif animation speed matches for all interval gifs with all 
> other browser (Which includes firefox,
> safari, chrome). Only animation speed for gif file which is attached in issue 
> doesn't match. javafx webkit plays it
> faster. Imageview plays everything at its original speed. No clamping happens 
> here.

Is this related to https://bugs.openjdk.java.net/browse/JDK-8209560?

-

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

Re: RFR: 8252546: Move ObservableValue's equality check and lazy evaluation descriptions to @implSpec [v3]

[Nir Lisker]

On Tue, 15 Sep 2020 15:57:00 GMT, Ambarish Rapte  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Addressed review comments and added 
>
> Marked as reviewed by arapte (Reviewer).

It's not really ready to be integrated, right? It's not finding the CSR I think.

-

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

Re: RFR: 8252546: Move ObservableValue's equality check and lazy evaluation descriptions to @implSpec [v3]

[Nir Lisker]

On Tue, 15 Sep 2020 16:29:58 GMT, Kevin Rushforth  wrote:

>> It's not really ready to be integrated, right? It's not finding the CSR I 
>> think.
>
> Correct. It is not yet ready. 
> [SKARA-548](https://bugs.openjdk.java.net/browse/SKARA-548) is not yet 
> resolved, so shows
> as ready even though it isn't.

The CSR has been approved, I'll go ahead and integrate.

-

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

Integrated: 8252546: Move ObservableValue's equality check and lazy evaluation descriptions to @implSpec

[Nir Lisker]

On Sun, 30 Aug 2020 16:09:14 GMT, Nir Lisker  wrote:

> Moving implementation details about lazy evaluation and equality checking to 
> `@implSpec`.

This pull request has now been integrated.

Changeset: b2e2000c
Author:    Nir Lisker 
URL:   https://git.openjdk.java.net/jfx/commit/b2e2000c
Stats: 11 lines in 1 file changed: 2 ins; 6 del; 3 mod

8252546: Move ObservableValue's equality check and lazy evaluation descriptions 
to @implSpec

Reviewed-by: kcr, arapte

-

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

Re: RFR: 8253086: Optimization of removeAll and retainAll of ObservableListWrapper

[Nir Lisker]

On Mon, 14 Sep 2020 09:57:26 GMT, yosbits 
 wrote:

> https://bugs.openjdk.java.net/browse/JDK-8253086
> 
> ObservableListWrapper.java
>  * public boolean removeAll(Collection c)
> * public boolean retainAll(Collection c)
> 
> These two methods use BitSet, but it doesn't make sense.
> By rewriting to the equivalent behavior that does not use BitSet, it is 
> possible to reduce the CPU load in a wide range
> of use cases.
> The test is done with the following command.
> 
> * gradle base: test
> * gradle controls: test

I will review this.
@kevinrushforth I am not able to self-request a review. Is it on purpose?

-

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

Integrated: 8252547: Correct transformations docs in Node

[Nir Lisker]

On Mon, 31 Aug 2020 23:28:51 GMT, Nir Lisker  wrote:

> Correction to the order of transforms specified in the docs of `Node`.

This pull request has now been integrated.

Changeset: d6dee348
Author:    Nir Lisker 
URL:   https://git.openjdk.java.net/jfx/commit/d6dee348
Stats: 45 lines in 1 file changed: 12 ins; 15 del; 18 mod

8252547: Correct transformations docs in Node

Reviewed-by: kcr, arapte

-

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

Re: RFR: 8252547: Correct transformations docs in Node [v4]

[Nir Lisker]

On Sat, 12 Sep 2020 14:41:01 GMT, Kevin Rushforth  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Replaced link
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 336:
> 
>> 334:  * The transforms are applied in the reverse order of the matrix 
>> multiplication outlined above: last element of
>> the transforms list 335:  * to 0th element, scale, rotate, and layout and 
>> translate. By applying the transforms in this
>> order, the bounds in the local 336:  * coordinates of the node are 
>> transformed to the bounds in the parent coordinate
>> of the node (see the Bounding Rectangles
> 
> Maybe hyperlink to the Bounding Rectangles section? (although as it is right 
> below, it isn't as important).

Originally I had "see the next section" but didn't want to depend on the order. 
I put the link anyway.

-

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

Re: RFR: 8252547: Correct transformations docs in Node [v5]

[Nir Lisker]

> Correction to the order of transforms specified in the docs of `Node`.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Replaced a link

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/293/files
  - new: https://git.openjdk.java.net/jfx/pull/293/files/453435ef..45f9d8aa

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

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

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

Re: Media Player Failure When Speakers Not Present

[Nir Lisker]

>
> PS: If someone can send me the URL for OpenJFX's JIRA or equivalent, I will
> submit a bug report.
>

bugreport.java.com

On Mon, Sep 14, 2020 at 11:03 PM  wrote:

> While I am still looking for where I can file this problem I thought I'd
> bring it to the list. A program I use in my course plays a short video.
> Have
> used it for a few years and it works fine. EXCEPT, if there is not a
> connected audio out device such as speakers or headphones. The video does
> not play and there is no stack trace or any other indication of a problem.
> Stage opens fine but the media control does nothing and the scene appears
> empty. Discovered when I used a machine that did not have speakers plugged
> into it. Expected behaviour was just to see the video without sound. It is
> possible that this bug has been around since the introduction of the Media
> /
> Media Player control.
>
>
>
> I assume the code works like "Hi M. PCaudio, what are you connected to?"
> "I'm not connected to anything, M. Media Player" "That's OK M. PCAudio,
> I'll
> just wait forever or just give up and do nothing".
>
>
>
> Or, it can just be a crazy Xeon based Windows machine that is giving grief
> to OpenJFX. Using Java 14 and OpenJFX 16-ea+1.
>
>
>
> Ken Fogel
>
>
>
> PS: If someone can send me the URL for OpenJFX's JIRA or equivalent, I will
> submit a bug report.
>
>
>
>

Re: Media Player Failure When Speakers Not Present

[Nir Lisker]

The issues database is the same for all OpenJDK projects, including JDK and
OpenJFX. When you submit the issue, it is triaged internally and
transferred to the public JBS at https://bugs.openjdk.java.net.

On Mon, Sep 14, 2020 at 11:27 PM  wrote:

> I am a little confused. The url you sent appears to be for Java SE bug
> reporting and I thought that JavaFX as OpenJFX is not part of the Java SE
> distribution. If so then this may not be where I report my issue.
>
>
>
> Ken
>
>
>
>
>
> *From:* Nir Lisker 
> *Sent:* September 14, 2020 4:19 PM
> *To:* omnip...@gmail.com
> *Cc:* openjfx-dev@openjdk.java.net Mailing 
> *Subject:* Re: Media Player Failure When Speakers Not Present
>
>
>
> PS: If someone can send me the URL for OpenJFX's JIRA or equivalent, I will
> submit a bug report.
>
>
>
> bugreport.java.com
>
>
>
> On Mon, Sep 14, 2020 at 11:03 PM  wrote:
>
> While I am still looking for where I can file this problem I thought I'd
> bring it to the list. A program I use in my course plays a short video.
> Have
> used it for a few years and it works fine. EXCEPT, if there is not a
> connected audio out device such as speakers or headphones. The video does
> not play and there is no stack trace or any other indication of a problem.
> Stage opens fine but the media control does nothing and the scene appears
> empty. Discovered when I used a machine that did not have speakers plugged
> into it. Expected behaviour was just to see the video without sound. It is
> possible that this bug has been around since the introduction of the Media
> /
> Media Player control.
>
>
>
> I assume the code works like "Hi M. PCaudio, what are you connected to?"
> "I'm not connected to anything, M. Media Player" "That's OK M. PCAudio,
> I'll
> just wait forever or just give up and do nothing".
>
>
>
> Or, it can just be a crazy Xeon based Windows machine that is giving grief
> to OpenJFX. Using Java 14 and OpenJFX 16-ea+1.
>
>
>
> Ken Fogel
>
>
>
> PS: If someone can send me the URL for OpenJFX's JIRA or equivalent, I will
> submit a bug report.
>
>
>

Re: RFR: 8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes

[Nir Lisker]

On Sat, 12 Sep 2020 17:16:38 GMT, Kevin Rushforth  wrote:

> This PR updates the `CONTRIBUTING.md` guide to address the following:
> 
> 1. Clarify the process for adding new features / API changes, specifically 
> that they must be discussed on the mailing
> list as the first step. 2. Add a link to the mailing list in the section 
> regarding contributing bug fixes.
> 3. Remove the text about cross-linking the PR and JBS issue, and add a note 
> that the Skara tooling takes care of this
> 4. Remove the section about manually resolving the JBS issue, and add a note 
> that the Skara bot automatically does this
> when the PR is integrated. 5. Suggest the use of the "/reviewers 2" and 
> "/csr" commands when appropriate
> 6. Update the note regarding which JDK(s) to use.

Additional comments:

1. "Use Unix-style (LF) line endings not DOS-style (CRLF)" needs a comma before 
"not".
2. "Line width is no more than 120 characters" I remember that it was 130 or 
135 somewhere.
3. "Wildcard imports (import foo.bar.baz.*) are forbidden" Junit imports use 
them extensively.
4. `./gradlew all test` will cause failure on webkit tests if it was not built.

CONTRIBUTING.md line 18:

> 16: 
> 17:
> 18: All new feature requests, including any API changes, need prior 
> discussion on the
> [openjfx-dev](mailto:openjfx-dev@openjdk.java.net) mailing list, even if 
> there is already an open

I think that the mailing list link shouldn't be to a `mailto` protocol as these 
aren't always configured in the
browser, and in any case, since the intention is to have a discussion, the user 
should be advised to register to the
list and not send a one-time mail. So I suggest to redirect to
https://mail.openjdk.java.net/mailman/listinfo/openjfx-dev.

CONTRIBUTING.md line 19:

> 17:
> 18: All new feature requests, including any API changes, need prior 
> discussion on the
> [openjfx-dev](mailto:openjfx-dev@openjdk.java.net) mailing list, even if 
> there is already an open 19: [JBS
> issue](https://bugs.openjdk.java.net). See the [New features / API 
> additions](#new-features--api-additions) section at
> the end of this guide for more information.

Also for the link under "Bug reports". I think that the issue list link should 
direct to a JavaFX search like
`https://bugs.openjdk.java.net/issues/?jql=component %3D javafx order by 
updated DESC` to make it easier to search.

CONTRIBUTING.md line 24:

> 22: ---
> 23:
> 24: If you have a bug fix or new feature that you would like to contribute to 
> OpenJFX, please find or open an issue
> about it first. Talk about what you would like to do on the 
> [openjfx-dev](mailto:openjfx-dev@openjdk.java.net) mailing
> list. It may be that somebody is already working on it, or that there are 
> particular issues that you should know about
> before implementing the change. Feature requests, in particular, must be 
> discussed ahead of time and will require
> significant effort on your part.

"please find or open an issue about it first"
Shouldn't a discussion happen before a feature is submitted in JBS? Or do we 
close as "won't fix" if the feature is
rejected?

CONTRIBUTING.md line 24:

> 22: ---
> 23:
> 24: If you have a bug fix or new feature that you would like to contribute to 
> OpenJFX, please find or open an issue
> about it first. Talk about what you would like to do on the 
> [openjfx-dev](mailto:openjfx-dev@openjdk.java.net) mailing
> list. It may be that somebody is already working on it, or that there are 
> particular issues that you should know about
> before implementing the change. Feature requests, in particular, must be 
> discussed ahead of time and will require
> significant effort on your part.

Same comment on the "openjfx-dev" link.

CONTRIBUTING.md line 88:

> 86: of the PR title and check for whitespace errors. Once that passes,
> 87: it will automatically send a Request For Review (RFR) email to the
> 88: [openjfx-dev](mailto:openjfx-dev@openjdk.java.net) mailing list.

Same comment on the "openjfx-dev" link.

CONTRIBUTING.md line 96:

> 94: TIP: prefix the pull request title with `WIP:` if you aren't yet
> 95: ready for it to be reviewed. The Skara bot will not send an RFR
> 96: email unless the title starts with a 7-digit bug ID.

I'm pretty sure it doesn't send RFR mails on draft issues regardless of the 
title. I create all my PRs as draft to make
sure everything is correct, and only then transition. I've never seen a 
premature RFR mail, though it could be a
natural delay of he bot.

CONTRIBUTING.md line 100:

> 98: Please adhere to the general guideline that you should never force 
> push
> 99: to a publicly shared branch. Once you have opened your pull request, 
> you
> 100: should consider your branch publicly shared. Instead of force pushing

Minor: this whole paragraph is filled with git terminology like squash, rebase, 
force push... 

Re: RFR: 8217472: Add attenuation for PointLight [v5]

[Nir Lisker]

On Wed, 2 Sep 2020 17:16:00 GMT, Kevin Rushforth  wrote:

>> I wrote the following test:
>> 
>> import static org.junit.Assert.assertTrue;
>> import static org.junit.Assert.fail;
>> 
>> import java.util.concurrent.CountDownLatch;
>> import java.util.concurrent.TimeUnit;
>> 
>> import org.junit.AfterClass;
>> import org.junit.BeforeClass;
>> import org.junit.Test;
>> 
>> import javafx.application.Application;
>> import javafx.application.Platform;
>> import javafx.scene.Group;
>> import javafx.scene.PointLight;
>> import javafx.scene.Scene;
>> import javafx.scene.paint.Color;
>> import javafx.scene.shape.Box;
>> import javafx.stage.Stage;
>> import javafx.stage.WindowEvent;
>> 
>> public class PointLightAttenuationTest {
>> 
>> private static CountDownLatch startupLatch;
>> private static Stage stage;
>> private static PointLight light = new PointLight(Color.BLUE);
>> private static Box box = new Box(100, 100, 1);
>> 
>> @BeforeClass
>> public static void initFX() throws Exception {
>> startupLatch = new CountDownLatch(1);
>> new Thread(() -> Application.launch(TestApp.class, 
>> (String[])null)).start();
>> assertTrue("Timeout waiting for FX runtime to start", 
>> startupLatch.await(15, TimeUnit.SECONDS));
>> }
>> 
>> public class TestApp extends Application {
>> 
>> @Override
>> public void start(Stage mainStage) {
>> stage = mainStage;
>> light.setTranslateZ(-50);
>> var root = new Group(light, box);
>> var scene = new Scene(root);
>> stage.setScene(scene);
>> stage.setFullScreen(true);
>> stage.addEventHandler(WindowEvent.WINDOW_SHOWN, e -> 
>> Platform.runLater(startupLatch::countDown));
>> stage.show();
>> }
>> }
>> 
>> @Test
>> public void testAttenuation() {
>> var image = box.snapshot(null, null);
>> var nonAttenColor = image.getPixelReader().getColor(1, 1);
>> 
>> light.setLinearAttenuation(2);
>> image = box.snapshot(null, null);
>> var attenColor = image.getPixelReader().getColor(1, 1);
>> 
>> System.out.println(nonAttenColor);
>> System.out.println(attenColor);
>> if (nonAttenColor.getBlue() > attenColor.getBlue()) {
>> throw new AssertionError("Attenuation color should be less than 
>> non-attenuated");
>> }
>> }
>> 
>> @AfterClass
>> public static void teardown() {
>> Platform.runLater(() -> {
>> stage.hide();
>> Platform.exit();
>> });
>> }
>> }
>> But when executing it with
>> 
>> ./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests 
>> PointLightAttenuationTest
>> 
>> I get the error
>> 
>> test.javafx.scene.lighting3D.PointLightAttenuationTest > classMethod FAILED
>> java.lang.AssertionError: Timeout waiting for FX runtime to start
>> at org.junit.Assert.fail(Assert.java:91)
>> at org.junit.Assert.assertTrue(Assert.java:43)
>> at 
>> test.javafx.scene.lighting3D.PointLightAttenuationTest.initFX(PointLightAttenuationTest.java:59)
>> 
>> So for some reason the Application doesn't start, it seems. I ran 
>> `ShapeViewOrderLeakTest` and `RestoreSceneSizeTest`
>> which look the same, and those work. Any idea?
>
> If you run it with `--info` you will see something like this:
> 
> test.javafx.scene.shape.PointLightAttenuationTest STANDARD_ERROR
> Exception in Application constructor
> Exception in thread "Thread-4" java.lang.RuntimeException: Unable to 
> construct Application instance: class
> test.javafx.scene.shape.PointLightAttenuationTest$TestApp
> at 
> javafx.graphics/com.sun.javafx.application.LauncherImpl.launchApplication1(LauncherImpl.java:890)
> at 
> javafx.graphics/com.sun.javafx.application.LauncherImpl.lambda$launchApplication$2(LauncherImpl.java:195)
> at java.base/java.lang.Thread.run(Thread.java:832)
> Caused by: java.lang.NoSuchMethodException: 
> test.javafx.scene.shape.PointLightAttenuationTest$TestApp.()
> at java.base/java.lang.Class.getConstructor0(Class.java:3427)
> at java.base/java.lang.Class.getConstructor(Class.java:2165)
> at 
> javafx.graphics/com.sun.javafx.application.LauncherImpl.lambda$launchApplication1$8(LauncherImpl.java:801)
> at 
> javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runAndWait$12(PlatformImpl.java:455)
> at 
> javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:428)
> at 
> java.base/java.security.AccessController.doPrivileged(AccessController.java:391)
> at 
> javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:427)
> at 
> javafx.graphics/com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:96)
> at 
> 

Re: RFR: 8217472: Add attenuation for PointLight [v5]

[Nir Lisker]

On Tue, 1 Sep 2020 11:42:30 GMT, Kevin Rushforth  wrote:

>> I've written a simple draft for the automated test:
>> 
>> PointLight light = new PointLight(Color.BLUE);
>> light.setTranslateZ(-50);
>> var box = new Box(100, 100, 1);
>> var root = new Group(light, box);
>> var scene = new Scene(root);
>> var image = box.snapshot(null, null);
>> var nonAttenColor = image.getPixelReader().getColor(1, 1);
>> light.setLinearAttenuation(2);
>> image = box.snapshot(null, null);
>> var attenColor = image.getPixelReader().getColor(1, 1);
>> assertTrue("Attenuation color should be less than non-attenuated", 
>> nonAttenColor.getBlue() > attenColor.getBlue());
>> 
>> However, I'm getting the error:
>> 
>> java.lang.UnsupportedOperationException
>>  at 
>> javafx.graphics/test.com.sun.javafx.pgstub.StubToolkit.renderToImage(StubToolkit.java:719)
>>  at javafx.graphics/javafx.scene.Scene.doSnapshotTile(Scene.java:1383)
>>  at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1319)
>>  at javafx.graphics/javafx.scene.Node.doSnapshot(Node.java:2136)
>>  at javafx.graphics/javafx.scene.Node.snapshot(Node.java:2214)
>> Where is the test that takes the snapshot? I am missing something in my 
>> setup.
>
> I guess you are trying a unit test in the `javafx.graphics` project? That 
> won't work. It needs to be a system test in
> `tests/system/...`

I tried several approaches. When I put it under `tests/system` and run the 
tests task, I don't see it being run. I'm
not familiar with the way tests under `tests` are run in general.

-

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

Re: RFR: 8217472: Add attenuation for PointLight [v5]

[Nir Lisker]

On Fri, 4 Sep 2020 14:34:40 GMT, Nir Lisker  wrote:

>> If you run it with `--info` you will see something like this:
>> 
>> test.javafx.scene.shape.PointLightAttenuationTest STANDARD_ERROR
>> Exception in Application constructor
>> Exception in thread "Thread-4" java.lang.RuntimeException: Unable to 
>> construct Application instance: class
>> test.javafx.scene.shape.PointLightAttenuationTest$TestApp
>> at 
>> javafx.graphics/com.sun.javafx.application.LauncherImpl.launchApplication1(LauncherImpl.java:890)
>> at 
>> javafx.graphics/com.sun.javafx.application.LauncherImpl.lambda$launchApplication$2(LauncherImpl.java:195)
>> at java.base/java.lang.Thread.run(Thread.java:832)
>> Caused by: java.lang.NoSuchMethodException: 
>> test.javafx.scene.shape.PointLightAttenuationTest$TestApp.()
>> at java.base/java.lang.Class.getConstructor0(Class.java:3427)
>> at java.base/java.lang.Class.getConstructor(Class.java:2165)
>> at 
>> javafx.graphics/com.sun.javafx.application.LauncherImpl.lambda$launchApplication1$8(LauncherImpl.java:801)
>> at 
>> javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runAndWait$12(PlatformImpl.java:455)
>> at 
>> javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:428)
>> at 
>> java.base/java.security.AccessController.doPrivileged(AccessController.java:391)
>> at 
>> javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:427)
>> at 
>> javafx.graphics/com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:96)
>> at 
>> javafx.graphics/com.sun.glass.ui.win.WinApplication._runLoop(Native Method)
>> at 
>> javafx.graphics/com.sun.glass.ui.win.WinApplication.lambda$runLoop$3(WinApplication.java:174)
>> ... 1 more
>> 
>> The nested `TestApp` class should be declared as `static`. Btw, I don't 
>> recommend using `setFullScreen` for a test such
>> as this.
>
> Thanks, the test runs now, but I've run into an issue with `snapshot`.
> 
> `Node#snapshot` uses its scene parameters (like lights) to render the image, 
> but if the node is in a subscene then the
> image will be wrong since it takes the wrong data. Bounds transforms like 
> `sceneToLocal` and `localToScene` do take
> care of this case. I think that this is a mistake in `snapshot`, and maybe it 
> should use the subscene, or a `boolean
> subScene` parameter should be offered, or `SnapshotParameters` should allow 
> to specify it.

Looks like there's more to it. I've created the test code:

import javafx.application.Application;
import javafx.scene.Group;
import javafx.scene.PerspectiveCamera;
import javafx.scene.PointLight;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Slider;
import javafx.scene.image.ImageView;
import javafx.scene.layout.BorderPane;
import javafx.scene.paint.Color;
import javafx.scene.shape.Box;
import javafx.stage.Stage;

public class PointLightAttenuationTest extends Application {

public static void main(String[] args) throws Exception {
launch(args);
}

@Override
public void start(Stage stage) {
var light = new PointLight(Color.BLUE);
light.setTranslateZ(-10);
Box box = new Box(100, 100, 1);
var group = new Group(light, box);

var scene = new Scene(group, 500, 500, true);
var camera = new PerspectiveCamera(true);
camera.setTranslateZ(-300);
camera.setFarClip(1000);
scene.setCamera(camera);
stage.setScene(scene);
stage.show();

var imageView = new ImageView();
var snapshotButton = new Button("Node Snaphot");
snapshotButton.setOnAction(e -> 
imageView.setImage(scene.snapshot(null)));

var slider = new Slider(0, 0.2, 0);
slider.setShowTickMarks(true);
slider.setShowTickLabels(true);

light.linearAttenuationProperty().bindBidirectional(slider.valueProperty());

var snapshotStage = new Stage();
snapshotStage.setScene(new Scene(new BorderPane(imageView, 
snapshotButton, null, slider, null)));
snapshotStage.setWidth(600);
snapshotStage.setHeight(600);
snapshotStage.show();
}
}
If the line `snapshotButton.setOnAction(e -> 
imageView.setImage(scene.snapshot(null)));`  is replaced to use
`box.snapshot(null, null)` then the snapshot is wrong.

-

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

Re: RFR: 8217472: Add attenuation for PointLight [v5]

[Nir Lisker]

On Thu, 8 Oct 2020 22:08:16 GMT, Kevin Rushforth  wrote:

>> In that case, it seems like a generally useful optimization (not just at 
>> initialization) to send down `maxRange` as 0
>> whenever `ca`, `la`, and `qa` are all at their default values.
>
> Actually, my above comment is wrong. A `maxRange` of 0 will effectively 
> disable the lighting, even in the case whether
> the other three values are 1, 0, 0. So `maxRange` should be set to 
> `Float.POSITIVE_INFINITY` (or else just use the
> default constants).

These lights are fillers for the native lights array in the case where the user 
set less than 3 lights, they are not
supposed to do anything, and their color is black. A range of 0 will guarantee 
that the computation in the shader will
be skipped.

In a WIP that I have locally I have removed the "3 lights constraint" 
altogether, so this part will change in that
proposed patch anyway.

-

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

Re: RFR: 8217472: Add attenuation for PointLight [v14]

[Nir Lisker]

On Thu, 8 Oct 2020 22:21:06 GMT, Kevin Rushforth  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Change range after clamping
>
> modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.cc line 64:
> 
>> 62: }
>> 63:
>> 64: /*void D3DLight::setRange(float r) {
> 
> Remove this unused function?

I forgot to mention this point. These setter functions for color and position 
(and range) are never used since whenever
there is a change in the java side the whole array and lights are recreated 
instead of being adjusted for the change.
I'm not familiar enough with the memory model of JNI, but it seems expensive to 
re-render everything on every change (I
think that the mesh is also recreated every time in the native code). Is this 
the way it's supposed to work?

-

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

Re: RFR: 8217472: Add attenuation for PointLight [v15]

[Nir Lisker]

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

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Addressed review comments

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/43/files
  - new: https://git.openjdk.java.net/jfx/pull/43/files/21f91dd7..e1fdf8e2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=43=14
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=43=13-14

  Stats: 36 lines in 3 files changed: 25 ins; 4 del; 7 mod
  Patch: https://git.openjdk.java.net/jfx/pull/43.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/43/head:pull/43

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

Re: RFR: 8217472: Add attenuation for PointLight [v14]

[Nir Lisker]

On Thu, 8 Oct 2020 22:20:17 GMT, Kevin Rushforth  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Change range after clamping
>
> modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.cc line 49:
> 
>> 47: attenuation[1] = 0;
>> 48: attenuation[2] = 0;
>> 49: maxRange = 0;
> 
> Same comment as in `NGShape3D` about using infinity here, although if this 
> value is always set from the Java side, then
> this native default might not matter.

Yes, these values are always overridden.

-

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

Re: RFR: 8217472: Add attenuation for PointLight [v16]

[Nir Lisker]

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

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Fixed test

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/43/files
  - new: https://git.openjdk.java.net/jfx/pull/43/files/e1fdf8e2..475e1bd2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=43=15
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=43=14-15

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

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

Default value of Spinner editable property

[Nir Lisker]

Hi,

Spinner's editable property seems to have conflicting initial values.
isEditable() returns true on an uninitialized property, but initializing it
sets its default to false:

   private BooleanProperty editable;
public final void setEditable(boolean value) {
editableProperty().set(value);
}
public final boolean isEditable() {
return editable == null ? true : editable.get(); // < true
}
public final BooleanProperty editableProperty() {
if (editable == null) {
editable = new SimpleBooleanProperty(this, "editable", false);
// < false
}
return editable;
}

Seeme like a bug. What is the correct default?

- Nir

Re: Default value of Spinner editable property

[Nir Lisker]

I noticed now that the class docs specify that the default is false.
Should isEditable() be changed to return false on a null property?

On Fri, Oct 16, 2020 at 2:59 PM Nir Lisker  wrote:

> Hi,
>
> Spinner's editable property seems to have conflicting initial values.
> isEditable() returns true on an uninitialized property, but initializing it
> sets its default to false:
>
>private BooleanProperty editable;
> public final void setEditable(boolean value) {
> editableProperty().set(value);
> }
> public final boolean isEditable() {
> return editable == null ? true : editable.get(); // < true
> }
> public final BooleanProperty editableProperty() {
> if (editable == null) {
> editable = new SimpleBooleanProperty(this, "editable", false);
> // < false
> }
> return editable;
> }
>
> Seeme like a bug. What is the correct default?
>
> - Nir
>

Re: Default value of Spinner editable property

[Nir Lisker]

I created https://github.com/openjdk/jfx/pull/323.

On Sat, Oct 17, 2020 at 5:21 PM Kevin Rushforth 
wrote:

> Yes, this looks like a bug. Would you mind filing it? We could add an
> "@defaultValue false" to the property docs as well. The class docs do
> indicate false (as you noted), but the property docs are silent on the
> default value.
>
> It looks like the only reason it hasn't caused problems for us is that
> the Spinner constructor calls editableProperty() which initializes the
> editable field (so the incorrect default value is not used). Still, it
> would be good to fix this, since it is a bug waiting to happen if that
> initialization were to ever change.
>
> -- Kevin
>
>
> On 10/16/2020 5:06 AM, Nir Lisker wrote:
> > I noticed now that the class docs specify that the default is false.
> > Should isEditable() be changed to return false on a null property?
> >
> > On Fri, Oct 16, 2020 at 2:59 PM Nir Lisker  wrote:
> >
> >> Hi,
> >>
> >> Spinner's editable property seems to have conflicting initial values.
> >> isEditable() returns true on an uninitialized property, but
> initializing it
> >> sets its default to false:
> >>
> >> private BooleanProperty editable;
> >>  public final void setEditable(boolean value) {
> >>  editableProperty().set(value);
> >>  }
> >>  public final boolean isEditable() {
> >>  return editable == null ? true : editable.get(); // < true
> >>  }
> >>  public final BooleanProperty editableProperty() {
> >>  if (editable == null) {
> >>  editable = new SimpleBooleanProperty(this, "editable",
> false);
> >> // < false
> >>  }
> >>  return editable;
> >>  }
> >>
> >> Seeme like a bug. What is the correct default?
> >>
> >> - Nir
> >>
>
>

RFR: 8254964: Fix default values in Spinner class

[Nir Lisker]

Added 2 `@defaultValue` and fixed the incorrect default for `editable`. Also 
corrected a typo that doesn't show anyway
in the docs.

-

Commit messages:
 - Initial commit

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

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

Re: RFR: 8217472: Add attenuation for PointLight [v16]

[Nir Lisker]

On Fri, 9 Oct 2020 18:11:14 GMT, Kevin Rushforth  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fixed test
>
> tests/performance/3DLighting/attenuation/LightingSample.java line 62:
> 
>> 60: environment.setStyle("-fx-background-color: teal");
>> 61:
>> 62: var subdivisionSlider = new Slider(10, 200, 60);
> 
> I might recommend setting the max value of the slider to something like 1000, 
> since even on a slow system, it runs
> quite nicely at that tessellation.

I forgot to reply to this and the other comment. Since we're going to implement 
more light types, this small test app
is going to be updated anyway, so I will change the values then.

-

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

Integrated: 8217472: Add attenuation for PointLight

[Nir Lisker]

On Sun, 17 Nov 2019 04:15:34 GMT, Nir Lisker  wrote:

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

This pull request has now been integrated.

Changeset: aab26d93
Author:    Nir Lisker 
URL:   https://git.openjdk.java.net/jfx/commit/aab26d93
Stats: 1223 lines in 33 files changed: 1115 ins; 13 del; 95 mod

8217472: Add attenuation for PointLight

Reviewed-by: arapte, kcr

-

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

Re: RFR: 8217472: Add attenuation for PointLight [v15]

[Nir Lisker]

On Fri, 9 Oct 2020 12:34:08 GMT, Kevin Rushforth  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Addressed review comments
>
> tests/system/src/test/java/test/javafx/scene/lighting3D/PointLightAttenuationTest.java
>  line 135:
> 
>> 133: @AfterClass
>> 134: public static void teardown() {
>> 135: Platform.runLater(() -> {
> 
> I forgot to mention that the `@AfterClass` method is still run even if the 
> call to `assumeTrue` in the `@BeforeClass`
> method causes the test to be skipped. You will need to repeat the check (or 
> set a flag in `initFX` and check that flag)
> here and skip the cleanup if the initialization was skipped (using an `if` 
> check, not repeating the `assumeTrue`).
> Otherwise the exception that is thrown here will cause the test to fail.  You 
> can test this by forcing the SW pipeline.
> I did it like this:  _JAVA_OPTIONS="-Dprism.order=sw" gradle --info 
> -PFULL_TEST=true :systemTests:test --tests
> PointLightAttenuationTest

The test fails even with this addition. I found that I need to use `@Before` to 
run the check, and then I don't need
the ones in `@BeforeClass` and `@AfterClass`, should I keep those?

-

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

Re: RFR: 8253086: Optimization of removeAll and retainAll of ObservableListWrapper [v4]

[Nir Lisker]

On Mon, 5 Oct 2020 19:22:41 GMT, Kevin Rushforth  wrote:

>>> The listView test is passing for the bitSet and for the back-to-front 
>>> approach. Can we imagine a context where the
>>> broken selectedItems impl would add wreckage to the latter?
>> 
>> To answer my own question: yes. A failing test with the back-to-front 
>> approach (the existing test in ListViewTest
>> modified in having the last item selected)
>> @Test public void test_rt35857Back() {
>> ObservableList fxList = 
>> FXCollections.observableArrayList("A", "B", "C");
>> final ListView listView = new ListView(fxList);
>> 
>> listView.getSelectionModel().select(2);
>> 
>> ObservableList selectedItems =
>> listView.getSelectionModel().getSelectedItems();
>> assertEquals(1, selectedItems.size());
>> assertEquals("C", selectedItems.get(0));
>> 
>> listView.getItems().removeAll(selectedItems);
>> assertEquals(2, fxList.size());
>> assertEquals("A", fxList.get(0));
>> assertEquals("B", fxList.get(1));
>> }
>> 
>> Feels like coming nearer to the why of the BitSet approach: it guards 
>> against the scenario where changing the current
>> list implicitly changes the list given as parameter - in that case, it's 
>> unsafe to query the parameter list while
>> removing items (c.contains simply reports nonsense).  This may happen 
>> whenever the parameter list does some kind of
>> live lookup into the current list (such as f.i. 
>> SelectedItemsReadOnlyObservableList) - which is not as evil as I
>> thought yesterday (did it myself in custom selectionModels ;) The BitSet 
>> solves it by a two-pass approach: first
>> collect all items to remove/retain without (that is keep the parameter list 
>> in a valid state, allowing to safely access
>> it), then do the removal without further accessing the (then invalid) 
>> parameter list.   The fix at that time was a
>> deliberate decision by the designer of the collections, so the context when 
>> it happens was deemed a valid use-case.
>> Looks like we should **not** revert it.
>
> Nice catch! So now it's clear that the current approach was adopted because 
> the source list itself can change as a
> result of removing an element from the target list in some cases, such as the 
> one you pointed out above. I filed
> [JDK-8254040](https://bugs.openjdk.java.net/browse/JDK-8254040) to add the 
> test case you listed above so we avoid a
> possible future regression.  So a two-pass algorithm is still needed: the 
> first one to collect the elements to be
> removed, the second to actually remove them. While it isn't necessary to use 
> a BitSet to collect the indexes to be
> removed, that does seems a reasonable approach. Unless there is a good reason 
> to change it to some other two-pass
> algorithm, it's probably best to leave it as-is, in which case this PR and 
> the associated JBS issue can be withdrawn.

@kleopatra Thanks for the investigation. I had a feeling there is a practical 
reason and not a lack of skill :)

If there is no way to do removal simultaneously then we indeed need to keep the 
2-pass approach.

-

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

Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]

[Nir Lisker]

On Tue, 18 Aug 2020 23:47:07 GMT, Kevin Rushforth  wrote:

>> modules/javafx.graphics/src/main/java/javafx/css/PseudoClass.java line 83:
>> 
>>> 82:
>>> 83: /**
>>> 84:  * There is only one PseudoClass instance for a given pseudoClass.
>> 
>> 1. Is having a public constructor for this class a good idea? Instances are 
>> created by a factory method.
>> 2. Both methods in this class need documentation update. `getPseudoClass` 
>> has a poor method description and the
>> formatting of the `@return` tag is wrong (lowercase and no period). 
>> `getPseudoClassName` is missing a description.
>
> Yes, this constructor is needed. `PseudoClass` is abstract, so it's 
> constructor is just there for subclasses to call
> (note that for a constructor of an abstract class, `public` and `protected` 
> mean the same thing).
> As for the other methods in the class, I agree that they should be 
> changed...but in a follow-up issue.

Maybe we can add the method docs fixes to 
[JDK-8250590](https://bugs.openjdk.java.net/browse/JDK-8250590)?

-

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

Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]

[Nir Lisker]

On Wed, 19 Aug 2020 00:12:01 GMT, Kevin Rushforth  wrote:

> I think that two of the classes have implicit constructors that are there by 
> accident. Once we get agreement, I'll file
> a follow-on bug for those, and those changes should be reverted.

I finished reviewing the rest of the classes and I had no additional comments 
about them, so I agree about deprecating
for removal those 2 classes' constructors.

> As for the other comments, I would prefer a follow-up bug for any doc cleanup 
> that isn't part of adding in an explicit
> constructor. Tempting as it might be to fix it, it seems out of scope.

That's fine. I left inline comments about those.

> That leaves the question about the wording. The more I think about it the 
> more I like what the JDK did as opposed to
> what we did. The question then becomes: if we agree that it's a better 
> pattern, do we adopt it here and then clean it
> up for the other two batches or just leave it as is for now, and file one 
> cleanup bug for later. I'd like to hear from
> @aghaisas and @nlisker

Since it will require an additional cleanup issue anyway, I don't think it 
matters when we do it, but since we're here
I see no reason not to start already.

-

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

Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]

[Nir Lisker]

On Wed, 19 Aug 2020 00:12:01 GMT, Kevin Rushforth  wrote:

>> Bhawesh Choudhary has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Marked few class constructor as deprecated as per review
>
> I think that two of the classes have implicit constructors that are there by 
> accident. Once we get agreement, I'll file
> a follow-on bug for those, and those changes should be reverted.
> As for the other comments, I would prefer a follow-up bug for any doc cleanup 
> that isn't part of adding in an explicit
> constructor. Tempting as it might be to fix it, it seems out of scope.
> That leaves the question about the wording. The more I think about it the 
> more I like what the JDK did as opposed to
> what we did. The question then becomes: if we agree that it's a better 
> pattern, do we adopt it here and then clean it
> up for the other two batches or just leave it as is for now, and file one 
> cleanup bug for later. I'd like to hear from
> @aghaisas and @nlisker

@bhaweshkc You can't deprecate the constructors in this PR since it will need a 
separate CSR. You should revert the
changes on these classes completely and file a new bug (and CSR) dealing with 
deprecations.

-

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

Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]

[Nir Lisker]

On Tue, 18 Aug 2020 23:31:42 GMT, Kevin Rushforth  wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/TableFocusModel.java
>>  line 51:
>> 
>>> 50:
>>> 51: /**
>>> 52:  * Causes the item at the given index to receive the focus.
>> 
>> Please add a missing `)` for the class docs on line 30.
>
> The change to the class header seems unrelated. It would be a good thing to 
> fix, but could be done as part of a
> follow-on doc cleanup issue.

Alright, we could add them to this version's doc fixes issue.

-

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

Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v2]

[Nir Lisker]

On Tue, 18 Aug 2020 23:41:23 GMT, Kevin Rushforth  wrote:

>> modules/javafx.graphics/src/main/java/javafx/application/Preloader.java line 
>> 121:
>> 
>>> 120: public Preloader() {
>>> 121: }
>>> 122:
>> 
>> Not sure that "default" means anything here. I don't see any configuration.
>
> Right, but isn't that true of most of the classes, including those in the two 
> related bugs that were fixed?
> 
> Worth noting is that the JDK chose different language for abstract classes 
> than concrete ones. For abstract classes,
> they just use the following language:
> * Constructor for subclasses to call.
> 
> And for concrete ones:
> 
> Constructs a {@code Foo}.
> 
> This gets around the problem of whether or not `default` adds any useful 
> information since it is the only constructor.
> Not saying we should adopt this now, but just adding another data point.

I didn't look closely at whether "default" was suitable in the previous cases. 
I like the JDK's wording, but in cases
where there are constructors that allow configuration, the empty constructor 
could use "default", though specifying
what the default values are is more beneficial anyway.

-

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

Re: Make TransformationList (Filtered and Sorted) extendable

[Nir Lisker]

Hi Tobias,

I thought that ReactFX superseded EasyBind and InhiBeans. What is the
current relation between these?
In any case, while removing the final modifier could work (I didn't look
into it much), I suggest to take a broader approach. These libraries have
many features that I think are desirable to add to JavaFX, and they could
be added cleanly considering that we don't need to jump through the hoops
that a 3rd party library needs to. For example, the Val and Var extensions
could be integrated seamlessly (more or less) into the current classes,
solving problems such as the current "select" mechanism and wrong recursive
behavior.

I suggest to look at an approach where we adapt these libraries for javaFX
rather than adapting JavaFX for these libraries. If you are familiar with
these libraries, can we try to form a broader plan and see what meets the
bar?

- Nir

On Sun, Aug 16, 2020 at 4:29 PM Tobias Diez  wrote:

> Hello everybody,
>
> Right now it is hard to extend the FilteredList and the SortedList as these
> classes are marked final. This makes it practically impossible to add
> convenient helper methods, or change or extend the behavior of these
> classes.
> I agree that normally you don't need to extend them, but I also don't see
> any reason why you shouldn't be allowed to do so.
>
> Usually, you could use composition over inheritance if a class is marked
> final but you still want to extend it. However, some of the controls expect
> really a SortedList , e.g.
>
> https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/
> java/javafx/scene/control/TableView.java#L442
> 
> So this approach does not work either.
>
> The motivation for me comes from some limitations that we encountered while
> developing EasyBind, a small library to make bindings easier. We provide a
> few convenient methods that extend the ObservableList, and we want to
> provide a fluent interface similar to streams, e.g.
> EasyBind.wrap(standard observable list).filter(predicate).map(mapper).
> For this to work, the filter method needs to return a FilteredList with the
> additional map method. The easiest solution would be to derive from
> FilteredList, and implement the "EasyObservableList" interface that we
> have.
>
> Removing the final keyword is done in the following PR:
> https://github.com/openjdk/jfx/pull/278
>
> Looking forward to your feedback.
> Best regards
> Tobias
>
>

Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors

[Nir Lisker]

On Mon, 17 Aug 2020 11:16:55 GMT, Bhawesh Choudhary  
wrote:

> Added missing explicit no-arg constructors to classes in package 
> javafx.scene, javafx.css and javafx.stage.

I've completed a partial review. I think that just mechanically adding a 
constructor with the same doc everywhere is
not a good approach. The classes should be inspected to see if one is needed 
and if its doc is suitable. See my
comments on some of the classes I've looked at.

modules/javafx.controls/src/main/java/javafx/scene/control/TableFocusModel.java 
line 51:

> 50:
> 51: /**
> 52:  * Causes the item at the given index to receive the focus.

Please add a missing `)` for the class docs on line 30.

modules/javafx.controls/src/main/java/javafx/scene/control/TableSelectionModel.java
 line 46:

> 45:
> 46: /**
> 47:  * Convenience function which tests whether the given row and column 
> index

Same missing `)`

modules/javafx.graphics/src/main/java/javafx/css/PseudoClass.java line 83:

> 82:
> 83: /**
> 84:  * There is only one PseudoClass instance for a given pseudoClass.

1. Is having a public constructor for this class a good idea? Instances are 
created by a factory method.
2. Both methods in this class need documentation update. `getPseudoClass` has a 
poor method description and the
formatting of the `@return` tag is wrong (lowercase and no period). 
`getPseudoClassName` is missing a description.

modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 51:

> 50:
> 51: private static class UniversalSelector {
> 52: private static final Selector INSTANCE =

Is a public constructor suitable in this class? `createSelector(String)` is the 
factory. There are public abstract
methods here, on the other hand, so I don't know what the design idea is. The 
class docs should be updated too to say
how to use this class.

modules/javafx.graphics/src/main/java/javafx/css/StyleConverter.java line 95:

> 94:
> 95: /**
> 96:  * Convert from the parsed CSS value to the target property type.

Looks like a constructor is fine here if the predefined factories are not 
suitable, but I'm not familiar with this.

modules/javafx.graphics/src/main/java/javafx/css/converter/ShapeConverter.java 
line 51:

> 50: }
> 51:
> 52: @Override public Shape convert(ParsedValue value, Font 
> font) {

Looks like a singleton class.

modules/javafx.graphics/src/main/java/javafx/scene/input/ClipboardContent.java 
line 45:

> 44:  */
> 45: public ClipboardContent() {
> 46: }

Not sure that "default" means anything here. I don't see any configuration.

modules/javafx.graphics/src/main/java/javafx/application/Preloader.java line 
121:

> 120: public Preloader() {
> 121: }
> 122:

Not sure that "default" means anything here. I don't see any configuration.

-

Changes requested by nlisker (Reviewer).

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

Re: RFR: 8252387: Deprecate for removal css Selector and ShapeConverter constructors

[Nir Lisker]

On Fri, 28 Aug 2020 14:55:55 GMT, Bhawesh Choudhary  
wrote:

> Deprecate the public constructor of javafx.css.Selector as it should not be 
> public due to only being extended by
> classes in same package. Deprecate the public constructor of 
> javafx.css.converter.ShapeConverter as its a singleton
> class.

Looks good. Added a single comment.

modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 47:

> 46:  * @deprecated This constructor was exposed erroneously and will be 
> removed in the next version.
> 47:  */
> 48: @Deprecated(since="16", forRemoval=true)

Please direct the reader to the way of obtaining an instance of this class like 
you did for `ShapeConverter`.

-

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

Re: Release notes presentation

[Nir Lisker]

Reviving this, the JDK has a page for release notes [1] that makes it
easier for the developer to be updated on the changes. I think that we can
do something similar.

[1] https://www.oracle.com/java/technologies/javase/14-relnote-issues.html

On Mon, Mar 9, 2020 at 2:56 PM Jeanette Winzenburg 
wrote:

>
> agree with better highlighting of the important changes/issues
> disagree with leaving out the not so important - that's in the eye of
> the reader, anyway - changes/issues: after all, the target readership
> are technicians, not suits :)
>
> -- Jeanette
>
> Zitat von Nir Lisker :
>
> > Hi,
> >
> > I posted the release notes for OpenJFX 14 on Reddit. The top comment [1]
> > complained about the presentation. Maybe on openjfx.io the Release Notes
> > link could show a more "friendly" summary with a link to the detailed
> > table. I suggest:
> >
> > 1. Including only changes that matter to the users of the library with
> > separate sections for new API and for (non-internal) bug fixes.
> > 2. Highlighting significant changes.
> >
> > Some example bug fixes to include:
> > - TableView, ListView: unexpected scrolling behaviour on up/down keys
> > - DragAndDrop no longer works with GTK3
> > - Window order is not correct when Modality.WINDOW_MODAL
> >
> > Don't include:
> > - [WebView] Sub-resource integrity check fails on Windows and Linux
> > - javafx.web build fails on XCode 10.2
> > - Cherry pick GTK WebKit ___ changes
> >
> > Enhancements to include:
> > - Add support for e-paper displays
> > - Add exclusion scope for LightBase
> > - Point2D and Point3D should implement Interpolatable
> >
> > Don't include:
> > -  Color, Point2D and Point3D's fields should be made final
> >
> > Thoughts?
> >
> > - Nir
> >
> > [1]
> >
> https://www.reddit.com/r/java/comments/fegcv9/release_notes_for_javafx_14/fjphqfn?utm_source=share_medium=web2x
>
>
>
>

Re: RFR: 8252387: Deprecate for removal css Selector and ShapeConverter constructors [v2]

[Nir Lisker]

On Sat, 29 Aug 2020 14:35:58 GMT, Kevin Rushforth  wrote:

>> Looks good. Please update the CSR to reflect the change in the docs Selector.
>
> This should not have been marked as ready by the Skara bot. Looks like there 
> is a bug in the processing of the `/csr`
> since the CSR is not yet approved.
> @bhaweshkc please wait until the CSR is approved before integrating.

@kevinrushforth Why is the Skara bot saying this PR can be integrated if the 
CSR is not approved yet?

Also, in the main post under Reviewers, the links to the users lead to a 404.

Should I submit bugs for these?

-

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

Re: RFR: 8252387: Deprecate for removal css Selector and ShapeConverter constructors [v2]

[Nir Lisker]

On Fri, 28 Aug 2020 17:26:41 GMT, Bhawesh Choudhary  
wrote:

>> Deprecate the public constructor of javafx.css.Selector as it should not be 
>> public due to only being extended by
>> classes in same package. Deprecate the public constructor of 
>> javafx.css.converter.ShapeConverter as its a singleton
>> class.
>
> Bhawesh Choudhary has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Updated Selector class comments as per review

Marked as reviewed by nlisker (Reviewer).

-

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

Re: Release notes presentation

[Nir Lisker]

Any of those options seem fine to me. Just have the "marketable" summary
link to the full table if they are not on the same page.

On Sat, Aug 29, 2020 at 6:24 PM Kevin Rushforth 
wrote:

> I also like having both. We did something like this for JavaFX 11:
>
> https://github.com/openjdk/jfx/blob/master/doc-files/release-notes-11.md
>
> with sections for "Important Changes", "New Features", "Known Issues",
> etc., and the list of fixes and enhancements at the end.
>
> We haven't done this for subsequent releases, but it seems worth doing,
> either on openjfx.io or in the release notes in the repo.
>
> -- Kevin
>
> On 8/29/2020 8:02 AM, Johan Vos wrote:
> > Hi Nir,
> >
> > I am very ok with that. I think we somehow need both. Currently, the
> > release notes that are also in the repository are a dry summary of the
> bug
> > fixes/features that have been added in this release. They are a 1-1
> > translation of the issues in JBS that are fixed in the given release. I
> > think that is good to have, as it allows for an easy lookup of what
> feature
> > was in which release.
> >
> > Apart from that, I'm often asked what the "key features" are for a
> specific
> > release. That is of course very subjective, but from a marketing point,
> or
> > even for developers, it makes sense to have those release notes as well.
> I
> > don't think those should go in the github repo, but they can definitely
> go
> > on openjfx.io.
> >
> > Those "friendly" release notes will always be subjective, but I don't see
> > big problems with that. In case of doubt, the "official" (dry) release
> > notes are objective.
> >
> > - Johan
> >
> > On Sat, Aug 29, 2020 at 4:32 PM Nir Lisker  wrote:
> >
> >> Reviving this, the JDK has a page for release notes [1] that makes it
> >> easier for the developer to be updated on the changes. I think that we
> can
> >> do something similar.
> >>
> >> [1]
> https://www.oracle.com/java/technologies/javase/14-relnote-issues.html
> >>
> >> On Mon, Mar 9, 2020 at 2:56 PM Jeanette Winzenburg <
> >> faste...@swingempire.de>
> >> wrote:
> >>
> >>> agree with better highlighting of the important changes/issues
> >>> disagree with leaving out the not so important - that's in the eye of
> >>> the reader, anyway - changes/issues: after all, the target readership
> >>> are technicians, not suits :)
> >>>
> >>> -- Jeanette
> >>>
> >>> Zitat von Nir Lisker :
> >>>
> >>>> Hi,
> >>>>
> >>>> I posted the release notes for OpenJFX 14 on Reddit. The top comment
> >> [1]
> >>>> complained about the presentation. Maybe on openjfx.io the Release
> >> Notes
> >>>> link could show a more "friendly" summary with a link to the detailed
> >>>> table. I suggest:
> >>>>
> >>>> 1. Including only changes that matter to the users of the library with
> >>>> separate sections for new API and for (non-internal) bug fixes.
> >>>> 2. Highlighting significant changes.
> >>>>
> >>>> Some example bug fixes to include:
> >>>> - TableView, ListView: unexpected scrolling behaviour on up/down keys
> >>>> - DragAndDrop no longer works with GTK3
> >>>> - Window order is not correct when Modality.WINDOW_MODAL
> >>>>
> >>>> Don't include:
> >>>> - [WebView] Sub-resource integrity check fails on Windows and Linux
> >>>> - javafx.web build fails on XCode 10.2
> >>>> - Cherry pick GTK WebKit ___ changes
> >>>>
> >>>> Enhancements to include:
> >>>> - Add support for e-paper displays
> >>>> - Add exclusion scope for LightBase
> >>>> - Point2D and Point3D should implement Interpolatable
> >>>>
> >>>> Don't include:
> >>>> -  Color, Point2D and Point3D's fields should be made final
> >>>>
> >>>> Thoughts?
> >>>>
> >>>> - Nir
> >>>>
> >>>> [1]
> >>>>
> >>
> https://www.reddit.com/r/java/comments/fegcv9/release_notes_for_javafx_14/fjphqfn?utm_source=share_medium=web2x
> >>>
> >>>
> >>>
>
>

Re: JavaFX14/Listener not called

[Nir Lisker]

Hi Peter,

This is a development mailing list, not a help list. I suggest you ask
questions on a help site.

The reason you see this difference is that you're using an invalidation
listener which uses lazy evaluation, so in your second example it is not
updated because it is not used in the printing method. Read the
documentations/tutorial of listeners and observables.

- Nir

On Thu, Aug 20, 2020 at 5:19 PM Petr Nemecek  wrote:

> Dear all,
>
>
>
> could somebody explain me, please, why the output differs for CODE-1 and
> CODE-2 see below? I would expect the listener to be called four times in
> both cases.
>
>
>
> I believe there's trivial answer for that, but I'm not able to find it by
> myself, probably due to the hot weather here in Prague, CZ.
>
>
>
> Many thanks,
>
> Petr
>
>
>
> ***
>
> CODE-1
>
> IntegerProperty property = new SimpleIntegerProperty();
>
>
>
> property.addListener((o) -> {
>
> System.out.println("value changed to " + property.get() + " ... " +
> System.currentTimeMillis());
>
> });
>
>
>
> for (int i = 0; i < 5; i++) {
>
> property.set(i);
>
> }
>
>
>
> OUTPUT-1
>
> value changed to 1 ... 1597932211288
>
> value changed to 2 ... 1597932211293
>
> value changed to 3 ... 1597932211293
>
> value changed to 4 ... 1597932211294
>
>
>
> ***
>
> CODE-2
>
> IntegerProperty property = new SimpleIntegerProperty();
>
>
>
> property.addListener((o) -> {
>
>System.out.println("value changed" + " . " +
> System.currentTimeMillis());
>
> });
>
>
>
> for (int i = 0; i < 5; i++) {
>
>  property.set(i);
>
> }
>
>
>
> OUTPUT-2
>
> value changed ... 1597932237151
>
> ***
>
>

Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors [v5]

[Nir Lisker]

On Wed, 26 Aug 2020 15:46:46 GMT, Bhawesh Choudhary  
wrote:

>> Added missing explicit no-arg constructors to classes in package 
>> javafx.scene, javafx.css and javafx.stage.
>
> Bhawesh Choudhary has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Updated comments as per JDK Convention

Marked as reviewed by nlisker (Reviewer).

-

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