Re: RFR: 8253351: MediaPlayer does not display an mp4 if there no speakers connected to the PC's [v2]

2021-07-27 Thread Johan Vos
On Mon, 26 Jul 2021 22:23:20 GMT, Alexander Matveev  
wrote:

>> Fixed by not failing initialization if DSERR_NODRIVER is returned, which 
>> will be return if device is not present at all. Fixed format initialization 
>> even if DirectSound device was not created in case if audio device will 
>> arrive after playback started. Since we already handle correctly device 
>> arrival after playback started, audio will resume if device is enabled or 
>> USB audio card is plugged back. Due to lack of access to USB audio device, 
>> it was tested by disabling sound card via Device Manager, then starting 
>> playback (video plays, but not audio) and then enabling device and once 
>> enabled audio will start playing.
>
> Alexander Matveev has updated the pull request with a new target base due to 
> a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains two additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'upstream/master' into JDK-8253351
>  - 8253351: MediaPlayer does not display an mp4 if there no speakers 
> connected to the PC's

Looks good. I will do builds and tests, but the idea seems ok. I can imagine 
it's very hard to have a unit or even system test for this.

-

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


Re: [jfx17] RFR: 8250590: Classes and methods in the javafx.css package are missing documentation

2021-07-27 Thread Ajit Ghaisas
On Mon, 26 Jul 2021 18:58:59 GMT, Kevin Rushforth  wrote:

