Re: [Rev 01] RFR: 8175358: Memory leak when moving MenuButton into another Scene

[Ambarish Rapte]

On Mon, 27 Apr 2020 12:35:11 GMT, Ajit Ghaisas  wrote:

>> Issue : https://bugs.openjdk.java.net/browse/JDK-8175358
>> 
>> Root cause : When a MenuItem is removed from a Scene, if any accelerator has 
>> been set on MenuItem, it does not get
>> removed from Scene's list of accelerators.
>> Fix : If Scene changes for a Menu, remove the registered accelerators from 
>> Scene.
>> 
>> Testing : Added a unit test that fails before the fix and passes with it.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   spelling_correction

Looks good to me.

-

Marked as reviewed by arapte (Reviewer).

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

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

[Kevin Rushforth]

On Mon, 27 Apr 2020 23:32:04 GMT, Nir Lisker  wrote:

> I will review this too anyway.

Thank you. That will be helpful.

-

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

Re: RFR: 8242861: Update ImagePattern to apply SVG pattern transforms

[Kevin Rushforth]

On Mon, 20 Apr 2020 05:40:51 GMT, Arun Joseph  wrote:

> fillPath() and fillRect() functions in
> [GraphicsContextJava.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WebCore/platform/graphics/java/GraphicsContextJava.cpp)
> use Image::drawPattern() for applying patterns as fill. But drawPattern() 
> doesn't use patternTransform argument as
> ImagePattern doesn't have the same attribute. So, the final image won't be 
> transformed.

Since this is in a common method used by all shapes, and not just WebView, we 
will need to ensure no regressions.

-

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

Re: [Rev 01] RFR: 8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi

[Kevin Rushforth]

On Mon, 27 Apr 2020 11:09:51 GMT, Alexander Scherbatiy  
wrote:

>> See the detailed issue description on: 
>> http://mail.openjdk.java.net/pipermail/openjfx-dev/2020-April/025975.html
>> 
>> The fix 8236448 https://github.com/openjdk/jfx/pull/75 changes
>> [MonocleApplication.staticScreen_getScreens()](https://github.com/openjdk/jfx/pull/75/files#diff-b66ff7fe72c6c5cd26003572ca901bfdL228)
>> method code from
>>> ns.getDPI(), ns.getDPI(), ns.getScale(), ns.getScale(), ns.getScale(), 
>>> ns.getScale()
>> 
>>  to
>>  > ns.getWidth(), ns.getHeight(), 1.f, 1.f, ns.getScale(), ns.getScale()"
>> 
>> so the font size is not properly calculated based on the given DPI value.
>> 
>> I left the platformScaleX and platformScaleY as 1.f because I do not know 
>> how it affects Android/Dalvik platform. On
>> Raspberry Pi where I run JavaFX code with Monocle the DispmanScreen is used 
>> which have fixed scale 1.0 value so the app
>> works in the same way.
>
> Alexander Scherbatiy has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Search javafx.platform.properties in jfx-runtime/lib directory

modules/javafx.base/src/main/java/com/sun/javafx/PlatformUtil.java line 241:

> 240: s.lastIndexOf('/'), s.lastIndexOf('\\'));
> 241: return new File(new URL(s.substring(0, lastIndexOfSlash + 
> 1)).getPath());
> 242: } catch (MalformedURLException e) {

The previous code looks like a hold-over from JDK 8, where jfxrt.jar was in 
`lib/ext`. It also assumes that the JavaFX
runtime is being loaded from a jar URL, which isn't necessarily the case. 
Probably the only reason it hasn't caused
problems before now is that it is only used to locate 
`javafx.platform.properties`. Worth noting is that we won't get
here in the case JavaFX is jlinked into the Java runtime, although in that 
case, the fallback method of locating it
relative to the JDK should be used.

I'll take a closer look this specific change, but I think it should be OK.

I'll defer the review as a whole to Johan.

-

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

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

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

[Kevin Rushforth]

On Fri, 24 Apr 2020 00:58:30 GMT, Nir Lisker  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.

@arapte can you also review this?

-

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

Re: [Rev 01] RFR: 8175358: Memory leak when moving MenuButton into another Scene

[Kevin Rushforth]

On Mon, 27 Apr 2020 14:45:19 GMT, Jeanette Winzenburg  
wrote:

>> Ajit Ghaisas has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   spelling_correction
>
> forgot to formally start a review - and approving it :)

