Re: RFR: 8263322: Calling Application.launch on FX thread should throw IllegalStateException, but causes deadlock [v9]

[Florian Kirmaier]

On Tue, 23 Mar 2021 07:53:03 GMT, Ambarish Rapte  wrote:

>> Florian Kirmaier has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   JDK-8263322
>>   fixed the names of some tests
>
> Marked as reviewed by arapte (Reviewer).

The CSR is now in the proposed state!

-

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

Re: RFR: 8264127: ListCell editing status is true, when index changes while editing

[Florian Kirmaier]

On Thu, 25 Mar 2021 11:44:21 GMT, Jeanette Winzenburg  
wrote:

>> Fixing ListCell editing status is true, when index changes while editing.
>
> good catch, yet another property that's not correctly updated on index change 
> :) 
> 
> Quick questions:
> - what are the macroscopic effects (that is in a running list) that you see 
> without fixing it?
> - do we want mere cell re-use fire a editCancel? (which the list seems to 
> ignore in the test, don't know why)

To clarify, this with happening with ListViews in production code.
In some conditions, the status of a single cell doesn't update. The effect is, 
that a Cell is stuck in the Editing mode.
If I remember correctly, it was irreversible stuck. It happened rarely, it 
might be connected to cells with variable sizes.

Handling the logic from the ListView seems wrong to me, I think the ListCell 
should update its state automatically dependent of the properties. But I 
wouldn't know where to add it in the ListView. If that's how it's done in the 
other cell-based components, then it would make sense and the code more linear. 
I'm not really opinionated on how to solve it, just went for the simplest fix 
I've found.

-

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

Re: RFR: 8242508: Upgrade to Visual Studio 2019 version 16.5.3

[Kevin Rushforth]

On Wed, 3 Mar 2021 22:07:34 GMT, Alessadro Parisi 
 wrote:

> Can you also update the build script located in tools/scripts?
> Build fails on my system with `FAIL: WINSDK_DIR not defined`

I filed [JDK-8264219](https://bugs.openjdk.java.net/browse/JDK-8264219) to 
track this. We don't use that script, and I have no good way to test it. Maybe 
someone in the community could pick it up.

> Also, it's not possible to install Windows SDK 7.1 on Windows 10, so that 
> should be updated too

The dependency on Windows SDK 7.1 has been eliminated; I noted that in the bug 
report.

-

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

Re: RFR: 8264162: PickResult.toString() is missing the closing square bracket [v2]

[Nir Lisker]

On Thu, 25 Mar 2021 22:08:36 GMT, Kevin Rushforth  wrote:

>> Simple fix to add a missing closing bracket to `PickResult::toString`. This 
>> includes a unit test that fails without the fix and passes with the fix.
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address review feedback: fix indentation in PickResult::toString, add 
> comment to the test

Marked as reviewed by nlisker (Reviewer).

-

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

Re: RFR: 8264162: PickResult.toString() is missing the closing square bracket [v2]

[Kevin Rushforth]

> Simple fix to add a missing closing bracket to `PickResult::toString`. This 
> includes a unit test that fails without the fix and passes with the fix.

Kevin Rushforth has updated the pull request incrementally with one additional 
commit since the last revision:

  Address review feedback: fix indentation in PickResult::toString, add comment 
to the test

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/443/files
  - new: https://git.openjdk.java.net/jfx/pull/443/files/7b037029..ebad8e26

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

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

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

Re: RFR: 8150709: Mac OSX and German Keyboard Layout (Y/Z)

[Martin Fox]

On Wed, 24 Mar 2021 22:00:03 GMT, Kevin Rushforth  wrote:

>> I've written a command line utility that runs through all the keyboard 
>> layouts available in macOS 11.2.3 and generates a report on how this PR 
>> would change the Java key codes for each layout. I used it to spot-check the 
>> behavior on various layouts and to flag potential trouble spots (like 
>> Lithuanian where digits are typed using a dead key.) I thought it would also 
>> be useful for this review. For example, I can now say that with this PR 
>> Meta-, (comma) will still be available on all layouts.
>> 
>> If the report would be of interest where should I put it? Add it to this PR, 
>> attach it to a comment in this thread, put it someplace public and link to 
>> it?
>
> I guess it depends on how large the table is. Adding it as an inline table 
> using markdown could be helpful, unless it is so large that it takes too long 
> to scroll past it. In that case, I'd probably recommend attaching it to a 
> comment in the PR.

I've attached a text document which details which Java key codes are lost and 
gained for each keyboard layout compared to the pre-PR code. It also flags some 
potential trouble spots with the word "Warning". Note that at the bottom of the 
file there's a long list of layouts which are unchanged. I've added it as a 
file to make it easier to diff against future versions if I need to tweak the 
algorithm some more (here's hoping I don't).

With the latest code tweak the Java key codes A to Z are available on all 
keyboards except Turkmen as there really is no way to type 'X' on that layout. 
The key codes Digit0 to Digit9 are available on all keyboards except Lithuanian 
where the digits require a dead-key to type.

[GainsAndLosses.txt](https://github.com/openjdk/jfx/files/6207211/GainsAndLosses.txt)

-

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

Re: RFR: 8264010: Add Gradle dependency verification

[John Neffenger]

On Wed, 24 Mar 2021 19:50:11 GMT, John Neffenger  wrote:

> Are all of them actually needed?

Just to follow up on that question, all of them are in fact downloaded during 
the build, at least. I removed the Gradle directory `$HOME/.gradle` and ran the 
build as follows. Then I compared the list of downloaded artifacts with the 
ones listed in the dependency verification file.

$ rm -r $HOME/.gradle
$ bash gradlew sdk jmods javadoc apps test -x :web:test
  ...
$ find ~/.gradle/caches/modules-2 ( -name "*.jar" -o -name "*.pom" ) \
  -exec basename {} ; | sort > downloaded.log
$ grep '/\1/' \
  | sort > verified.log
$ diff downloaded.log verified.log 
31a32
> org.eclipse.swt.cocoa.macosx.x86_64_3.105.3.v20170228-0512-.jar
32a34
> org.eclipse.swt.win32.win32.x86_64_3.105.3.v20170228-0512-.jar

A total of 36 artifacts (14 JAR files and 22 POM files) are downloaded during 
the build. The SWT libraries for macOS and Windows were not downloaded because 
I ran the build on Linux.

-

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

Re: RFR: 8264162: PickResult.toString() is missing the closing square bracket

[Kevin Rushforth]

On Thu, 25 Mar 2021 16:30:30 GMT, Nir Lisker  wrote:

>> Good catch. Test fails before and succeeds after the patch.
>
> By the way, this class could easily be converted to a `record`, and that 
> would take care of the `toString` and other ceremonies. I'm not sure it's a 
> backwards compatible change, though.

Converting an existing class to a Record wouldn't be backward compatible (not 
to mention we can't use them yet). It wouldn't help for the Node::toString 
portion in any case.

I'll add the comment and send out a new version.

-

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

Re: RFR: 8150709: Mac OSX and German Keyboard Layout (Y/Z) [v4]

[Martin Fox]

> This PR adds code to ensure that KeyCodeCombinations match KeyEvents as 
> expected by more accurately mapping from a Mac key code to a Java key code 
> based on the user’s active keyboard layout (the existing code assumes a US 
> QWERTY layout). The new code first identifies a set of Mac keys which can 
> produce different characters based on the user’s keyboard layout. A Mac key 
> code outside that area is processed exactly as before. For a key inside the 
> layout-sensitive area the code calls UCKeyTranslate to translate the key to 
> an unshifted ASCII character based on the active keyboard and uses that to 
> determine the Java key code.
> 
> When performing the reverse mapping for the Robot the code first uses the old 
> QWERTY mapping to find a candidate key. If it lies in the layout-sensitive 
> area the code then scans the entire area calling UCKeyTranslate until it 
> finds a match. If the key lies outside the layout-sensitive area it’s 
> processed exactly as before.
> 
> There are multiple duplicates of these bugs logged against Mac applications 
> built with JavaFX.
> 
> https://bugs.openjdk.java.net/browse/JDK-8090257 Mac: Inconsistent KeyEvents 
> with alternative keyboard layouts
> https://bugs.openjdk.java.net/browse/JDK-8088120 [Accelerator, Mac] CMD + Z 
> accelerator is not working with French keyboard
> https://bugs.openjdk.java.net/browse/JDK-8087915 Mac: accelerator doesn't 
> take into account azerty keyboard layout
> https://bugs.openjdk.java.net/browse/JDK-8150709 Mac OSX and German Keyboard 
> Layout (Y/Z)

Martin Fox has updated the pull request incrementally with one additional 
commit since the last revision:

  Fixed whitespace error.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/425/files
  - new: https://git.openjdk.java.net/jfx/pull/425/files/26a6fb0d..fb449d93

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

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

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

Re: RFR: 8150709: Mac OSX and German Keyboard Layout (Y/Z) [v3]

[Martin Fox]

> This PR adds code to ensure that KeyCodeCombinations match KeyEvents as 
> expected by more accurately mapping from a Mac key code to a Java key code 
> based on the user’s active keyboard layout (the existing code assumes a US 
> QWERTY layout). The new code first identifies a set of Mac keys which can 
> produce different characters based on the user’s keyboard layout. A Mac key 
> code outside that area is processed exactly as before. For a key inside the 
> layout-sensitive area the code calls UCKeyTranslate to translate the key to 
> an unshifted ASCII character based on the active keyboard and uses that to 
> determine the Java key code.
> 
> When performing the reverse mapping for the Robot the code first uses the old 
> QWERTY mapping to find a candidate key. If it lies in the layout-sensitive 
> area the code then scans the entire area calling UCKeyTranslate until it 
> finds a match. If the key lies outside the layout-sensitive area it’s 
> processed exactly as before.
> 
> There are multiple duplicates of these bugs logged against Mac applications 
> built with JavaFX.
> 
> https://bugs.openjdk.java.net/browse/JDK-8090257 Mac: Inconsistent KeyEvents 
> with alternative keyboard layouts
> https://bugs.openjdk.java.net/browse/JDK-8088120 [Accelerator, Mac] CMD + Z 
> accelerator is not working with French keyboard
> https://bugs.openjdk.java.net/browse/JDK-8087915 Mac: accelerator doesn't 
> take into account azerty keyboard layout
> https://bugs.openjdk.java.net/browse/JDK-8150709 Mac OSX and German Keyboard 
> Layout (Y/Z)

Martin Fox has updated the pull request incrementally with one additional 
commit since the last revision:

  A small number of keyboard layouts require the Option key to reach
  critical letters like 'Q'. Added a third probe (after Command and
  Shift+Command) to look for letters that require Option. The
  keyboards in question are Azeri, Turkmen, and the Sami layouts.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/425/files
  - new: https://git.openjdk.java.net/jfx/pull/425/files/6238c49c..26a6fb0d

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

  Stats: 59 lines in 1 file changed: 29 ins; 14 del; 16 mod
  Patch: https://git.openjdk.java.net/jfx/pull/425.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/425/head:pull/425

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

Re: RFR: 8264162: PickResult.toString() is missing the closing square bracket

[Nir Lisker]

On Thu, 25 Mar 2021 13:21:24 GMT, Johan Vos  wrote:

>> Simple fix to add a missing closing bracket to `PickResult::toString`. This 
>> includes a unit test that fails without the fix and passes with the fix.
>
> Good catch. Test fails before and succeeds after the patch.

By the way, this class could easily be converted to a `record`, and that would 
take care of the `toString` and other ceremonies. I'm not sure it's a backwards 
compatible change, though.

-

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

Re: RFR: 8264162: PickResult.toString() is missing the closing square bracket

[Nir Lisker]

On Thu, 25 Mar 2021 14:15:47 GMT, Kevin Rushforth  wrote:

>> modules/javafx.graphics/src/test/java/test/javafx/scene/input/MouseEventTest.java
>>  line 139:
>> 
>>> 137: case ']':
>>> 138: --bracketCount;
>>> 139: assertTrue("Too many closing brackets: " + str, 
>>> bracketCount >= 0);
>> 
>> This test can fail due to a malformed `toString` result in the node 
>> (`getIntersectedNode()`), which I would think is outside the scope of this 
>> test. In practice, this test's result is dependent on what node I choose to 
>> test.
>> 
>> Shouldn't we be testing the structure of the string and not its contents?
>
> On the one hand, I can see your point that it's somewhat tangential that 
> Rectangle has matched brackets, but you could say the same about the Point3D. 
> Here is the result of MouseEvent.toString for the test:
> 
> MouseEvent [source = 
> javafx.event.Event$$Lambda$110/0x000800c52fa8@46c0,
> target = javafx.event.Event$$Lambda$110/0x000800c52fa8@46c0, 
> eventType = MOUSE_PRESSED,
> consumed = false, x = 18.0, y = 27.0, z = 150.0, button = PRIMARY, 
> primaryButtonDown,
> pickResult = PickResult [node = Rectangle[x=0.0, y=0.0, width=0.0, 
> height=0.0, fill=0x00ff],
> point = Point3D [x = 15.0, y = 25.0, z = 100.0], distance = 33.0]]
> 
> The test would be more complicated if it were changed to ignore the results 
> of the `toString()` of both the  intersected node and the point. If I were to 
> go down this path, I would likely just change it to do match a regex of 
> `"^MouseEvent [.*PickResult [.*]]$"`, which would be simple. Do you think 
> it's worth it?
> 
> Maybe instead I could add a comment that this test checks for matching 
> brackets in the entire composed string returned by `MouseEvent::toString`, 
> including `PickResult::toString`, which in turn includes `Node::toString` for 
> the intersected node?

I don't think it matters too much, but a change in any of the classes used here 
could break this test, which is a bit off. I will be satisfied with a comment 
warning about this case so if it breaks it will be easy to see where the 
problem is.

-

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

Re: [External] : Re: E-mail addresses at openjdk.org

[Kevin Rushforth]

The email address used by the Skara tooling for all git commits, for the 
Committer and for the Author if the Author has an associated OpenJDK ID, 
is: `openjd...@openjdk.org`


-- Kevin


On 3/25/2021 7:03 AM, Nir Lisker wrote:

What are these email addresses?

On Thu, Mar 25, 2021 at 1:12 PM Kevin Rushforth 
mailto:kevin.rushfo...@oracle.com>> wrote:


Eventually the openjdk.org  addresses will be
live. I haven't heard a
recent update on the timing for this. Until then, adding the email
address as an unverified additional email address in your GitHub
profile
is the way to go.

-- Kevin


On 3/24/2021 9:37 PM, John Neffenger wrote:
> I noticed that my last commit used the address
jgn...@openjdk.org 
> instead of the address I normally use to create and sign commits.
> GitHub didn't recognize the commit as mine until I added the
address
> to my account.
>
> Do the openjdk.org  addresses receive
e-mail? If not, do we just leave
> the address unverified in our GitHub accounts?
>
> Thanks,
> John

Re: RFR: 8264162: PickResult.toString() is missing the closing square bracket

[Kevin Rushforth]

On Thu, 25 Mar 2021 13:32:53 GMT, Nir Lisker  wrote:

>> Simple fix to add a missing closing bracket to `PickResult::toString`. This 
>> includes a unit test that fails without the fix and passes with the fix.
>
> modules/javafx.graphics/src/test/java/test/javafx/scene/input/MouseEventTest.java
>  line 139:
> 
>> 137: case ']':
>> 138: --bracketCount;
>> 139: assertTrue("Too many closing brackets: " + str, 
>> bracketCount >= 0);
> 
> This test can fail due to a malformed `toString` result in the node 
> (`getIntersectedNode()`), which I would think is outside the scope of this 
> test. In practice, this test's result is dependent on what node I choose to 
> test.
> 
> Shouldn't we be testing the structure of the string and not its contents?

On the one hand, I can see your point that it's somewhat tangential that 
Rectangle has matched brackets, but you could say the same about the Point3D. 
Here is the result of MouseEvent.toString for the test:

MouseEvent [source = javafx.event.Event$$Lambda$110/0x000800c52fa8@46c0,
target = javafx.event.Event$$Lambda$110/0x000800c52fa8@46c0, eventType 
= MOUSE_PRESSED,
consumed = false, x = 18.0, y = 27.0, z = 150.0, button = PRIMARY, 
primaryButtonDown,
pickResult = PickResult [node = Rectangle[x=0.0, y=0.0, width=0.0, height=0.0, 
fill=0x00ff],
point = Point3D [x = 15.0, y = 25.0, z = 100.0], distance = 33.0]]

The test would be more complicated if it were changed to ignore the results of 
the `toString()` of both the  intersected node and the point. If I were to go 
down this path, I would likely just change it to do match a regex of 
`"^MouseEvent [.*PickResult [.*]]$"`, which would be simple. Do you think it's 
worth it?

Maybe instead I could add a comment that this test checks for matching brackets 
in the entire composed string returned by `MouseEvent::toString`, including 
`PickResult::toString`, which in turn includes `Node::toString` for the 
intersected node?

-

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

Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v2]

[Jeanette Winzenburg]

On Thu, 25 Mar 2021 13:52:20 GMT, Nir Lisker  wrote:

>>> 
>>> 
>>> Can you list the other affected methods?
>> 
>> at line 211 (in the changed skinBase)
>> 
>> /**
>>  * Subclasses can invoke this method to register that they want to 
>> listen to
>>  * property change events for the given property. Registered {@link 
>> Consumer} instances
>>  * will be executed in the order in which they are registered.
>>  * @param property the property
>>  * @param consumer the consumer
>>  */
>> protected final void registerChangeListener(ObservableValue property, 
>> Consumer> consumer) {
>> 
>> 
>> 
>> and at line 255
>> 
>> /**
>>  * Unregisters all change listeners that have been registered using 
>> {@link #registerChangeListener(ObservableValue, Consumer)}
>>  * for the given property. The end result is that the given property is 
>> no longer observed by any of the change
>>  * listeners, but it may still have additional listeners registered on 
>> it through means outside of
>>  * {@link #registerChangeListener(ObservableValue, Consumer)}.
>>  *
>>  * @param property The property for which all listeners should be 
>> removed.
>>  * @return A single chained {@link Consumer} consisting of all {@link 
>> Consumer consumers} registered through
>>  *  {@link #registerChangeListener(ObservableValue, Consumer)}. If 
>> no consumers have been registered on this
>>  *  property, null will be returned.
>>  * @since 9
>>  */
>> protected final Consumer> 
>> unregisterChangeListeners(ObservableValue property) {
>
> I see. I recommend that they be improved in this PR. I don't know if this 
> will need to be part of the CSR, though.

@nlisker and @Kevin so we agree, thanks :)

my plan: 

- will work on the exact doc along the lines of Nir's suggestion for the 
xxInvalidationEvent methods
- once that's basically agreed upon, will c the spec (with adjustments) to 
the methods for the other listener types
- at a last step, create the csr draft (that would be a patch for the 
changeListener methods and simply added spec for the new methods, I assume?)

-

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

Re: E-mail addresses at openjdk.org

[Nir Lisker]

What are these email addresses?

On Thu, Mar 25, 2021 at 1:12 PM Kevin Rushforth 
wrote:

> Eventually the openjdk.org addresses will be live. I haven't heard a
> recent update on the timing for this. Until then, adding the email
> address as an unverified additional email address in your GitHub profile
> is the way to go.
>
> -- Kevin
>
>
> On 3/24/2021 9:37 PM, John Neffenger wrote:
> > I noticed that my last commit used the address jgn...@openjdk.org
> > instead of the address I normally use to create and sign commits.
> > GitHub didn't recognize the commit as mine until I added the address
> > to my account.
> >
> > Do the openjdk.org addresses receive e-mail? If not, do we just leave
> > the address unverified in our GitHub accounts?
> >
> > Thanks,
> > John
>
>

Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v2]

[Kevin Rushforth]

On Thu, 25 Mar 2021 13:52:20 GMT, Nir Lisker  wrote:

>>> 
>>> 
>>> Can you list the other affected methods?
>> 
>> at line 211 (in the changed skinBase)
>> 
>> /**
>>  * Subclasses can invoke this method to register that they want to 
>> listen to
>>  * property change events for the given property. Registered {@link 
>> Consumer} instances
>>  * will be executed in the order in which they are registered.
>>  * @param property the property
>>  * @param consumer the consumer
>>  */
>> protected final void registerChangeListener(ObservableValue property, 
>> Consumer> consumer) {
>> 
>> 
>> 
>> and at line 255
>> 
>> /**
>>  * Unregisters all change listeners that have been registered using 
>> {@link #registerChangeListener(ObservableValue, Consumer)}
>>  * for the given property. The end result is that the given property is 
>> no longer observed by any of the change
>>  * listeners, but it may still have additional listeners registered on 
>> it through means outside of
>>  * {@link #registerChangeListener(ObservableValue, Consumer)}.
>>  *
>>  * @param property The property for which all listeners should be 
>> removed.
>>  * @return A single chained {@link Consumer} consisting of all {@link 
>> Consumer consumers} registered through
>>  *  {@link #registerChangeListener(ObservableValue, Consumer)}. If 
>> no consumers have been registered on this
>>  *  property, null will be returned.
>>  * @since 9
>>  */
>> protected final Consumer> 
>> unregisterChangeListeners(ObservableValue property) {
>
> I see. I recommend that they be improved in this PR. I don't know if this 
> will need to be part of the CSR, though.

Making this change seems fine to me, and better than using different language 
in the new methods without fixing the existing.

> I don't know if this will need to be part of the CSR, though.

It would be best to include them.

-

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

Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v2]

[Nir Lisker]

On Thu, 25 Mar 2021 13:29:44 GMT, Jeanette Winzenburg  
wrote:

>> Can you list the other affected methods?
>
>> 
>> 
>> Can you list the other affected methods?
> 
> at line 211 (in the changed skinBase)
> 
> /**
>  * Subclasses can invoke this method to register that they want to listen 
> to
>  * property change events for the given property. Registered {@link 
> Consumer} instances
>  * will be executed in the order in which they are registered.
>  * @param property the property
>  * @param consumer the consumer
>  */
> protected final void registerChangeListener(ObservableValue property, 
> Consumer> consumer) {
> 
> 
> 
> and at line 255
> 
> /**
>  * Unregisters all change listeners that have been registered using 
> {@link #registerChangeListener(ObservableValue, Consumer)}
>  * for the given property. The end result is that the given property is 
> no longer observed by any of the change
>  * listeners, but it may still have additional listeners registered on it 
> through means outside of
>  * {@link #registerChangeListener(ObservableValue, Consumer)}.
>  *
>  * @param property The property for which all listeners should be removed.
>  * @return A single chained {@link Consumer} consisting of all {@link 
> Consumer consumers} registered through
>  *  {@link #registerChangeListener(ObservableValue, Consumer)}. If no 
> consumers have been registered on this
>  *  property, null will be returned.
>  * @since 9
>  */
> protected final Consumer> 
> unregisterChangeListeners(ObservableValue property) {

I see. I recommend that they be improved in this PR. I don't know if this will 
need to be part of the CSR, though.

-

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

Re: RFR: 8264162: PickResult.toString() is missing the closing square bracket

[Kevin Rushforth]

On Thu, 25 Mar 2021 13:18:24 GMT, Nir Lisker  wrote:

>> Simple fix to add a missing closing bracket to `PickResult::toString`. This 
>> includes a unit test that fails without the fix and passes with the fix.
>
> modules/javafx.graphics/src/main/java/javafx/scene/input/PickResult.java line 
> 204:
> 
>> 202: }
>> 203: if (getIntersectedTexCoord() != null) {
>> 204: sb.append(", texCoord = 
>> ").append(getIntersectedTexCoord());
> 
> Can you fix the double indentation in the `if` bodies?

Sure, as long as I am there, I'll do that.

-

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

Re: RFR: 8264162: PickResult.toString() is missing the closing square bracket

[Nir Lisker]

On Thu, 25 Mar 2021 12:29:38 GMT, Kevin Rushforth  wrote:

> Simple fix to add a missing closing bracket to `PickResult::toString`. This 
> includes a unit test that fails without the fix and passes with the fix.

modules/javafx.graphics/src/main/java/javafx/scene/input/PickResult.java line 
204:

> 202: }
> 203: if (getIntersectedTexCoord() != null) {
> 204: sb.append(", texCoord = 
> ").append(getIntersectedTexCoord());

Can you fix the double indentation in the `if` bodies?

modules/javafx.graphics/src/test/java/test/javafx/scene/input/MouseEventTest.java
 line 139:

> 137: case ']':
> 138: --bracketCount;
> 139: assertTrue("Too many closing brackets: " + str, 
> bracketCount >= 0);

This test can fail due to a malformed `toString` result in the node 
(`getIntersectedNode()`), which I would think is outside the scope of this 
test. In practice, this test's result is dependent on what node I choose to 
test.

Shouldn't we be testing the structure of the string and not its contents?

-

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

Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v2]

[Jeanette Winzenburg]

On Thu, 25 Mar 2021 13:14:45 GMT, Nir Lisker  wrote:

> 
> 
> Can you list the other affected methods?

at line 211 (in the changed skinBase)

/**
 * Subclasses can invoke this method to register that they want to listen to
 * property change events for the given property. Registered {@link 
Consumer} instances
 * will be executed in the order in which they are registered.
 * @param property the property
 * @param consumer the consumer
 */
protected final void registerChangeListener(ObservableValue property, 
Consumer> consumer) {



and at line 255

/**
 * Unregisters all change listeners that have been registered using {@link 
#registerChangeListener(ObservableValue, Consumer)}
 * for the given property. The end result is that the given property is no 
longer observed by any of the change
 * listeners, but it may still have additional listeners registered on it 
through means outside of
 * {@link #registerChangeListener(ObservableValue, Consumer)}.
 *
 * @param property The property for which all listeners should be removed.
 * @return A single chained {@link Consumer} consisting of all {@link 
Consumer consumers} registered through
 *  {@link #registerChangeListener(ObservableValue, Consumer)}. If no 
consumers have been registered on this
 *  property, null will be returned.
 * @since 9
 */
protected final Consumer> 
unregisterChangeListeners(ObservableValue property) {

-

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

Re: RFR: 8264162: PickResult.toString() is missing the closing square bracket

[Johan Vos]

On Thu, 25 Mar 2021 12:29:38 GMT, Kevin Rushforth  wrote:

> Simple fix to add a missing closing bracket to `PickResult::toString`. This 
> includes a unit test that fails without the fix and passes with the fix.

Good catch. Test fails before and succeeds after the patch.

-

Marked as reviewed by jvos (Reviewer).

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

Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v2]

[Nir Lisker]

On Thu, 25 Mar 2021 11:25:18 GMT, Jeanette Winzenburg  
wrote:

>> I reviewed only the public API methods.
>
> @nlisker thanks for the detailed doc review - I really like your suggestions, 
> which I think are considerable improvements (I did part of it in the 
> respective methods of the handler) over the original doc :)  Because that's 
> what the doc of the new methods are: c from the un/registerChangeListener 
> javadoc, adjusted to the specific type of the listener. 
> 
> For consistency, all related method doc should be improved along your lines. 
> How to get there? Options
> 
> - use old doc with new methods, leave improving the doc for all methods to a 
> follow-up issue
> - keep old doc for old method, use improved doc for the new methods, leave 
> improving the doc for the old method to a follow-up issue
> - use improved doc in old and new methods in this issue, changing the old
> 
> Last would be doing it right once and for all .. my reluctant (against my 
> lazy self ;) preference would be the last.

Can you list the other affected methods?

-

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

Integrated: 8211362: Restrict export of libjpeg symbols from libjavafx_iio.so

[Johan Vos]

On Thu, 25 Mar 2021 12:02:25 GMT, Johan Vos  wrote:

> Fix for JDK-8211362
> 
> Compile javafx-iio native files with -f-visibiliy=hidden in order
> not to export the non-JNI symbols.
> Although this issue was about libjavafx_iio.so only (and not about 
> libjavafx_iio.a), this PR allows fixing the static build as well.
> 
> For static builds, we also use ld -r to build a static library, so that 
> objcopy or similar can be used to remove the names of the
> hidden symbols.

This pull request has now been integrated.

Changeset: ed5cfe72
Author:Johan Vos 
URL:   https://git.openjdk.java.net/jfx/commit/ed5cfe72
Stats: 9 lines in 2 files changed: 5 ins; 0 del; 4 mod

8211362: Restrict export of libjpeg symbols from libjavafx_iio.so

Reviewed-by: kcr

-

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

Re: RFR: 8211362: Restrict export of libjpeg symbols from libjavafx_iio.so

[Kevin Rushforth]

On Thu, 25 Mar 2021 12:02:25 GMT, Johan Vos  wrote:

> Fix for JDK-8211362
> 
> Compile javafx-iio native files with -f-visibiliy=hidden in order
> not to export the non-JNI symbols.
> Although this issue was about libjavafx_iio.so only (and not about 
> libjavafx_iio.a), this PR allows fixing the static build as well.
> 
> For static builds, we also use ld -r to build a static library, so that 
> objcopy or similar can be used to remove the names of the
> hidden symbols.

Looks good. Test build passed. I confirmed that the internal jpeg symbols are 
now hidden.

-

Marked as reviewed by kcr (Lead).

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

RFR: 8264162: PickResult.toString() is missing the closing square bracket

[Kevin Rushforth]

Simple fix to add a missing closing bracket to `PickResult::toString`. This 
includes a unit test that fails without the fix and passes with the fix.

-

Commit messages:
 - 8264162: PickResult.toString() is missing the closing square bracket

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

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

RFR: 8211362: Restrict export of libjpeg symbols from libjavafx_iio.so

[Johan Vos]

Fix for JDK-8211362

Compile javafx-iio native files with -f-visibiliy=hidden in order
not to export the non-JNI symbols.
Although this issue was about libjavafx_iio.so only (and not about 
libjavafx_iio.a), this PR allows fixing the static build as well.

For static builds, we also use ld -r to build a static library, so that objcopy 
or similar can be used to remove the names of the
hidden symbols.

-

Commit messages:
 - remove trailing whitespace
 - Fix for JDK-8211362

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

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

Re: RFR: 8264127: ListCell editing status is true, when index changes while editing

[Jeanette Winzenburg]

On Wed, 24 Mar 2021 14:58:40 GMT, Florian Kirmaier  
wrote:

> Fixing ListCell editing status is true, when index changes while editing.

good catch, yet another property that's not correctly updated on index change 
:) 

Quick questions:
- what are the macroscopic effects (that is in a running list) that you see 
without fixing it?
- do we want mere cell re-use fire a editCancel? (which the list seems to 
ignore in the test, don't know why)

-

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

Re: RFR: 8258777: SkinBase: add api to un-/register invalidation-/listChange listeners [v2]

[Jeanette Winzenburg]

On Wed, 24 Mar 2021 23:53:24 GMT, Nir Lisker  wrote:

>> Jeanette Winzenburg has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   fixed missing/incorrect @since tags in new api doc
>
> I reviewed only the public API methods.

@nlisker thanks for the detailed doc review - I really like your suggestions, 
which I think are considerable improvements (I did part of it in the respective 
methods of the handler) over the original doc :)  Because that's what the doc 
of the new methods are: c from the un/registerChangeListener javadoc, 
adjusted to the specific type of the listener. 

For consistency, all related method doc should be improved along your lines. 
How to get there? Options

- use old doc with new methods, leave improving the doc for all methods to a 
follow-up issue
- keep old doc for old method, use improved doc for the new methods, leave 
improving the doc for the old method to a follow-up issue
- use improved doc in old and new methods in this issue, changing the old

Last would be doing it right once and for all .. my reluctant (against my lazy 
self ;) preference would be the last.

-

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

Re: E-mail addresses at openjdk.org

[Kevin Rushforth]

Eventually the openjdk.org addresses will be live. I haven't heard a 
recent update on the timing for this. Until then, adding the email 
address as an unverified additional email address in your GitHub profile 
is the way to go.


-- Kevin


On 3/24/2021 9:37 PM, John Neffenger wrote:
I noticed that my last commit used the address jgn...@openjdk.org 
instead of the address I normally use to create and sign commits. 
GitHub didn't recognize the commit as mine until I added the address 
to my account.


Do the openjdk.org addresses receive e-mail? If not, do we just leave 
the address unverified in our GitHub accounts?


Thanks,
John