>> This PR corrects/adds missing documentation for classes in javafx.css 
>> package.
>
> modules/javafx.graphics/src/main/java/javafx/css/StyleClass.java line 60:
> 
>> 58: 
>> 59: /**
>> 60:  * Returns the index of this StyleClass in styleClasses list.
> 
> Where is the `styleClasses` list defined? This may be beyond the scope of 
> this fix, so a follow-on issue could be filed. At the least, you should add 
> "the" before "styleClasses".

Added "the" as suggested.
I have reused the text existing near private member. Hence, do not know the 
exact context. I have filed 
[JDK-8271326](https://bugs.openjdk.java.net/browse/JDK-8271326) for this.

-

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


Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v3]

2021-07-27 Thread Jeanette Winzenburg
On Fri, 9 Jul 2021 09:46:04 GMT, Jeanette Winzenburg  
wrote:

>
>  // another test, exposing one of the NPEs in createList
>  ComboBox comboBox = new ComboBox<>(items);
>  comboBox.setSelectionModel(null);
>  installDefaultSkin(comboBox);
> 

> the difference is that setup already installs a skin - so at the time of init 
> the selectionModel still is available ;)

still failing 

> 
> The other access is in the listener to listView's selectedIndex .. it might 
> be a bit tricky to expose in a test, I would go for a combo in a 
> stage/loader, access the list and change its selectedIndex (maybe needs the 
> popup open and/or firing a key onto it)

good fix and test :)

-

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


Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v5]

2021-07-27 Thread Jeanette Winzenburg
On Thu, 22 Jul 2021 19:49:44 GMT, Marius Hanl  wrote:

>> This PR fixes multiple NPEs in Choice-and ComboBox, when the selection model 
>> is null.
>> 
>> ChoiceBox: 
>> - Null check in **valueProperty()** listener
>> 
>> ComboBox:
>> - Null check in **editableProperty()** listener
>> - Null check in **valueProperty()** listener
>> - Null check in **ComboBoxListViewSkin#updateValue()**
>> - Null check in **ComboBoxListViewSkin#layoutChildren()**
>> - Null check in **ComboBoxListViewSkin#createListView()**
>> 
>> ~~The tests checks, that no NPE is printed to the console.
>> Some checks, that the set value is still displayed (either in the ComboBox 
>> button cell or the ChoiceBox display label)~~
>
> Marius Hanl has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - removed unneeded assert and added comment
>  - removed unneeded asserts and added another NPE fix.

there were two NPEs in createLists - one is fixed, the other not yet :) See the 
failing test in my comment

copying the comment, don't know if you could find it otherwise (any way to 
un-resolve a review conversation?)

>
>  // another test, exposing one of the NPEs in createList
>  ComboBox comboBox = new ComboBox<>(items);
>  comboBox.setSelectionModel(null);
>  installDefaultSkin(comboBox);
> 

> the difference is that setup already installs a skin - so at the time of init 
> the selectionModel still is available ;)

still failing 

> 
> The other access is in the listener to listView's selectedIndex .. it might 
> be a bit tricky to expose in a test, I would go for a combo in a 
> stage/loader, access the list and change its selectedIndex (maybe needs the 
> popup open and/or firing a key onto it)

good fix and test :)

-

Changes requested by fastegal (Reviewer).

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


Re: [jfx17] RFR: 8250590: Classes and methods in the javafx.css package are missing documentation

2021-07-27 Thread Ajit Ghaisas
On Mon, 26 Jul 2021 19:05:51 GMT, Kevin Rushforth  wrote:

>> This PR corrects/adds missing documentation for classes in javafx.css 
>> package.
>
> modules/javafx.graphics/src/main/java/javafx/css/converter/EffectConverter.java
>  line 83:
> 
>> 81: /**
>> 82:  * Converter to convert DropShadow {@code Effect}
>> 83:  * @since 9
> 
> Good catch on the `@since 9` tag. I'm a little surprised it doesn't inherit 
> the `@since` from the enclosing class if not present in the nested class, but 
> I can see that it doesn't. So we'll to check the docs of all nested classes 
> to make sure that there aren't any missing.

This can only be checked manually.
I have checked and added the @since tag to these *Converter nested classes.

-

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


Re: [jfx17] RFR: 8250590: Classes and methods in the javafx.css package are missing documentation [v2]

2021-07-27 Thread Ajit Ghaisas
> This PR corrects/adds missing documentation for classes in javafx.css package.

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

  8250590 - address review comments

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/589/files
  - new: https://git.openjdk.java.net/jfx/pull/589/files/2feb4d5d..426863c8

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=589&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=589&range=00-01

  Stats: 177 lines in 26 files changed: 30 ins; 0 del; 147 mod
  Patch: https://git.openjdk.java.net/jfx/pull/589.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/589/head:pull/589

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


Eclipse: any way to checkout a PR for review?

2021-07-27 Thread Jeanette Winzenburg



when reviewing a PR with only a few files changed, I simply create a  
local branch and c&p the changes (*cough, pretty sure there's a better  
way, but then that's the most simple ;).


With changes to many files (like f.i.  
https://github.com/openjdk/jfx/pull/569) that still would be doable,  
but rather cumbersome - so looking for something like "gh pr checkout  
569" (not that I ever tried that, just copied from the doc :) inside  
Eclipse.


An alternative might be to fork-the-fork .. ?

-- Jeanette



Re: Eclipse: any way to checkout a PR for review?

2021-07-27 Thread Kevin Rushforth

I do something like this:

git fetch upstream pull/569/head:pr_569

Then you can checkout the "pr_569" branch. I typically will then merge 
in the current upstream/master to test. If you don't have an "upstream" 
remote you can instead:


git fetch https://github.com/openjdk/jfx pull/569/head:pr_569

-- Kevin

On 7/27/2021 4:15 AM, Jeanette Winzenburg wrote:


when reviewing a PR with only a few files changed, I simply create a 
local branch and c&p the changes (*cough, pretty sure there's a better 
way, but then that's the most simple ;).


With changes to many files (like f.i. 
https://github.com/openjdk/jfx/pull/569) that still would be doable, 
but rather cumbersome - so looking for something like "gh pr checkout 
569" (not that I ever tried that, just copied from the doc :) inside 
Eclipse.


An alternative might be to fork-the-fork .. ?

-- Jeanette





Re: Eclipse: any way to checkout a PR for review?

2021-07-27 Thread Jeanette Winzenburg



ahh .. yeah, that looks doable - had hoped to get away without  
installing git and frickle with commandlines ;)


thanks

Zitat von Kevin Rushforth :


I do something like this:

git fetch upstream pull/569/head:pr_569

Then you can checkout the "pr_569" branch. I typically will then  
merge in the current upstream/master to test. If you don't have an  
"upstream" remote you can instead:


git fetch https://github.com/openjdk/jfx pull/569/head:pr_569

-- Kevin

On 7/27/2021 4:15 AM, Jeanette Winzenburg wrote:


when reviewing a PR with only a few files changed, I simply create  
a local branch and c&p the changes (*cough, pretty sure there's a  
better way, but then that's the most simple ;).


With changes to many files (like f.i.  
https://github.com/openjdk/jfx/pull/569) that still would be  
doable, but rather cumbersome - so looking for something like "gh  
pr checkout 569" (not that I ever tried that, just copied from the  
doc :) inside Eclipse.


An alternative might be to fork-the-fork .. ?

-- Jeanette







Re: [jfx17] RFR: 8250590: Classes and methods in the javafx.css package are missing documentation [v2]

2021-07-27 Thread Kevin Rushforth
On Tue, 27 Jul 2021 10:00:10 GMT, Ajit Ghaisas  wrote:

>> modules/javafx.graphics/src/main/java/javafx/css/converter/EffectConverter.java
>>  line 83:
>> 
>>> 81: /**
>>> 82:  * Converter to convert DropShadow {@code Effect}
>>> 83:  * @since 9
>> 
>> Good catch on the `@since 9` tag. I'm a little surprised it doesn't inherit 
>> the `@since` from the enclosing class if not present in the nested class, 
>> but I can see that it doesn't. So we'll to check the docs of all nested 
>> classes to make sure that there aren't any missing.
>
> This can only be checked manually.
> I have checked and added the @since tag to these *Converter nested classes.

I ran a script to check for missing `@since` tags. The following class are 
still missing them:


DeriveColorConverter
DeriveSizeConverter
StopConverter
StringConverter.SequenceConverter

-

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


Re: [jfx17] RFR: 8250590: Classes and methods in the javafx.css package are missing documentation [v2]

2021-07-27 Thread Ajit Ghaisas
On Tue, 27 Jul 2021 12:03:02 GMT, Kevin Rushforth  wrote:

>> This can only be checked manually.
>> I have checked and added the `@since` tag to these *Converter nested classes.
>
> I ran a script to check for missing `@since` tags. The following class are 
> still missing them:
> 
> 
> DeriveColorConverter
> DeriveSizeConverter
> StopConverter
> StringConverter.SequenceConverter

Ah.. Thanks for running the script. It is tough to find it manually. I will fix 
these soon.

-

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


Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v5]

2021-07-27 Thread Marius Hanl
On Tue, 27 Jul 2021 09:55:47 GMT, Jeanette Winzenburg  
wrote:

>  ComboBox comboBox = new ComboBox<>(items);
>  comboBox.setSelectionModel(null);
>  installDefaultSkin(comboBox);

Ahh yes, I see, nice catch. I will fix it and update this PR. 

But I'm wondering, this might require more as we don't set this dirty flag 
anymore as soon as we install a selection model again.
But thinking about this more, this would be a general problem as soon as we 
switch the combox selection model.
EDIT: Or maybe there is no issue at all and this listener can be removed.

-

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


Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v6]

2021-07-27 Thread Marius Hanl
> This PR fixes multiple NPEs in Choice-and ComboBox, when the selection model 
> is null.
> 
> ChoiceBox: 
> - Null check in **valueProperty()** listener
> 
> ComboBox:
> - Null check in **editableProperty()** listener
> - Null check in **valueProperty()** listener
> - Null check in **ComboBoxListViewSkin#updateValue()**
> - Null check in **ComboBoxListViewSkin#layoutChildren()**
> - Null check in **ComboBoxListViewSkin#createListView()**
> 
> ~~The tests checks, that no NPE is printed to the console.
> Some checks, that the set value is still displayed (either in the ComboBox 
> button cell or the ChoiceBox display label)~~

Marius Hanl has updated the pull request incrementally with one additional 
commit since the last revision:

  Fixed another NPE on skin creation when the cbx selection model is null

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/557/files
  - new: https://git.openjdk.java.net/jfx/pull/557/files/039b826b..40a30e08

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=557&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=557&range=04-05

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

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


Re: [jfx17] RFR: 8250590: Classes and methods in the javafx.css package are missing documentation [v3]

2021-07-27 Thread Ajit Ghaisas
> This PR corrects/adds missing documentation for classes in javafx.css package.

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

  8250590 - add missing @since tag

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/589/files
  - new: https://git.openjdk.java.net/jfx/pull/589/files/426863c8..76439ae4

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=589&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=589&range=01-02

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

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


Re: [jfx17] RFR: 8240640: [macos] Wrong focus behaviour with multiple Alerts [v3]

2021-07-27 Thread Ambarish Rapte
On Fri, 23 Jul 2021 17:40:32 GMT, Pankaj Bansal  wrote:

>> The bug is a regression as a result of fix done for JDK-8227366 and is 
>> reproducible on Linux and Mac. This fix is being reverted in this change and 
>> a new bug (JDK-8271054) has been created to redo the JDK-8227366
>> 
>> An automated testcase is being added to make sure similar regression is not 
>> introduced when working on the redo bug. The automated testcase fails 
>> without the current change and passes after the change.
>> 
>> The testcase works fine on Mac and Linux, but I see some issues in windows. 
>> The stage is minimised after calling stage.show without calling 
>> stage.setAlwaysOnTop(true). I see that there are other tests like 
>> DualWindowsTest.java which are failing for same reason. Also, if I remove 
>> the stage.setAlwaysOnTop(true) from tests like 
>> FocusParentWindowOnChildCloseTest.java,  the stage remains minimised there 
>> as well. This seems wrong as calling stage.setAlwaysOnTop(true) should not 
>> be required to bring stage window in focus on running the test. This may be 
>> an issue on my Window machine or something similar. I hope reviewers can 
>> shed some light on this.
>
> Pankaj Bansal has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix formatting for binary operators

looks good to me.

Marked as reviewed by arapte (Reviewer).

-

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


[jfx17] Integrated: 8240640: [macos] Wrong focus behaviour with multiple Alerts

2021-07-27 Thread Pankaj Bansal
On Mon, 19 Jul 2021 19:52:05 GMT, Pankaj Bansal  wrote:

> The bug is a regression as a result of fix done for JDK-8227366 and is 
> reproducible on Linux and Mac. This fix is being reverted in this change and 
> a new bug (JDK-8271054) has been created to redo the JDK-8227366
> 
> An automated testcase is being added to make sure similar regression is not 
> introduced when working on the redo bug. The automated testcase fails without 
> the current change and passes after the change.
> 
> The testcase works fine on Mac and Linux, but I see some issues in windows. 
> The stage is minimised after calling stage.show without calling 
> stage.setAlwaysOnTop(true). I see that there are other tests like 
> DualWindowsTest.java which are failing for same reason. Also, if I remove the 
> stage.setAlwaysOnTop(true) from tests like 
> FocusParentWindowOnChildCloseTest.java,  the stage remains minimised there as 
> well. This seems wrong as calling stage.setAlwaysOnTop(true) should not be 
> required to bring stage window in focus on running the test. This may be an 
> issue on my Window machine or something similar. I hope reviewers can shed 
> some light on this.

This pull request has now been integrated.

Changeset: 2cd5d1fd
Author:Pankaj Bansal 
URL:   
https://git.openjdk.java.net/jfx/commit/2cd5d1fd0fd3e7d974b46bd43a6f98dbca6f28c0
Stats: 135 lines in 2 files changed: 134 ins; 0 del; 1 mod

8240640: [macos] Wrong focus behaviour with multiple Alerts

Reviewed-by: kcr, arapte

-

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


Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v6]

2021-07-27 Thread Jeanette Winzenburg
On Tue, 27 Jul 2021 12:41:00 GMT, Marius Hanl  wrote:

>> This PR fixes multiple NPEs in Choice-and ComboBox, when the selection model 
>> is null.
>> 
>> ChoiceBox: 
>> - Null check in **valueProperty()** listener
>> 
>> ComboBox:
>> - Null check in **editableProperty()** listener
>> - Null check in **valueProperty()** listener
>> - Null check in **ComboBoxListViewSkin#updateValue()**
>> - Null check in **ComboBoxListViewSkin#layoutChildren()**
>> - Null check in **ComboBoxListViewSkin#createListView()**
>> 
>> ~~The tests checks, that no NPE is printed to the console.
>> Some checks, that the set value is still displayed (either in the ComboBox 
>> button cell or the ChoiceBox display label)~~
>
> Marius Hanl has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed another NPE on skin creation when the cbx selection model is null

looks good :)

-

Marked as reviewed by fastegal (Reviewer).

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


Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v5]

2021-07-27 Thread Jeanette Winzenburg
On Tue, 27 Jul 2021 12:36:52 GMT, Marius Hanl  wrote:

> But I'm wondering, this might require more as we don't set this dirty flag 
> anymore as soon as we install a selection model again.
> But thinking about this even more, this would be a general problem as soon as 
> we switch the combobox selection model.
> EDIT: Or maybe there is no issue at all and this listener can be removed.

the interaction of selection listener wiring in combo skin is complex and buggy 
(there are many issues already reported, work started, work aborted ..) - off 
scope for this PR, IMO

-

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


Re: Eclipse: any way to checkout a PR for review?

2021-07-27 Thread Nir Lisker
I'm not really sure what you mean. If you pull from the remote that the PR
is on and checkout the remote branch, is it not good enough for a review?

On Tue, Jul 27, 2021 at 2:16 PM Jeanette Winzenburg 
wrote:

>
> when reviewing a PR with only a few files changed, I simply create a
> local branch and c&p the changes (*cough, pretty sure there's a better
> way, but then that's the most simple ;).
>
> With changes to many files (like f.i.
> https://github.com/openjdk/jfx/pull/569) that still would be doable,
> but rather cumbersome - so looking for something like "gh pr checkout
> 569" (not that I ever tried that, just copied from the doc :) inside
> Eclipse.
>
> An alternative might be to fork-the-fork .. ?
>
> -- Jeanette
>
>


[jfx17] Withdrawn: 8268642: Expand the javafx.beans.property.Property documentation

2021-07-27 Thread Michael Strauß
On Mon, 14 Jun 2021 17:06:34 GMT, Michael Strauß  wrote:

> * Expand the `Property.bind` and `Property.bindBidirectional` documentation
> * Change the name of the formal parameter of `Property.bind` to "source" 
> (currently, it is inconsistently named "observable", "rawObservable" or 
> "newObservable")

This pull request has been closed without being integrated.

-

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


Improve property system to facilitate correct usage

2021-07-27 Thread Michael Strauß
I propose a set of changes to the JavaFX property system that I've
outlined in this PR: https://github.com/openjdk/jfx/pull/590

The changes fall into two categories: enforcement of correct usage
(there are several cases listed in the PR), and deprecating untyped
APIs (for removal in a future version) so as to make the intent of the
API more clear to developers.

Even though there are breaking changes, the impact on application code
should be minimal. I'd welcome any comments on this proposal.


Re: Content binding API

2021-07-27 Thread Kevin Rushforth
Looking at content bindings at the same time as regular bindings makes a 
lot of sense, as long as the proposed changes don't result in the loss 
of any well-defined, workable functionality (e.g., tightening the 
restriction that a property P1 that is unidirectionally bound to another 
property P2 cannot also be bound bidirectionally to a property P3 is not 
a loss of any "workable" functionality).


I'll need a little more time to look at the API implications of your 
proposed changes (and questions) for content bindings. One thing to keep 
in mind is that content bindings are often created via the "Bindings" 
utility class, for which the object to be bound is a plain List or Map 
(not an Observable). That likely has something to do with the reason for 
the unbind methods in the properties taking an Object rather than an 
observable.


-- Kevin


On 7/24/2021 7:02 AM, Michael Strauß wrote:

There has been some discussion in this PR
https://github.com/openjdk/jfx/pull/533 on the semantics of
unidirectional and bidirectional bindings.

I think we've come to the understanding that unidirectional and
bidirectional bindings cannot be meaningfully used at the same time,
and such usage should be specified to be illegal and fail fast.

I think the same argument is true for content bindings, which should
mirror the semantics of regular bindings.

1. Let's look at the API of regular bindings:

void bind(ObservableValue)
void unbind()

void bindBidirectional(Property)
void unbindBidirectional(Property)

It makes intuitive sense that unbind() does not take an argument,
because there can only be a single unidirectional binding at a time,
while unbindBidirectional(Property) needs to know which bidirectional
binding should be unbound.

2. Now let's look at the API of content bindings:

void bindContent(ObservableList)
void unbindContent(Object)

void bindContentBidirectional(ObservableList)
void unbindContentBidirectional(Object)

It's a bit unclear why unbindContent(Object) requires an argument, let
alone of type Object. Mirroring regular bindings, we could potentially
introduce a new method unbindContent() without an argument, and
deprecate the other method not for removal.

In the case of unbindContentBidirectional(Object), I'm not sure why
the method doesn't accept an argument of type ObservableList. Can
someone educate me if there is a reason for this seeming inconsistency
with bindContentBidirectional(ObservableList)?`

If there is no reason for the Object argument, "fixing" this would
obviously be more problematic because it would be a
binary-incompatible change.




Re: Improve property system to facilitate correct usage

2021-07-27 Thread Kevin Rushforth
This will take a while to work through, and we will need to get general 
consensus on the API changes.


I doubt I can support incompatible breaking changes in this area, given 
how fundamental property and bindings are to JavaFX. I'll take a look, 
but it is likely that the incompatible API changes part of your proposed 
change will not be accepted.


The changes enforcing correct usage should be a lot less controversial 
and easier to get through.


-- Kevin


On 7/27/2021 4:23 PM, Michael Strauß wrote:

I propose a set of changes to the JavaFX property system that I've
outlined in this PR: https://github.com/openjdk/jfx/pull/590

The changes fall into two categories: enforcement of correct usage
(there are several cases listed in the PR), and deprecating untyped
APIs (for removal in a future version) so as to make the intent of the
API more clear to developers.

Even though there are breaking changes, the impact on application code
should be minimal. I'd welcome any comments on this proposal.




Re: Improve property system to facilitate correct usage

2021-07-27 Thread Michael Strauß
I should point out that the rest of the JavaFX framework did not
require a single code change as a result of the API changes. So while
some changes are binary incompatible, they are syntactically
transparent.

Am Mi., 28. Juli 2021 um 01:39 Uhr schrieb Kevin Rushforth
:
>
> This will take a while to work through, and we will need to get general
> consensus on the API changes.
>
> I doubt I can support incompatible breaking changes in this area, given
> how fundamental property and bindings are to JavaFX. I'll take a look,
> but it is likely that the incompatible API changes part of your proposed
> change will not be accepted.
>
> The changes enforcing correct usage should be a lot less controversial
> and easier to get through.
>
> -- Kevin
>
>
> On 7/27/2021 4:23 PM, Michael Strauß wrote:
> > I propose a set of changes to the JavaFX property system that I've
> > outlined in this PR: https://github.com/openjdk/jfx/pull/590
> >
> > The changes fall into two categories: enforcement of correct usage
> > (there are several cases listed in the PR), and deprecating untyped
> > APIs (for removal in a future version) so as to make the intent of the
> > API more clear to developers.
> >
> > Even though there are breaking changes, the impact on application code
> > should be minimal. I'd welcome any comments on this proposal.
>


Re: [External] : Re: Improve property system to facilitate correct usage

2021-07-27 Thread Kevin Rushforth




some changes are binary incompatible, they are syntactically
transparent.


Yes, that's the big problem. Binary compatibility is very important. The 
value proposition of making the types a bit more clear doesn't justify 
breaking binary compatibility.


-- Kevin


On 7/27/2021 6:09 PM, Michael Strauß wrote:

I should point out that the rest of the JavaFX framework did not
require a single code change as a result of the API changes. So while
some changes are binary incompatible, they are syntactically
transparent.

Am Mi., 28. Juli 2021 um 01:39 Uhr schrieb Kevin Rushforth
:

This will take a while to work through, and we will need to get general
consensus on the API changes.

I doubt I can support incompatible breaking changes in this area, given
how fundamental property and bindings are to JavaFX. I'll take a look,
but it is likely that the incompatible API changes part of your proposed
change will not be accepted.

The changes enforcing correct usage should be a lot less controversial
and easier to get through.

-- Kevin


On 7/27/2021 4:23 PM, Michael Strauß wrote:

I propose a set of changes to the JavaFX property system that I've
outlined in this PR: 
https://urldefense.com/v3/__https://github.com/openjdk/jfx/pull/590__;!!ACWV5N9M2RV99hQ!egor-rENORC_wn09Va7FeNBmsgCGsCm3yzccC3yvO_x-wuuyMXrzpE_OplPNe2pJWeue$

The changes fall into two categories: enforcement of correct usage
(there are several cases listed in the PR), and deprecating untyped
APIs (for removal in a future version) so as to make the intent of the
API more clear to developers.

Even though there are breaking changes, the impact on application code
should be minimal. I'd welcome any comments on this proposal.