@arapte can you also review this?

-

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

Re: [Rev 01] RFR: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()

[Ambarish Rapte]

On Sun, 19 Apr 2020 09:51:07 GMT, Jesper Skov 
 wrote:

>> Make the two ways of associating a ToggleButton with a ToggleGroup interact 
>> correctly.
>> 
>> This fixes https://bugs.openjdk.java.net/browse/JDK-8198402
>
> Jesper Skov has updated the pull request incrementally with three additional 
> commits since the last revision:
> 
>  - Fail instead of print message
>  - addedToggles list is final
>  - Leave unrelated file alone

Fix and tests look good to me.

-

Marked as reviewed by arapte (Reviewer).

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

Re: [Rev 01] RFR: 8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi

[Alexander Scherbatiy]

On Mon, 27 Apr 2020 17:43:09 GMT, Johan Vos  wrote:

> I'm confused by this, what is the full version of JDK 14.0.1? JavaFX is not 
> part of the JDK anymore, therefore I don't
> expect a javafx.platform.properties in the JDK. Iif this is the case, it 
> seems a serious bug to me.

I used non Oracle jdk 14.0.1 build which full version includes JavaFX.

-

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

Re: [Rev 02] RFR: 8227425: Add support for e-paper displays on i.MX6 devices

[Johan Vos]

On Fri, 17 Apr 2020 19:11:19 GMT, John Neffenger 
 wrote:

>> This pull request adds support for e-paper displays with the i.MX 6 Series 
>> of Applications Processors and implements
>> [Issue #521](https://github.com/javafxports/openjdk-jfx/issues/521) in the 
>> obsolete *javafxports/openjdk-jfx*
>> repository. Some of the changes were made to support the new device models, 
>> while others are minor changes to the
>> existing support in JavaFX 13.  The following changes were made to support 
>> the new device models.
>> * Work around problems on the Kobo Glo HD Model N437.
>> 
>> Ignore the error ENOTTY (25), "Inappropriate ioctl for device," when 
>> setting the waveform modes. The IOCTL call is
>> ignored by the driver on the Kobo Glo HD where the error occurs, anyway.
>> 
>> Use the visible y-resolution (`yres`), not the virtual y-resolution 
>> (`yres_virtual`), when calculating the capacity of
>> the off-screen byte buffer and the length of the frame buffer mapping. 
>> The virtual y-resolution reported by the frame
>> buffer device driver can be an incorrect value.
>> 
>> * Work around problems on the Kobo Clara HD Model N249.
>> 
>> The Kobo Clara HD requires the native screen width to be set to the 
>> visible x-resolution (`xres`), instead than the
>> normal virtual x-resolution (`xres_virtual`), when using an unrotated 
>> and uninverted 8-bit grayscale frame buffer. The
>> work-around is provided through a new Boolean system property called 
>> *monocle.epd.fixWidthY8UR*.
>> 
>> The following changes were made to the existing code that supports e-paper 
>> displays in JavaFX 13.
>> 
>> * Use the correct constant for the number of bytes per pixel.
>> 
>> Use the number of bytes per pixel (`Integer.BYTES`), not the number of 
>> bits per pixel (`Integer.SIZE`), when
>> calculating the capacity of the 32-bit off-screen byte buffer.
>> 
>> * Do not round the luma value to the nearest integer.
>> 
>> Use the value of luma rounded toward zero automatically by Java when 
>> converting a `float` to an `int` instead of
>> rounding to the nearest integer. The additional rounding operation can 
>> decrease performance anywhere from zero to 10
>> percent and doesn't seem worth it for a display with just 16 levels of 
>> gray.
>> 
>> * Change camel case of system property *y8inverted*.
>> 
>> Change the camel case of the system property *monocle.epd.y8inverted* to 
>> the form *monocle.epd.Y8Inverted* so that it
>> is consistent with the other system properties containing Y1, Y4, and Y8 
>> in their names.
>> 
>> * Improve error handling when mapping the frame buffer.
>> 
>> Log the mapping and unmapping of the frame buffer. Log any errors that 
>> occur on either of the system calls. Handle a
>> `null` return value from `EPDFrameBuffer.getMappedBuffer`.
>> 
>> * Replace non-ASCII characters with their ASCII equivalent.
>> 
>> Replace non-ASCII characters in comments and log messages as follows:
>> 
>> * "×" with "x" for display resolution and color depth,
>> * "×" with "*" for multiplication,
>> * "→" with "to" for transition, and
>> * "°C" with "degrees Celsius" for temperature.
>> 
>> * Add descriptions to Monocle EPD system properties.
>> 
>> Add Javadoc comments to each constant that defines a Monocle EPD system 
>> property.
>> 
>> * Rename `IntStructure.getInteger` to `IntStructure.get`.
>> 
>> Rename the methods `getInteger` and `setInteger` in `IntStructure` to 
>> avoid confusion with the Java method
>> `Integer.getInteger`, which gets the integer value of a system property.
>> 
>> Below are some of the more interesting test results.
>> 
>> * The Kobo Glo HD and Kobo Clara HD implement a true GC4 waveform mode that 
>> displays only pixels with the four gray
>>   values `0x00`, `0x55`, `0xAA`, and `0xFF`. On the older Kobo Touch models, 
>> the GC4 waveform mode is the same as GC16.
>> 
>> * When the *forceMonochrome* update flag is enabled, the Kobo Clara HD uses 
>> a zero-percent threshold that displays black
>>   for pixels with the value `0x00` and white for all other values. The other 
>> models use a 50-percent threshold that
>>   displays black for values `0x7F` or less and white for values `0x80` or 
>> greater.
>> 
>> * The OpenJDK 11 package in Ubuntu 18.04 LTS runs twice as fast as the 
>> AdoptOpenJDK build of OpenJDK 13 for some of the
>>   tests. The speed difference is greatest for the tests that require pixel 
>> conversion into an 8-bit or 16-bit frame
>>   buffer. I plan to investigate the cause of the performance difference.
>> 
>> * In general, the faster processor and memory bus of the newer models does 
>> not fully compensate for the threefold
>>   increase in the number of pixels in their displays. The table below shows 
>> the animation speed of each model in
>>   milliseconds per frame and frames per second.
>> 
>> | Product Name  | Speed (ms) | Rate (fps) |
>> 

Re: [Rev 01] RFR: 8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi

[Johan Vos]

On Mon, 27 Apr 2020 15:49:31 GMT, John Neffenger 
 wrote:

>> Alexander Scherbatiy has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Search javafx.platform.properties in jfx-runtime/lib directory
>
> Thanks, Alexander! I just tested your pull request again, and JavaFX is now 
> picking up the properties file along with
> the correct native screen DPI on my Monocle ARM platform.

> > @AlexanderScherbatiy Did you move the _javafx.platform.properties_ file to 
> > its parent directory manually, as I just did?
> 
> I used the full version of jdk 14.0.1 on Raspberry Pi where JavaFX is 
> included in jdk so the file
> _javafx.platform.properties_ is already in jdk-14.0.1-full/lib directory.
> To debug the JavaFX on Raspberry Pi I built armv6hf-sdk and just copied the 
> file _javafx.platform.properties_ from the
> full jdk version with JavaFX to jdk-14.0.1/lib directory of jdk without 
> JavaFX which I used to run my java application
> with JavaFX modules from armv6hf-sdk/lib.

I'm confused by this, what is the full version of JDK 14.0.1? JavaFX is not 
part of the JDK anymore, therefore I don't
expect a javafx.platform.properties in the JDK. Iif this is the case, it seems 
a serious bug to me.

-

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

Re: Community request to test 3D performance

[David Grieve]

Results with NVIDIA Quadro P400:

Without the fix, 1000 quads, average FPS ~7.4
With the fix, 1000 quads, average FPS ~6.1
(this is with Kevin’s pointlighttest.zip)

From: Nir Lisker 
Sent: Thursday, April 23, 2020 5:41 PM
To: David Grieve ; openjfx-dev@openjdk.java.net 
Mailing 
Subject: [EXTERNAL] Re: RE; Community request to test 3D performance

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 
mailto:david.gri...@microsoft.com>> wrote:
I have an NVIDIA Quadro P400. Will that help?

-Original Message-
From: openjfx-dev 
mailto:openjfx-dev-boun...@openjdk.java.net>>
 On Behalf Of Nir Lisker
Sent: Thursday, April 23, 2020 3:55 PM
To: openjfx-dev@openjdk.java.net Mailing 
mailto:openjfx-dev@openjdk.java.net>>
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: [Rev 03] RFR: 8217472: Add attenuation for PointLight

[David Grieve]

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

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

Results with NVIDIA Quadro P400:

Without the fix, 1000 quads, average FPS ~7.4
With the fix, 1000 quads, average FPS ~6.1

-

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

Re: [Rev 01] RFR: 8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi

[John Neffenger]

On Mon, 27 Apr 2020 11:09:51 GMT, Alexander Scherbatiy  
wrote:

>> See the detailed issue description on: 
>> http://mail.openjdk.java.net/pipermail/openjfx-dev/2020-April/025975.html
>> 
>> The fix 8236448 https://github.com/openjdk/jfx/pull/75 changes
>> [MonocleApplication.staticScreen_getScreens()](https://github.com/openjdk/jfx/pull/75/files#diff-b66ff7fe72c6c5cd26003572ca901bfdL228)
>> method code from
>>> ns.getDPI(), ns.getDPI(), ns.getScale(), ns.getScale(), ns.getScale(), 
>>> ns.getScale()
>> 
>>  to
>>  > ns.getWidth(), ns.getHeight(), 1.f, 1.f, ns.getScale(), ns.getScale()"
>> 
>> so the font size is not properly calculated based on the given DPI value.
>> 
>> I left the platformScaleX and platformScaleY as 1.f because I do not know 
>> how it affects Android/Dalvik platform. On
>> Raspberry Pi where I run JavaFX code with Monocle the DispmanScreen is used 
>> which have fixed scale 1.0 value so the app
>> works in the same way.
>
> Alexander Scherbatiy has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Search javafx.platform.properties in jfx-runtime/lib directory

Thanks, Alexander! I just tested your pull request again, and JavaFX is now 
picking up the properties file along with
the correct native screen DPI on my Monocle ARM platform.

-

Marked as reviewed by jgn...@github.com (no known OpenJDK username).

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

Re: [Rev 04] RFR: 8241840: Memoryleak: Closed focused Stages are not collected with Monocle.

[Ambarish Rapte]

On Wed, 22 Apr 2020 12:03:34 GMT, Florian Kirmaier  
wrote:

>> Closed focused Stages are not collected with Monocle
>> 
>> This commit adds a unit-test and a fix for collecting focused closed stages.
>> 
>> ticket: https://bugs.openjdk.java.net/browse/JDK-8241840
>
> Florian Kirmaier has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   JDK-8241840
>   The tests are now reused for native and monocle tests.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/Window.java line 1325:

> 1324: this.isFocused = focused;
> 1325: if (this.isFocused && this.isVisible) {
> 1326: setFocusedWindow(this);

On my Window10 machine, with this change, `Window.focusedWindow` remains `null` 
even after the first window is shown
onto the screen and is focused. And It continues to remain `null` until some 
mouse or key action is performed on the
window. I am not sure if this causes any side effects. It looks like the 
`Window.focusedWindow` is mostly(only) used
for Monocle. Can you please confirm the behavior that `Window.focusedWindow` 
remain `null` and check for any side
effects.

-

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

Re: [Rev 01] RFR: 8175358: Memory leak when moving MenuButton into another Scene

[Jeanette Winzenburg]

On Mon, 27 Apr 2020 12:35:11 GMT, Ajit Ghaisas  wrote:

>> Issue : https://bugs.openjdk.java.net/browse/JDK-8175358
>> 
>> Root cause : When a MenuItem is removed from a Scene, if any accelerator has 
>> been set on MenuItem, it does not get
>> removed from Scene's list of accelerators.
>> Fix : If Scene changes for a Menu, remove the registered accelerators from 
>> Scene.
>> 
>> Testing : Added a unit test that fails before the fix and passes with it.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   spelling_correction

forgot to formally start a review - and approving it :)

-

Marked as reviewed by fastegal (Author).

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

Re: RFR: 8175358: Memory leak when moving MenuButton into another Scene

[Jeanette Winzenburg]

On Mon, 27 Apr 2020 11:57:58 GMT, Ajit Ghaisas  wrote:

> Issue : https://bugs.openjdk.java.net/browse/JDK-8175358
> 
> Root cause : When a MenuItem is removed from a Scene, if any accelerator has 
> been set on MenuItem, it does not get
> removed from Scene's list of accelerators.
> Fix : If Scene changes for a Menu, remove the registered accelerators from 
> Scene.
> 
> Testing : Added a unit test that fails before the fix and passes with it.

fix looks good to me :)

Seeing the code, I think we have two follow-up issues (not introduced here, 
they just jump to visibility in the light
of reading the code and recent fixes around memory leaks :)

- the listener to the control's sceneProperty is never removed, introducing a 
memory leak, a fix could be similar to that
  of the recently [fixed 
JDK-8236840](https://bugs.openjdk.java.net/browse/JDK-8236840)

- we have the exact same issue with accelerators in a contextMenu of a control 
that's removed from the scene (the
  accelerators are still active, as can be seen in adapting your test method). 
Not sure which collaborator should be
  responsible for the cleanup, could be the helper class, the control, or ?

-

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

Re: [Rev 03] RFR: 8242548: Honor line spacing in Labeled reflow calculation

[John Hendrikx]

> This is a solution for 8242548.  There was zero test coverage, so I added a 
> few tests for this as well.

John Hendrikx has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix typo in comment

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/173/files
  - new: https://git.openjdk.java.net/jfx/pull/173/files/20ccd127..8fa8f3f0

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

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

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

Re: RFR: 8243115: Unregister bindings when unbind called multiple times

[Kevin Rushforth]

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

>> This fixes a bug where the first call to unbind would clear the internal 
>> invalidation listener used, resulting in
>> subsequent unbind calls to be no-ops, unless bind was called again first.
>> I had to rewrite the parameterized test slightly as Parameterized will only 
>> call the parameters method once, and my new
>> test modifies the internal state of the bindings used as parameters (by 
>> doing some unbind calls) which was making other
>> tests fail.
>
> 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?

-

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

Re: RFR: 8243115: Unregister bindings when unbind called multiple times

[Kevin Rushforth]

On Mon, 27 Apr 2020 11:43:28 GMT, John Hendrikx  wrote:

> This fixes a bug where the first call to unbind would clear the internal 
> invalidation listener used, resulting in
> subsequent unbind calls to be no-ops, unless bind was called again first.
> I had to rewrite the parameterized test slightly as Parameterized will only 
> call the parameters method once, and my new
> test modifies the internal state of the bindings used as parameters (by doing 
> some unbind calls) which was making other
> tests fail.

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

-

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

Re: backport request: JDK-8234916

[Kevin Rushforth]

+1

On 4/25/2020 11:02 AM, Johan Vos wrote:

Hi Kevin,

I request permission to backport JDK-8234916 ([macos 10.15] Garbled text
running with native-image) to OpenJFX 11.

The patch applies clean.

- Johan

Re: [Rev 01] RFR: 8175358: Memory leak when moving MenuButton into another Scene

[Ajit Ghaisas]

> Issue : https://bugs.openjdk.java.net/browse/JDK-8175358
> 
> Root cause : When a MenuItem is removed from a Scene, if any accelerator has 
> been set on MenuItem, it does not get
> removed from Scene's list of accelerators.
> Fix : If Scene changes for a Menu, remove the registered accelerators from 
> Scene.
> 
> Testing : Added a unit test that fails before the fix and passes with it.

Ajit Ghaisas has updated the pull request incrementally with one additional 
commit since the last revision:

  spelling_correction

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/199/files
  - new: https://git.openjdk.java.net/jfx/pull/199/files/e18b0cc9..8e3cfd25

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

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

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

Re: [Rev 02] RFR: 8242548: Honor line spacing in Labeled reflow calculation

[Ajit Ghaisas]

On Thu, 23 Apr 2020 11:32:26 GMT, John Hendrikx  wrote:

>> This is a solution for 8242548.  There was zero test coverage, so I added a 
>> few tests for this as well.
>
> John Hendrikx has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental
> views will show differences compared to the previous content of the PR.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/skin/Utils.java
 line 432:

> 431: // line spacing wouldn't fit, the height used for the calculation
> 432: // is increase here with the line spacing amount.
> 433:

increase -> increased

-

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

RFR: 8175358: Memory leak when moving MenuButton into another Scene

[Ajit Ghaisas]

Issue : https://bugs.openjdk.java.net/browse/JDK-8175358

Root cause : When a MenuItem is removed from a Scene, if any accelerator has 
been set on MenuItem, it does not get
removed from Scene's list of accelerators.

Fix : If Scene changes for a Menu, remove the registered accelerators from 
Scene.

Testing : Added a unit test that fails before the fix and passes with it.

-

Commit messages:
 - menuItem_accelerator_fix

Changes: https://git.openjdk.java.net/jfx/pull/199/files
 Webrev: https://webrevs.openjdk.java.net/jfx/199/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8175358
  Stats: 46 lines in 2 files changed: 44 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jfx/pull/199.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/199/head:pull/199

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

RFR: 8243115: Unregister bindings when unbind called multiple times

[John Hendrikx]

This fixes a bug where the first call to unbind would clear the internal 
invalidation listener used, resulting in
subsequent unbind calls to be no-ops, unless bind was called again first.

I had to rewrite the parameterized test slightly as Parameterized will only 
call the parameters method once, and my new
test modifies the internal state of the bindings used as parameters (by doing 
some unbind calls) which was making other
tests fail.

-

Commit messages:
 - 8243115: Unregister bindings when unbind called multiple times

Changes: https://git.openjdk.java.net/jfx/pull/198/files
 Webrev: https://webrevs.openjdk.java.net/jfx/198/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8243115
  Stats: 206 lines in 9 files changed: 147 ins; 43 del; 16 mod
  Patch: https://git.openjdk.java.net/jfx/pull/198.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/198/head:pull/198

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

Re: RFR: 8087555: [ChoiceBox] uncontained value not shown

[Ambarish Rapte]

On Fri, 24 Apr 2020 15:07:59 GMT, Jeanette Winzenburg  
wrote:

> Which seems to be hinted at in a code comment in 
> `SingleSelectionModel.select(item)`:

I agree that selectedIndex should be -1 for an uncontained value, so that the 
below condition always holds true. And
this is already working well with ComboBox.

`assertEquals(getItems().indexOf(selectedItem), selectedIndex)`

The fix for [JDK-8241999](https://bugs.openjdk.java.net/browse/JDK-8241999) 
would solve most of the behavior
differences of contained Vs uncontained item selection. (that I was looking at 
selectPrevious(), selectNext()).




> true, that's spec'ed in ComboBox only .. so by analogy I considered it being 
> the same here. Hmm .. might need an update
> of the doc?

Thanks for the the pointer, After reading it, I have a doubtful understanding 
about it as, that,

The spec is only about `ComboBox.valueProperty()` and not about 
`SelectionModel.selectedIndex` or
`SelectionModel.selectedItem`. so clearSelection() can/should clear the 
SelectionModel state but should not clear
ComboBox.valueProperty().

And ComboBox does not follow this spec,
In below scenario `ComboBox.valueProperty()` is cleared to NULL.

`comboBox.getSelectionModel().select(comboBox.getItems(1));`
`comboBox.getSelectionModel().clearSelection();` => clears the 
SelectionModel.selectedIndex to -1,
SelectionModel.selectedItem and ComboBox.getValue() to NULL.

So I think, for ChoieBox, In addition to doc change we would need fix the 
behavior as well,  and maintain same behavior
of `clearSelection()` with contained and uncontained items.

But again this is a different issue and not related to this fix.

So the fix itself looks good to me along with the upcoming fix for
[JDK-8241999](https://bugs.openjdk.java.net/browse/JDK-8241999).

-

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

Re: RFR: 8087555: [ChoiceBox] uncontained value not shown

[Ambarish Rapte]

On Mon, 20 Apr 2020 12:23:16 GMT, Jeanette Winzenburg  
wrote:

> The issue is that ChoiceBoxSkin
> a) doesn't update the text of the label if the value is not contained in the 
> items
> b) doesn't respect converter for label text
> 
> Fixed by
> - listening to value changes to update the label
> - removing ad-hoc updates (not needed), added update on converter change
> - passing all label updates through converter
> 
> Added test for text updates that failed before the fix and pass after (note: 
> there were no tests for the display text,
> so for coveragy, contains also tests that passed before as well as after)

Marked as reviewed by arapte (Reviewer).

-

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

Re: RFR: 8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi

[Alexander Scherbatiy]

On Fri, 24 Apr 2020 17:07:25 GMT, John Neffenger 
 wrote:

>>> To debug the JavaFX on Raspberry Pi I built armv6hf-sdk and just copied the 
>>> file _javafx.platform.properties_ from the
>>> full jdk version with JavaFX to jdk-14.0.1/lib directory of jdk without 
>>> JavaFX which I used to run my java application
>>> with JavaFX modules from armv6hf-sdk/lib.
>> 
>> May I then suggest one additional change to this pull request? It's a 
>> two-line fix:
>> 
>> --- a/modules/javafx.base/src/main/java/com/sun/javafx/PlatformUtil.java
>> +++ b/modules/javafx.base/src/main/java/com/sun/javafx/PlatformUtil.java
>> @@ -238,8 +238,7 @@ public class PlatformUtil {
>>  // Strip everything after the last "/" or "" to get rid of the 
>> jar filename
>>  int lastIndexOfSlash = Math.max(
>>  s.lastIndexOf('/'), s.lastIndexOf('\'));
>> -return new File(new URL(s.substring(0, lastIndexOfSlash + 
>> 1)).getPath())
>> -.getParentFile();
>> +return new File(new URL(s.substring(0, lastIndexOfSlash + 
>> 1)).getPath());
>>  } catch (MalformedURLException e) {
>>  return null;
>>  }
>> 
>> That change corrects the code to look for the `javafx.platform.properties` 
>> file in the same directory as the
>> `javafx.base.jar` file instead of looking for it in the parent directory of 
>> the JAR file. I just stepped through the
>> code with this change, and the method `PlatformUtil.loadProperties` now 
>> finds the properties file in the location where
>> it is packaged by the build in the SDK.
>
> Related issues, including the original request for enhancement:
> 
> * [JDK-8100775](https://bugs.openjdk.java.net/browse/JDK-8100775): Means of 
> bundling platform specific settings for
>   glass/runtime
> * [JDK-8115678](https://bugs.openjdk.java.net/browse/JDK-8115678): JavaFX 
> preview on Raspberry Pi
>   requires -Djavafx.platform=eglfb to be set
> * [JDK-8117705](https://bugs.openjdk.java.net/browse/JDK-8117705): Embedded 
> JavaFX can't find javafx.platform.properties
> 
> The last two issues listed above were fixed by the following changeset:
> 
> [Fix for RT-28035 (Can't find embedded platform properties) and RT-27194 
> (Must set
> javafx.platform)](https://hg.openjdk.java.net/openjfx/8/master/rt/rev/e4577fd9c0f1)
> In JDK 8, all of the Java property files, including 
> `javafx.platform.properties`, are located under the `jre/lib`
> directory of the JDK, but the JavaFX 8 JAR file `jfxrt.jar` in found in the 
> subdirectory `jre/lib/ext`. The changeset
> above fixed the JavaFX 8 code to look in the parent directory for its 
> properties.  Now, though, the JavaFX SDK puts the
> JAR file `javafx.base.jar` in the same `lib` directory as the property files, 
> so we need to remove the method call that
> changes the path to its parent.

> May I then suggest one additional change to this pull request? It's a 
> two-line fix.
> That change corrects the code to look for the `javafx.platform.properties` 
> file in the same directory as the
> `javafx.base.jar` file instead of looking for it in the parent directory of 
> the JAR file.

I have added the fix for `PlatformUtil.getRTDir()` method to the current pull 
request.

-

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

Re: [Rev 01] RFR: 8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi

[Alexander Scherbatiy]

> See the detailed issue description on: 
> http://mail.openjdk.java.net/pipermail/openjfx-dev/2020-April/025975.html
> 
> The fix 8236448 https://github.com/openjdk/jfx/pull/75 changes
> [MonocleApplication.staticScreen_getScreens()](https://github.com/openjdk/jfx/pull/75/files#diff-b66ff7fe72c6c5cd26003572ca901bfdL228)
> method code from
>> ns.getDPI(), ns.getDPI(), ns.getScale(), ns.getScale(), ns.getScale(), 
>> ns.getScale()
> 
>  to
>  > ns.getWidth(), ns.getHeight(), 1.f, 1.f, ns.getScale(), ns.getScale()"
> 
> so the font size is not properly calculated based on the given DPI value.
> 
> I left the platformScaleX and platformScaleY as 1.f because I do not know how 
> it affects Android/Dalvik platform. On
> Raspberry Pi where I run JavaFX code with Monocle the DispmanScreen is used 
> which have fixed scale 1.0 value so the app
> works in the same way.

Alexander Scherbatiy has updated the pull request incrementally with one 
additional commit since the last revision:

  Search javafx.platform.properties in jfx-runtime/lib directory

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/193/files
  - new: https://git.openjdk.java.net/jfx/pull/193/files/2551ce75..058e760a

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

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

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