Re: RFR: 8255248: NullPointerException in JFXPanel due to race condition in HostContainer [v3]

[Prasanta Sadhukhan]

On Mon, 24 Jul 2023 17:15:18 GMT, Andy Goryachev  wrote:

>> This code is still accessing shared state (`dnd`, `pWidth`, `pHeight`, 
>> `scaleFactorX`, `scaleFactorY`). Without further analysis of when and where 
>> these values are written, it is unclear whether this is the right move. In 
>> general, allowing interleaved code execution on multiple threads is very 
>> hard to get right. Better to not do it, if at all possible.
>
> No doubt.  `dnd`, even though it's commented as "// Accessed on EDT only" 
> (line 152) seems to be accessed from FX thread via setFxEnabled line 866.
> I tend to agree with @mstr2 that this component deserves a re-think and 
> perhaps a re-design (starting with clear separation of which methods can be 
> accessed from which thread, or adding explicit synchronization primitives).
>  I do recall seeing a number of issues with swing/fx interop in jdk8.

I have synchronized other shared resources.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1273010135

Re: RFR: 8255248: NullPointerException in JFXPanel due to race condition in HostContainer [v5]

[Prasanta Sadhukhan]

> Due to transient datatype of scenePeer, it can become null which can result 
> in NPE in scenarios where scene is continuously been reset and set, which 
> warrants a null check, as is done in other places for the same variable.

Prasanta Sadhukhan has updated the pull request incrementally with two 
additional commits since the last revision:

 - Locking for all shared resource between Swing and FX thread
 - Locking for all shared resource between Swing and FX thread

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1178/files
  - new: https://git.openjdk.org/jfx/pull/1178/files/f078f0a2..595f40d6

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1178=04
 - incr: https://webrevs.openjdk.org/?repo=jfx=1178=03-04

  Stats: 116 lines in 1 file changed: 71 ins; 0 del; 45 mod
  Patch: https://git.openjdk.org/jfx/pull/1178.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1178/head:pull/1178

PR: https://git.openjdk.org/jfx/pull/1178

Re: CSS Transitions

[Michael Strauß]

My comments below:

On Tue, Jul 25, 2023 at 1:18 AM Kevin Rushforth
 wrote:
>
> This seems like it might be a useful feature, if enough applications
> would want to take advantage of it.
>
> If we proceed, I have a couple comments:
>
> * All of our existing CSS attributes use "-fx-" as a prefix. My
> preference would be to do that for transitions as well, absent a
> compelling reason to do otherwise. I note that you say in your design
> doc that transition "...is special and distinct from all other CSS
> properties". Is that the reason you didn't prefix it with "-fx-"? Is it
> a sufficient reason?

The vendor prefix indicates a nonstandard/proprietary CSS property (see
also https://www.w3.org/TR/CSS/#proprietary). That's the case for almost
all JavaFX CSS properties (with one notable exception: "visibility").
However, "transition" is an exact implementation of the CSS standard, so
there's no need to use a vendor prefix. I think that would be setting a wrong
expectation, as CSS developers might think that "-fx-transition" works
differently than a standard-conforming "transition" property, when in reality
it's the same.


> * Initially, I wondered about your providing the CSS attributes without
> corresponding API on the scene graph objects in question, but I think
> that's a very good idea.

Note that if there's ever a compelling need to have an API on SG objects,
this can be retrofitted. But I doubt that such an API would be useful.


> * This will need a lot of testing since it touches the CSS attribute
> resolution mechanism.

Agreed.

Re: CSS Transitions

[Kevin Rushforth]

This seems like it might be a useful feature, if enough applications 
would want to take advantage of it.


If we proceed, I have a couple comments:

* All of our existing CSS attributes use "-fx-" as a prefix. My 
preference would be to do that for transitions as well, absent a 
compelling reason to do otherwise. I note that you say in your design 
doc that transition "...is special and distinct from all other CSS 
properties". Is that the reason you didn't prefix it with "-fx-"? Is it 
a sufficient reason?


* Initially, I wondered about your providing the CSS attributes without 
corresponding API on the scene graph objects in question, but I think 
that's a very good idea.


* This will need a lot of testing since it touches the CSS attribute 
resolution mechanism.


-- Kevin


On 7/17/2023 12:23 PM, Michael Strauß wrote:

Here's an updated summary of the proposed feature:
https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a

And here's the full implementation:
https://github.com/openjdk/jfx/pull/870

I'm interested in hearing from you whether you think this is a useful
feature and whether the proposed API is a good match for JavaFX.

Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]

[John Hendrikx]

On Mon, 24 Jul 2023 19:56:06 GMT, Michael Strauß  wrote:

>> John Hendrikx has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fix generic warnings
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java 
> line 143:
> 
>> 141:  */
>> 142: public void fireValueChanged(I instance, T oldValue) {
>> 143: Object data = getData(instance);
> 
> The `data` value could be passed into this method, which would save a 
> (potentially not devirtualized) method call.

Thanks, I'll look into that, it might speed up the 1 listener cases a bit.  The 
same applies to OldValueCachingListenerManager#getValue I think.  I know it 
isn't possible for the add/remove calls, as the data may change if they're 
nested, but for `fireValueChanged` I never really checked after going to this 
strategy.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272805838

Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]

[John Hendrikx]

On Mon, 24 Jul 2023 19:58:04 GMT, Michael Strauß  wrote:

>> John Hendrikx has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fix generic warnings
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java 
> line 145:
> 
>> 143: Object data = getData(instance);
>> 144: 
>> 145: if (data instanceof ListenerList) {
> 
> Why is `ListenerList` checked first, when most observables only have a single 
> `InvalidationListener`?

For some (unclear to me) reason this order performs better in my benchmark, 
even for the cases that only have a single invalidation listener. I've tweaked 
this method extensively, with different orders, and this was about the best I 
could get it. That said, the differences are small, and we can go with a more 
logical order.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272801049

Re: RFR: JDK-8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v11]

[Michael Strauß]

On Fri, 9 Jun 2023 12:00:06 GMT, John Hendrikx  wrote:

>> This provides and uses a new implementation of `ExpressionHelper`, called 
>> `ListenerManager` with improved semantics.
>> 
>> # Behavior
>> 
>> |Listener...|ExpressionHelper|ListenerManager|
>> |---|---|---|
>> |Invocation Order|In order they were registered, invalidation listeners 
>> always before change listeners|(unchanged)|
>> |Removal during Notification|All listeners present when notification started 
>> are notified, but excluded for any nested changes|Listeners are removed 
>> immediately regardless of nesting|
>> |Addition during Notification|Only listeners present when notification 
>> started are notified, but included for any nested changes|New listeners are 
>> never called during the current notification regardless of nesting|
>> 
>> ## Nested notifications:
>> 
>> | |ExpressionHelper|ListenerManager|
>> |---|---|---|
>> |Type|Depth first (call stack increases for each nested level)|(same)|
>> |# of Calls|Listeners * Depth (using incorrect old values)|Collapses nested 
>> changes, skipping non-changes|
>> |Vetoing Possible?|No|Yes|
>> |Old Value correctness|Only for listeners called before listeners making 
>> nested changes|Always|
>> 
>> # Performance
>> 
>> |Listener|ExpressionHelper|ListenerManager|
>> |---|---|---|
>> |Addition|Array based, append in empty slot, resize as needed|(same)|
>> |Removal|Array based, shift array, resize as needed|(same)|
>> |Addition during notification|Array is copied, removing collected 
>> WeakListeneres in the process|Tracked (append at end)|
>> |Removal during notification|As above|Entry is `null`ed|
>> |Notification completion with changes|-|Null entries (and collected 
>> WeakListeners) are removed|
>> |Notifying Invalidation Listeners|1 ns each|(same)|
>> |Notifying Change Listeners|1 ns each (*)|2-3 ns each|
>> 
>> (*) a simple for loop is close to optimal, but unfortunately does not 
>> provide correct old values
>> 
>> # Memory Use 
>> 
>> Does not include alignment, and assumes a 32-bit VM or one that is using 
>> compressed oops.
>> 
>> |Listener|ExpressionHelper|ListenerManager|OldValueCaching ListenerManager|
>> |---|---|---|---|
>> |No Listeners|none|none|none|
>> |Single InvalidationListener|16 bytes overhead|none|none|
>> |Single ChangeListener|20 bytes overhead|none|16 bytes overhead|
>> |Multiple listeners|57 + 4 per listener (excluding unused slots)|57 + 4 per 
>> listener (excluding unused slots)|61 + 4 per listener (excluding unused 
>> slots)|
>> 
>> # About nested changes
>> 
>> Nested changes are simply changes that are made to a property that is 
>> currently in the process of notif...
>
> John Hendrikx has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix generic warnings

modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java 
line 143:

> 141:  */
> 142: public void fireValueChanged(I instance, T oldValue) {
> 143: Object data = getData(instance);

The `data` value could be passed into this method, which would save a 
(potentially not devirtualized) method call.

modules/javafx.base/src/main/java/com/sun/javafx/binding/ListenerManager.java 
line 145:

> 143: Object data = getData(instance);
> 144: 
> 145: if (data instanceof ListenerList) {

Why is `ListenerList` checked first, when most observables only have a single 
`InvalidationListener`?

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272690289
PR Review Comment: https://git.openjdk.org/jfx/pull/1081#discussion_r1272692011

Re: RFR: 8309558: Create implementation of NSAccessibilityCheckBox protocol [v2]

[Alexander Zuev]

> also
> 8309629: Create implementation of NSAccessibilityRadioButton protocol
> 
> Create implementation of NSAccessibilityCheckBox and 
> NSAccessibilityRadioButton protocols
> Add workaround for the wrong focus owner announcement with radio buttons
> Add workaround for wrong magnifier text on buttons

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

  Add newline at the end of the header files.

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1184/files
  - new: https://git.openjdk.org/jfx/pull/1184/files/8010b18a..1ce209c1

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1184=01
 - incr: https://webrevs.openjdk.org/?repo=jfx=1184=00-01

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jfx/pull/1184.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1184/head:pull/1184

PR: https://git.openjdk.org/jfx/pull/1184

Re: RFR: 8301302: Platform preferences API [v3]

[Alessadro Parisi]

On Tue, 30 May 2023 15:37:29 GMT, Michael Strauß  wrote:

> > Talking about #511 and #1014. I think that both the APIs are quite nice 
> > additions to JavaFX. However I was thinking, @mstr2 can't the two features 
> > be split? It seems to me, correct me if I'm wrong, that the Platform 
> > Preferences API still requires quite some work. On the other hand the 
> > support for themes seems to be easier and faster to implement. I think it 
> > would be worth splitting the two features, and trying to add the Theme API 
> > by the next release
> 
> I've already proposed to deliver the feature in three installments to make it 
> easier to get it agreed upon and reviewed:
> 
> 1. [Platform Preferences 
> API](https://bugs.openjdk.org/browse/JDK-8301302) (this PR)
> 
> 2. [Allow Stage to control the appearance of window 
> decorations](https://bugs.openjdk.org/browse/JDK-8301303)
> 
> 3. [Add CSS themes as a first-class 
> concept](https://bugs.openjdk.org/browse/JDK-8267546)
> 
> 
> The Platform Preference API is ready to be reviewed, as far as I'm concerned. 
> While it has been discussed extensively on the mailing list and here on 
> GitHub, the feature proposal as a whole is still lacking buy-in from the 
> OpenJFX project leads.
> 
> Note that getting the Style Theme API in before Platform Preferences is 
> harder than it sounds. The built-in JavaFX themes Caspian and Modena will 
> need to be re-implemented as first-class themes, and that requires Platform 
> Preferences to support high-contrast themes on Windows. The alternative of 
> keeping two different code paths would lead to significant additional 
> maintenance cost.

I was hoping in getting at least the possibility of adding more than one 
stylesheet as the app user-agent, without necessarily implementing the Themes 
API yet.
Guess I'll have to wait a bit more, in the meanwhile I'll try expanding my 
'workaround' of converting multiple stylesheets to a single one on the fly to 
compensate
And again, thanks for your work!

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1569803226

Re: RFR: 8301302: Platform preferences API [v3]

[Alessadro Parisi]

On Thu, 11 May 2023 14:38:01 GMT, Michael Strauß  wrote:

>> Hi all,
>> 
>> First of all, thanks for the efforts to integrate this new API into JavaFX. 
>> As a style theme developer myself I think this API is missing nowadays and 
>> will be a good addition to the SDK!
>> 
>> A comment not related to this specific PR but... the discussion about this 
>> new API was being held on the mailing list some time ago, I was also 
>> involved in that discussion. It stopped so I thought this wasn't going to go 
>> forward. I've only recently realized the discussion moved to here. So, my 
>> comment is: is there a centralized place to check on all the new 
>> developments in the javafx platform including ongoing efforts (I thought the 
>> mailing list was that place) ...?
>> For instance, I was having a small discussion on Twitter with a prominent 
>> member of the JavaFX community about how having this kind of API would be of 
>> interest to JavaFX. I don't think anyone involved knows that this PR (that 
>> would add such a feature) is ongoing... they might also have interesting 
>> insights to add...
>
> @dukke I don't think there's a central place for all JavaFX discussions. I've 
> observed that high-level discussions seem to happen on the mailing list, 
> while discussions of concrete implementation proposals often happen on 
> GitHub. You didn't get any notifications on the mailing list for this PR 
> since it's still in the Draft state.
> 
> That being said, this proposal seems to be stuck in the "do we want this in 
> JavaFX?" stage.

> @mstr2 I see minor bugs and minor features as well as implementation details 
> being discussed in the mailing list. I think a centralized place to see all 
> that's being done in the javafx sdk (including proposed features) would be 
> nice.
> 
> Personally, I feel that the ability to specify multiple user-agent 
> stylesheets is a no-brainer. Right now, my javafx theme JMetro has to be 
> composed of scene stylesheets which can be a hassle for many developers using 
> it (given how it overrides styles set in code).
> 
> The other features are also important and I would be happy to see them in the 
> javafx sdk, especially since I'm a theme developer and I've been missing 
> these features for a while.
> 
> Perhaps if these features are discussed individually (for example only 
> discussing the ability to have multiple user agent stylesheets) they would be 
> "easier to sell"? And all the other ones, each in its own PR. At least they 
> would be easier to discuss (as each PR would be less complex) and so 
> potentially easier to sell. Without however missing the whole picture like 
> you're doing here...

I agree. Currently, the hardest challenge I'm facing is to find a way to style 
my applications entirely. Adding the stylesheets on the main Scene preserves 
the JavaFX user agent while still allowing me to style all my custom components 
in that Scene. The issue is that obviously, styles are not applied to other 
Windows/Scenes, which means that Dialogs are problematic components. Adding the 
entire theme on them leads to awful performance, and so the only way to also 
include them is to set the user agent...but then JavaFX controls would lose 
their styles. Recently, I came up with a naive workaround for this. I coded a 
mechanism that allows to merge/combine multiple stylesheets into one. The good 
thing about this is that I can produce a single user agent stylesheet that can 
cover both JavaFX controls and my controls in every single Window/Scene. The 
bad thing is that at-rules do not work, so no typefaces, no other stylesheets, 
only full themes can be merged. For the fonts, I found out th
 at it's enough to add the stylesheet containing the font-faces declarations on 
the main Scene, and they will be picked everywhere.

Talking about #511 and #1014. I think that both the APIs are quite nice 
additions to JavaFX. However I was thinking, @mstr2 can't the two features be 
split? It seems to me, correct me if I'm wrong, that the Platform Preferences 
API  still requires quite some work. On the other hand the support for themes 
seems to be easier and faster to implement. I think it would be worth splitting 
the two features, and trying to add the Theme API by the next release

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1566645669

Re: RFR: 8301302: Platform preferences API [v3]

[Pedro Duque Vieira]

On Thu, 11 May 2023 14:38:01 GMT, Michael Strauß  wrote:

>> Hi all,
>> 
>> First of all, thanks for the efforts to integrate this new API into JavaFX. 
>> As a style theme developer myself I think this API is missing nowadays and 
>> will be a good addition to the SDK!
>> 
>> A comment not related to this specific PR but... the discussion about this 
>> new API was being held on the mailing list some time ago, I was also 
>> involved in that discussion. It stopped so I thought this wasn't going to go 
>> forward. I've only recently realized the discussion moved to here. So, my 
>> comment is: is there a centralized place to check on all the new 
>> developments in the javafx platform including ongoing efforts (I thought the 
>> mailing list was that place) ...?
>> For instance, I was having a small discussion on Twitter with a prominent 
>> member of the JavaFX community about how having this kind of API would be of 
>> interest to JavaFX. I don't think anyone involved knows that this PR (that 
>> would add such a feature) is ongoing... they might also have interesting 
>> insights to add...
>
> @dukke I don't think there's a central place for all JavaFX discussions. I've 
> observed that high-level discussions seem to happen on the mailing list, 
> while discussions of concrete implementation proposals often happen on 
> GitHub. You didn't get any notifications on the mailing list for this PR 
> since it's still in the Draft state.
> 
> That being said, this proposal seems to be stuck in the "do we want this in 
> JavaFX?" stage.

@mstr2 
I see minor bugs and minor features as well as implementation details being 
discussed in the mailing list. I think a centralized place to see all that's 
being done in the javafx sdk (including proposed features) would be nice.

Personally, I feel that the ability to specify multiple user-agent stylesheets 
is a no-brainer. Right now, my javafx theme JMetro has to be composed of scene 
stylesheets which can be a hassle for many developers using it (given how it 
overrides styles set in code).

The other features are also important and I would be happy to see them in the 
javafx sdk, especially since I'm a theme developer and I've been missing these 
features for a while.

Perhaps if these features are discussed individually (for example only 
discussing the ability to have multiple user agent stylesheets) they would be 
"easier to sell"? And all the other ones, each in its own PR. At least they 
would be easier to discuss (as each PR would be less complex) and so 
potentially easier to sell. Without however missing the whole picture like 
you're doing here...

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1544126580

Re: RFR: 8301302: Platform preferences API [v3]

[Michael Strauß]

On Thu, 11 May 2023 14:38:01 GMT, Michael Strauß  wrote:

>> Hi all,
>> 
>> First of all, thanks for the efforts to integrate this new API into JavaFX. 
>> As a style theme developer myself I think this API is missing nowadays and 
>> will be a good addition to the SDK!
>> 
>> A comment not related to this specific PR but... the discussion about this 
>> new API was being held on the mailing list some time ago, I was also 
>> involved in that discussion. It stopped so I thought this wasn't going to go 
>> forward. I've only recently realized the discussion moved to here. So, my 
>> comment is: is there a centralized place to check on all the new 
>> developments in the javafx platform including ongoing efforts (I thought the 
>> mailing list was that place) ...?
>> For instance, I was having a small discussion on Twitter with a prominent 
>> member of the JavaFX community about how having this kind of API would be of 
>> interest to JavaFX. I don't think anyone involved knows that this PR (that 
>> would add such a feature) is ongoing... they might also have interesting 
>> insights to add...
>
> @dukke I don't think there's a central place for all JavaFX discussions. I've 
> observed that high-level discussions seem to happen on the mailing list, 
> while discussions of concrete implementation proposals often happen on 
> GitHub. You didn't get any notifications on the mailing list for this PR 
> since it's still in the Draft state.
> 
> That being said, this proposal seems to be stuck in the "do we want this in 
> JavaFX?" stage.

> Talking about #511 and #1014. I think that both the APIs are quite nice 
> additions to JavaFX. However I was thinking, @mstr2 can't the two features be 
> split? It seems to me, correct me if I'm wrong, that the Platform Preferences 
> API still requires quite some work. On the other hand the support for themes 
> seems to be easier and faster to implement. I think it would be worth 
> splitting the two features, and trying to add the Theme API by the next 
> release

I've already proposed to deliver the feature in three installments to make it 
easier to get it agreed upon and reviewed:
1. [Platform Preferences API](https://bugs.openjdk.org/browse/JDK-8301302) 
(this PR)
2. [Allow Stage to control the appearance of window 
decorations](https://bugs.openjdk.org/browse/JDK-8301303)
3. [Add CSS themes as a first-class 
concept](https://bugs.openjdk.org/browse/JDK-8267546)

The Platform Preference API is ready to be reviewed, as far as I'm concerned. 
While it has been discussed extensively on the mailing list and here on GitHub, 
the feature proposal as a whole is still lacking buy-in from the OpenJFX 
project leads.

Note that getting the Style Theme API in before Platform Preferences is harder 
than it sounds. The built-in JavaFX themes Caspian and Modena will need to be 
re-implemented as first-class themes, and that requires Platform Preferences to 
support high-contrast themes on Windows. The alternative of keeping two 
different code paths would lead to significant additional maintenance cost.

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1568656832

Re: RFR: 8301302: Platform preferences API [v3]

[Michael Strauß]

On Thu, 11 May 2023 11:38:14 GMT, Pedro Duque Vieira  wrote:

>> Michael Strauß has updated the pull request with a new target base due to a 
>> merge or a rebase.
>
> Hi all,
> 
> First of all, thanks for the efforts to integrate this new API into JavaFX. 
> As a style theme developer myself I think this API is missing nowadays and 
> will be a good addition to the SDK!
> 
> A comment not related to this specific PR but... the discussion about this 
> new API was being held on the mailing list some time ago, I was also involved 
> in that discussion. It stopped so I thought this wasn't going to go forward. 
> I've only recently realized the discussion moved to here. So, my comment is: 
> is there a centralized place to check on all the new developments in the 
> javafx platform including ongoing efforts (I thought the mailing list was 
> that place) ...?
> For instance, I was having a small discussion on Twitter with a prominent 
> member of the JavaFX community about how having this kind of API would be of 
> interest to JavaFX. I don't think anyone involved knows that this PR (that 
> would add such a feature) is ongoing... they might also have interesting 
> insights to add...

@dukke I don't think there's a central place for all JavaFX discussions. I've 
observed that high-level discussions seem to happen on the mailing list, while 
discussions of concrete implementation proposals often happen on GitHub. You 
didn't get any notifications on the mailing list for this PR since it's still 
in the Draft state.

That being said, this proposal seems to be stuck in the "do we want this in 
JavaFX?" stage.

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1544107419

Re: RFR: 8301302: Platform preferences API [v3]

[Pedro Duque Vieira]

On Thu, 2 Feb 2023 19:54:33 GMT, Michael Strauß  wrote:

>> Please read [this 
>> document](https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548) 
>> for an introduction to the Platform Preferences API, and how it interacts 
>> with the proposed style theme and stage appearance features.
>
> Michael Strauß has updated the pull request with a new target base due to a 
> merge or a rebase.

Hi all,

First of all, thanks for the efforts to integrate this new API into JavaFX. As 
a style theme developer myself I think this API is missing nowadays and will be 
a good addition to the SDK!

A comment not related to this specific PR but... the discussion about this new 
API was being held on the mailing list some time ago, I was also involved in 
that discussion. It stopped so I thought this wasn't going to go forward. I've 
only recently realized the discussion moved to here. So, my comment is: is 
there a centralized place to check on all the new developments in the javafx 
platform including ongoing efforts (I thought the mailing list was that place) 
...?
For instance, I was having a small discussion on Twitter with a prominent 
member of the JavaFX community about how having this kind of API would be of 
interest to JavaFX. I don't think anyone involved knows that this PR (that 
would add such a feature) is ongoing... they might also have interesting 
insights to add...

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1543837458

Re: RFR: 8301302: Platform preferences API [v3]

[Michael Strauß]

On Mon, 24 Apr 2023 19:24:31 GMT, Andy Goryachev  wrote:

> 1. ObservableMap.  Is it unmodifiable?  I'd expect it to be, since this API 
> exposes the properties of the underlying platform.

Yes and no. It is unmodifiable as it relates to the `Map` contract: you can't 
add, remove, or change mappings. However, you can _override_ mappings. For 
example, invoking `Platform.getPreferences().override("Windows.UIColor.Accent", 
Color.RED)` will change the _effective value_ of the mapping. This is also true 
for platform-independent properties: 
`Platform.getPreferences().setAppearance(Appearance.DARK)` will override the 
platform appearance with the specified value.

Overriding a property or mapping with a new value causes the new value to have 
a higher precedence than the platform-provided value. If the override is 
removed by calling `override("Windows.UIColor.Accent", null)`, or 
`setAppearance(null)`, the platform-provided value is restored, i.e. the 
effective value now resolves to the immutable platform-provided value. This 
elaborate system ensures that platform-provided values are immutable and you 
can't "lose" them by overwriting or removing the values.

> 2. Does this API track user changes (via OS widgets such as Control Panel in 
> Windows) at runtime?

Yes, it tracks changes in real-time when they occur. It even tracks changes for 
overridden preferences, and resolves the _current_ platform-provided value once 
the override is removed.

> 3. "Platform Preferences" name.  We already have java.util.prefs.Preferences, 
> the name might be confusing.  Swing has UIDefaults, perhaps we could think of 
> something along those lines?

I prefer "Preferences" for two reasons: it doesn't close the door to adding 
other kinds of platform-provided preferences in the future, which might not 
directly relate to graphics. Additionally, "UIDefaults" or something along 
those lines implies, well, that it's the default. But it's not really that; 
this API represents the _current_ configuration, not the _default_ 
configuration.

> 4. Re: Property API.  I think it is still a valid approach, once a call to 
> list all the available keys is added.  It would be nice to have a way to also 
> obtain the type of the value for a specific key, or infer the type from the 
> type of property returned.  Even more, it might be nice to be able to query 
> the:
> 
> * list of all the keys exposed by the API
> * value type

Do you mean that you want `Preferences.appearanceProperty()`, 
`Preferences.backgroundColorProperty()` and 
`Preferences.foregroundColorProperty()` to be returned as a list? For example, 
by adding another method like:

interface Preferences {
...
List> getProperties() {...}
...
}

I don't really see the added value.

> * possibly, a human-readable description of the property

Why? If it is human-readable, do you envision this to be shown in the user 
interface of an application? If so, we'd also need it to be localizable. 
However, I don't see why the API should provide this information. Seems like it 
could be included in the documentation, though.

> * possibly, whether the value can be changed by this application

All preferences can be overridden by an application, even computed properties 
like `Preferences.appearanceProperty()`.

> I am still not sold on the concept of Appearance (what if a Dusk mode will be 
> added in addition to the Dark and Light?). However, it could be just another 
> platform-specific key, could it not?

At this point, I think that light and dark mode are deeply baked into the 
operating systems (at least when it comes to macOS and Windows). Please note 
that this aligns with a follow-up enhancement that would add a 
`Stage.preferences` property to control the platform appearance of the window 
decorations (see [Stage 
appearance](https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548#stage-appearance)).

Supporting different OS appearances in client applications is quite hard, so I 
don't expect operating systems to add more than those two appearances in the 
forseeable future. But if that happens, we'll have two options:
1. If _Dusk Mode_  is widely supported by different operating systems, we can 
add it to the platform-independent `Appearance` enumeration. However, this 
would be a breaking change and not something we would do lightly.
2. If it is just a narrow feature of a single operating system, we can add a 
platform-specific key to the `Preferences` map, and style themes that want to 
support this particular appearance will then include code to handle just this 
platform-specific key.

> [discussion] the problem with ObservableMap is that it drags in a large API 
> surface of Map.

I outlined several alternatives to using `ObservableMap` 
[here](https://github.com/openjdk/jfx/pull/1014#issuecomment-1500535347), and 
concluded that `ObservableMap` is my preferred solution. The main reason is 
that, by the time you add in all of the operations that we 

Re: RFR: 8301302: Platform preferences API [v3]

[Andy Goryachev]

On Thu, 2 Feb 2023 19:54:33 GMT, Michael Strauß  wrote:

>> Please read [this 
>> document](https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548) 
>> for an introduction to the Platform Preferences API, and how it interacts 
>> with the proposed style theme and stage appearance features.
>
> Michael Strauß has updated the pull request with a new target base due to a 
> merge or a rebase.

catching up with the discussion:

1. ObservableMap.  Is it unmodifiable?  I'd expect it to be, since this API 
exposes the properties of the underlying platform.
2. Does this API track user changes (via OS widgets such as Control Panel in 
Windows) at runtime?
3. "Platform Preferences" name.  We already have java.util.prefs.Preferences, 
the name might be confusing.  Swing has UIDefaults, perhaps we could think of 
something along those lines?
4. Re: Property API.  I think it is still a valid approach, once a call to list 
all the available keys is added.  It would be nice to have a way to also obtain 
the type of the value for a specific key, or infer the type from the type of 
property returned.  Even more, it might be nice to be able to query the:
- list of all the keys exposed by the API
- value type
- possibly, a human-readable description of the property
- possibly, whether the value can be changed by this application

I am still not sold on the concept of Appearance (what if a Dusk mode will be 
added in addition to the Dark and Light?).  However, it could be just another 
platform-specific key, could it not?

What do you think?

Also, this PR needs a CSR once everyone is happy with the outcome of this 
discussion.

[discussion] the problem with ObservableMap is that it drags in a large API 
surface of Map.

[discussion] A native window appearance might be more than one attribute.  In 
that sense, I think it is better to treat them as such - as platform-specific 
attributes.

(An example I have in mind is a gradient color accent in Windows 10 - I don't 
know if that's still a thing, or how it's done in windows 11 and the dark 
mode).  But the idea of limiting the API to a very narrow set of existing 
variants bothers me.

[question] when an application changes the value of one or more properties, 
does it change the appearance of that application alone, all of the javafx 
applications, or all of the platform applications?

[opinion] I'd rather see UIPreferences name (PlatformUISettings?) and move it 
outside of the Platform class.

[discussion]

> `Appearance` is central to _all_ of those features

>From the API perspective, Appearance, though important, is just one of many 
>attributes.  Adding a separate method where we could operate with a Set of 
>attributes ("hints") is, in my opinion, a less optimal solution.

For example, Stage.setAttributes(Set) is a much more flexible approach.  If 
things change tomorrow with new attribute added, or some other change, this API 
does not have to be modified.  (The Stage should silently ignore any attributes 
it does not recognize or that are not supported by the platform).

For the Platform Preferences API, a native attribute such as 
`DWMWA_USE_IMMERSIVE_DARK_MODE`  may also be echoed by an Appearance attribute, 
so the app dev can use either one depending on the requirements. 

I suppose we could have a dedicated getAppearance() method as shortcut (as well 
as some other frequently used shortcuts).

What do you think?

[opinion]

I don't quite like the 'override' method and the idea of app modifying the 
values, as I see it as a purely informational API.  The changes should, in my 
opinion, be triggered by the platform only.  Any other functionality is 
app-specific.

Another thing i am not comfortable with is the eager initialization (and that's 
another reason I don't quite like ObservableMap idea).  Generally speaking, we 
should not be initializing things an application is not going to use.  That's 
why I like the concept of accessing individual attributes - if a theme needs 
Appearance, only that value and its machinery gets initialized.

Small delays added here and there add up.

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1520708740
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1520894055
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1520897723
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1520898504
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1520899926
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1523841849
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1523855676

Re: RFR: 8301302: Platform preferences API [v3]

[airsquared]

On Tue, 2 May 2023 12:31:20 GMT, Michael Strauß  wrote:

> But what do you do when a style theme uses platform preferences to adjust its 
> appearance (light/dark mode), but an application either doesn't want to offer 
> a dark mode, or wants the _application_ (not the OS) be able to toggle 
> light/dark appearance? If an application can't override the preferences that 
> go into a style theme, then it can't control its appearance.

Since this is mainly for if a user wants to adjust the preferences used in a 
style theme, would it make sense to provide this feature for overriding 
preferences in the style theme API? This could also give the user the option to 
override preferences only for a specific style theme instead of globally for 
all style themes. Moving the overriding preferences feature into the style 
theme API could also open the door for style theme authors to allow 
customization of other parts of a style theme.

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1532502159

Re: RFR: 8301302: Platform preferences API [v3]

[Michael Strauß]

On Mon, 24 Apr 2023 22:08:20 GMT, Andy Goryachev  wrote:

> I meant
> 
> ```
> ObjectProperty Preferences.getProperty(String key);
> Set listPropertyKeys();
> ```
> 
> it would also be nice to obtain a typed property right away. That might be 
> more difficult, since one needs to use a dedicated class for the key what 
> contains the value type, which might be clunky.

I discussed this briefly in a previous 
[comment](https://github.com/openjdk/jfx/pull/1014#issuecomment-1500535347). 
The problem with this idea is that you need to do two things: first, check 
whether a given preference is available on the current platform, and second, 
get the property for the key.

What happens if you try to retrieve a property for a key that's not available? 
Do you get an exception? Do you get a property implementation that holds a 
default value? What if you want to control the default value?

The `Optional` approach, as currently proposed, solves this problem with an API 
that Java users already know. For example, if you are targeting macOS and you 
are interested in the `macOS.NSColor.findHighlightColor` preference, which is 
only available starting with macOS 10.13, you could provide a suitable fallback 
like this:

var findHighlightColor = Platform.getPreferences()
.getColor("macOS.NSColor.findHighlightColor")
.orElse(Color.BLUE);

> Map.containsValue() would have to resolve values for all the keys. Is this 
> expected? If the code already resolves all values, I presume on the first 
> call to `getPreferences`, the Map is probably ok.
> 
> Does querying for all the values take long? Any possibility of getting 
> blocked waiting for something?

Yes, the `Map` contract holds, which is why all values will have to be resolved 
by the time you're calling `Map.containsValue()`. From an API perspective, it 
is unspecified how that works, i.e. whether values are lazily or eagerly 
resolved. From an implementation perspective, all values are queried on 
application startup, and after that the values are updated as updates come in.

For example, in the Windows toolkit, this works by intercepting several events 
in the main window loop, including `WM_SETTINGCHANGE`, `WM_THEMECHANGED`, or 
`WM_SYSCOLORCHANGE`. On macOS, the native toolkit subscribes to the 
`AppleColorPreferencesChangedNotification` and 
`AppleInterfaceThemeChangedNotification` events. After receiving one of these 
events, the updated values are sent back to the `PlatformImpl` class, where 
they are integrated into the `Preferences` map.

The event-based approach of the current implementation doesn't run any risk of 
blocking calls.

> > What I've been calling `Appearance` in this PR is _one_ of those knobs.
> 
> I think we are on the same page here.
> 
> My point here is that, from the API design standpoint, it is better to think 
> ahead and consider controlling multiple attributes instead of a singularly 
> invented Appearance enum. Why not have `Stage.setAttributes(Set)` instead of 
> `setAppearance()` then?
> 
> We can still have an Apperance enum, but this time it will be just one of 
> many possible attributes. it could be platform-independent if we agree that 
> it's well defined, or it could be platform-specific like 
> DWMWA_USE_IMMERSIVE_DARK_MODE.

There's a reason why `Appearance` is kind of singled out among all of the 
potential other knobs, and that is because it represents a concept that 
interacts very strongly with the upcoming Style Themes feature.

In total, I propose three PRs:
[JDK-8301302: Platform preferences 
API](https://bugs.openjdk.org/browse/JDK-8301302)
[JDK-8301303: Allow Stage to control the appearance of window 
decorations](https://bugs.openjdk.org/browse/JDK-8301303)
[JDK-8267546: Add CSS themes as a first-class 
concept](https://bugs.openjdk.org/browse/JDK-8267546)

`Appearance` is central to _all_ of those features, as it encodes the "light 
mode or dark mode" knob. This is the most significant knob that users will 
interact with, as it represents:
1. An OS-wide system setting
2. The general "light or dark" appearance of window borders
3. The colors used in application stylesheets, icons, images, etc.

For a style theme author, this bit of information is central, as the entire 
look and feel of the theme will be designed around it. Most likely there will 
be at least two hand-picked sets of colors for a style theme depending on 
whether light or dark mode is enabled.

I don't think we'll want the style theme author to bother figuring out whether 
some obscure platform-specific `DWMWA_USE_IMMERSIVE_DARK_MODE` flag is set, or 
whether the current macOS appearance is named `NSAppearanceNameDarkAqua`. So we 
need a platform-independent way of encoding this bit, and `Appearance` is 
pretty easy to use for style theme authors:

// Note that a real implementation would need to subscribe to Appearance 
changes,
// and update the stylesheets accordingly
public class MyTheme implements StyleTheme {
public MyTheme() {
   

Re: RFR: 8301302: Platform preferences API [v3]

[Michael Strauß]

On Wed, 3 May 2023 06:23:48 GMT, airsquared  wrote:

> Since this is mainly for if a user wants to adjust the preferences used in a 
> style theme, would it make sense to provide this feature for overriding 
> preferences in the style theme API? This could also give the user the option 
> to override preferences only for a specific style theme instead of globally 
> for all style themes. Moving the overriding preferences feature into the 
> style theme API could also open the door for style theme authors to allow 
> customization of other parts of a style theme.

Style themes are not the only place where platform preferences might be used. 
For example, macOS has an OS setting that controls whether clicking on a 
scrollbar moves the content page by page, or directly to the clicked spot. 
Skins could use those kinds of non-visual preferences in the future.

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1533051522

Re: RFR: 8301302: Platform preferences API [v3]

[Andy Goryachev]

On Mon, 24 Apr 2023 20:13:30 GMT, Michael Strauß  wrote:

> Do you mean that you want `Preferences.appearanceProperty()`, 
> `Preferences.backgroundColorProperty()` and 
> `Preferences.foregroundColorProperty()` to be returned as a list? For 
> example, by adding another method like:

I meant 

ObjectProperty Preferences.getProperty(String key);
Set listPropertyKeys();


it would also be nice to obtain a typed property right away.  That might be 
more difficult, since one needs to use a dedicated class for the key what 
contains the value type, which might be clunky.

> `ObservableMap` is my preferred solution.

Map.containsValue() would have to resolve values for all the keys.  Is this 
expected?
If the code already resolves all values, I presume on the first call to 
`getPreferences`, the Map is probably ok.

Does querying for  all the values take long? Any possibility of getting blocked 
waiting for something?

> What I've been calling `Appearance` in this PR is _one_ of those knobs.

I think we are on the same page here.

My point here is that, from the API design standpoint, it is better to think 
ahead and consider controlling multiple attributes instead of a singularly 
invented Appearance enum.  Why not have `Stage.setAttributes(Set)` instead of 
`setAppearance()` then?

We can still have an Apperance enum, but this time it will be just one of many 
possible attributes.  it could be platform-independent if we agree that it's 
well defined, or it could be platform-specific like 
DWMWA_USE_IMMERSIVE_DARK_MODE.

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1520892988
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1520935390
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1520944848

Re: RFR: 8301302: Platform preferences API [v3]

[John Hendrikx]

On Mon, 24 Apr 2023 06:23:41 GMT, Michael Strauß  wrote:

> > I'm not convinced that a delayed change + commit system is the correct way 
> > to do this. Properties should behave the same everywhere in JavaFX and this 
> > seems to change how they work quite a bit.
> > Instead, I propose to look at how layout in JavaFX is handling this 
> > problem. Layout looks at thousands of properties, yet changing one or many 
> > of the involved properties does not involve an expensive layout 
> > recalculation per change. Instead, changes are tracked by marking certain 
> > aspects of the involved controls dirty. On the next pulse, the layout code 
> > notices that something that would influence layout and CSS decisions has 
> > changed, and performs the required changes. The properties involved are all 
> > normal properties, that can be changed quickly, reflect their current value 
> > immediately and that can be overridden by the user or reset back to 
> > defaults. There is no override or commit system needed.
> > Have you considered allowing users to change preference values directly, 
> > but not acting on those changes until the next pulse occurs? Users can 
> > still listen for keys/properties, just like they can for layout properties, 
> > but the major changes that involve recomputing CSS is only done once per 
> > pulse.
> > This would make it possible to change several preference values without 
> > penalty (which happens on the FX thread anyway, so pulses are on hold 
> > during that time), and they're automatically "committed" once the user is 
> > done on the FX thread and the next pulse fires. I think it would be a very 
> > good fit.
> 
> I think this could work, but it also means giving up on instant change 
> notifications. A call to `setAppearance` or `override(key, value)` will not 
> instantly fire a change notification (after the code that modifies the 
> properties is done), but delay it until the next pulse. Resetting the value 
> to its original value before the next pulse would probably also elide the 
> change notification entirely. It's basically the same as change+commit, but 
> with an implicit commit in the next pulse.

That's not quite what I meant. You can add listeners still and get instant 
change notifications.  Just like when I listen to the `backgroundProperty` of a 
control, and someone changes it, I get notified instantly and the change is 
reflected (in the property) instantly.  Only when the next pulse fires is the 
change actually rendered.

I would think the same is possible with say the appearance property.  When I 
change it from LIGHT to DARK, everyone interested gets this notification 
immediately.  On the next pulse, the change is noticed and only then do we 
change the stylesheets or make other adjustments that are high impact.  
Basically, the computationally expensive stuff happens during a pulse; it could 
register invalidation listeners on properties of interest which just set a 
flag, that is checked and reset on the next pulse.

I'm not 100% sure, but it seems you want to add listeners to these properties 
yourself to instantly trigger the Theme updating code -- I'm saying, only set a 
boolean that they've changed, check it on the next pulse using 
`Scene#addPreLayoutPulseListener` (or perhaps there is another mechanism you 
can tap into internally), and then trigger the Theme change.

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1519514059

Re: RFR: 8301302: Platform preferences API [v3]

[Michael Strauß]

On Mon, 24 Apr 2023 07:14:47 GMT, John Hendrikx  wrote:

> That's not quite what I meant. You can add listeners still and get instant 
> change notifications. Just like when I listen to the `backgroundProperty` of 
> a control, and someone changes it, I get notified instantly and the change is 
> reflected (in the property) instantly. Only when the next pulse fires is the 
> change actually rendered.
> 
> I would think the same is possible with say the appearance property. When I 
> change it from LIGHT to DARK, everyone interested gets this notification 
> immediately. On the next pulse, the change is noticed and only then do we 
> change the stylesheets or make other adjustments that are high impact. 
> Basically, the computationally expensive stuff happens during a pulse; it 
> could register invalidation listeners on properties of interest which just 
> set a flag, that is checked and reset on the next pulse.
> 
> I'm not 100% sure, but it seems you want to add listeners to these properties 
> yourself to instantly trigger the Theme updating code -- I'm saying, only set 
> a boolean that they've changed, check it on the next pulse using 
> `Scene#addPreLayoutPulseListener` (or perhaps there is another mechanism you 
> can tap into internally), and then trigger the Theme change.

In the case of themes, listening to a changing preference may _be_ the 
expensive stuff, since it can involve recreating the entire list of stylesheets 
programmatically. Currently, a theme implementation might do something like 
this:


public class MyTheme implements StyleTheme {
private final InvalidationListener preferencesChanged = observable -> {
// change getStylesheet() list
};

public MyTheme() {
Platform.getPreferences().addListener(new 
WeakInvalidationListener(preferencesChanged));
}

...
}


However, I'm thinking that the Preferences API might not be the right place to 
solve this problem. Maybe that should be built into the style theme API 
instead, for example with another interface:


public interface PlatformStyleTheme extends StyleTheme {
void onPreferencesChanged(Iterable> changes);
}


The Preferences _implementation_ would then aggregate the changes that have 
accumulated since the last pulse, and inform the current style theme by 
invoking its `onPreferencesChanged` method. This means that a style theme 
doesn't need to register listeners for different kinds of preferences, and the 
Preferences API can behave just as any normal property would behave.

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1520681831

Re: RFR: 8301302: Platform preferences API [v3]

[Michael Strauß]

On Sun, 9 Apr 2023 20:23:55 GMT, John Hendrikx  wrote:

> With the preferences Map, this could work similar perhaps; set a key to 
> whatever you want with put, and restore it to its original value by setting 
> it to null.

That's how the current API works, with a little bit of added complexity:


interface Preferences {
...

/**
 * Overrides the value of the {@link #appearanceProperty() appearance} 
property.
 * 
 * Specifying {@code null} clears the override, which restores the value of 
the
 * {@code appearance} property to the platform-provided value.
 * 
 * Calling this method does not update the {@code appearance} property 
instantaneously;
 * instead, the property is only updated after calling {@link #commit()}, 
or after the
 * occurrence of an operating system event that causes the {@code 
appearance} property
 * to be recomputed.
 *
 * @param appearance the platform appearance override, or {@code null} to 
clear the override
 */
void setAppearance(Appearance appearance);

...

/**
 * Overrides a key-value mapping.
 * 
 * If a platform-provided mapping for the key already exists, calling this 
method overrides
 * the value that is mapped to the key. If a platform-provided mapping for 
the key doesn't
 * exist, this method creates a new mapping.
 * 
 * Specifying a {@code null} value clears the override, which restores the 
value mapped to
 * the key to the platform-provided value. If the platform does not provide 
a mapping for
 * the specified key, the mapping is effectively removed.
 * 
 * Calling this method does not update the mapping instantaneously; 
instead, the mapping
 * is only updated after calling {@link #commit()}, or after the occurrence 
of an operating
 * system event that causes the mapped value to be recomputed.
 *
 * @param key the key
 * @param value the new value, or {@code null} to clear the override
 * @throws NullPointerException if {@code key} is null
 * @throws IllegalArgumentException if a platform-provided mapping for the 
key exists, and
 *  the specified value is an instance of a 
different class
 *  than the platform-provided value
 * @return the previous value associated with {@code key}
 */
 T override(String key, T value);

/**
 * Commits outstanding overridden preferences, which also causes the values 
of derived
 * properties to be recomputed.
 */
void commit();
}


It is very likely the case that changing preferences can lead to very expensive 
operations in large real-world applications. For example, style themes or the 
entire user interface may be recreated, icons/images may be loaded, etc.

The `Preferences` implementation accounts for this by firing only a single 
invalidation notification, even when multiple platform preferences have 
changed. The documentation of `Preferences` contains guidance for users of this 
API:

 * @implNote In many cases, multiple platform preferences can change at the 
same time.
 *   For example, switching from light to dark mode changes various 
foreground elements to light
 *   colors, and various background elements to dark colors.
 *   
 *   The {@code Preferences} implementation returned from this 
method fires a single invalidation
 *   event for such bulk changes. If a listener performs 
potentially heavy work, such as recreating
 *   and applying CSS themes, it might be beneficial to use {@link 
javafx.beans.InvalidationListener}
 *   instead of {@link javafx.collections.MapChangeListener} to 
prevent the listener from firing
 *   multiple times in bulk change scenarios.


This works when the changes originate from the OS, but it doesn't work when an 
application overrides preference mappings manually. `ObservableMap` doesn't 
support bulk changes, so repeatedly calling `override(...)` would end up firing 
multiple change notifications, and subscribers would have no way of knowing 
when the bulk change transaction has ended.

That's where the concept of _uncommitted modifications_ comes into play: 
calling `override(...)` or any of the property setters like 
`setAppearance(...)` doesn't apply the changes immediately, it delays those 
changes until `commit()` is called or until an OS event causes the preference 
to be recomputed. When that happens, a single invalidation notification is 
fired, the same as it would have if the change originated from the OS.

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1502081527

Re: RFR: 8301302: Platform preferences API [v2]

[Michael Strauß]

On Tue, 31 Jan 2023 23:04:50 GMT, Scott Palmer  wrote:

>> Michael Strauß has refreshed the contents of this pull request, and previous 
>> commits have been removed. Incremental views are not available.
>
> On Tue, Jan 31, 2023 at 3:00 PM Andy Goryachev ***@***.***>
> wrote:
> 
>> In the context of adding theme support in javafx, I think this PR is a
>> step in the right direction. I also like a small set of
>> platform-independent properties like fg and bg colors. I wonder if the same
>> approach can be extended for other aspects of platform L, like fonts,
>> spacing, and may be other aspects (like, hiding scrollbars setting on Mac?)
>>
>> I agree with @kevinrushforth  - we'd
>> need more discussion on the actual implementation. There are a few items
>> that I feel are unnecessary, or might be done differently, and I'd like to
>> learn what other people think.
>>
>> Specifically:
>>
>>1. Appearance enum seems unnecessary - there might be more choices in
>>a specific platform (Mac Ventura has three: dark/light/auto).
>>
>>
> Is it necessary for any application to know about "auto"?  Presumably when
> the mode automatically changes from one to the other there would be an
> event that should be handled, just as if the user changed the setting while
> the application was running.
> 
> Scott
> 
>> Message ID: ***@***.***>
>>

@swpalmer @hjohn @andy-goryachev-oracle @kevinrushforth 
I think that the [Preferences 
API](https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548) has 
evolved quite well. There were quite some discussions on the mailing list, as 
well as here on GitHub. Given that there haven't been new arguments in favor or 
against this feature in a while, I'd like to wrap up the discussion on whether 
we want this in JavaFX.

The main reason why I think this should go into core JavaFX is that it is 
adding a useful feature that can only be reasonably provided by the native 
windowing toolkits, and not by third-party libraries.

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1514130844

Re: RFR: 8301302: Platform preferences API [v3]

[Michael Strauß]

On Mon, 24 Apr 2023 06:01:58 GMT, John Hendrikx  wrote:

> I'm not convinced that a delayed change + commit system is the correct way to 
> do this. Properties should behave the same everywhere in JavaFX and this 
> seems to change how they work quite a bit.
> 
> Instead, I propose to look at how layout in JavaFX is handling this problem. 
> Layout looks at thousands of properties, yet changing one or many of the 
> involved properties does not involve an expensive layout recalculation per 
> change. Instead, changes are tracked by marking certain aspects of the 
> involved controls dirty. On the next pulse, the layout code notices that 
> something that would influence layout and CSS decisions has changed, and 
> performs the required changes. The properties involved are all normal 
> properties, that can be changed quickly, reflect their current value 
> immediately and that can be overridden by the user or reset back to defaults. 
> There is no override or commit system needed.
> 
> Have you considered allowing users to change preference values directly, but 
> not acting on those changes until the next pulse occurs? Users can still 
> listen for keys/properties, just like they can for layout properties, but the 
> major changes that involve recomputing CSS is only done once per pulse.
> 
> This would make it possible to change several preference values without 
> penalty (which happens on the FX thread anyway, so pulses are on hold during 
> that time), and they're automatically "committed" once the user is done on 
> the FX thread and the next pulse fires. I think it would be a very good fit.

I think this could work, but it also means giving up on instant change 
notifications. A call to `setAppearance` or `override(key, value)` will not 
instantly fire a change notification (after the code that modifies the 
properties is done), but delay it until the next pulse. Resetting the value to 
its original value before the next pulse would probably also elide the change 
notification entirely. It's basically the same as change+commit, but with an 
implicit commit in the next pulse.

> I'm still not sold on the `override` method. Would it not be better to make 
> the map writable (with `put`) and if there is need to reset properties back 
> to default offer a `reset` method? I'm saying this because adding an override 
> method makes the map effectively writable (although delayed, if that's not 
> going to be changed), and in that case I'd prefer using existing map methods 
> for this purpose.

Overriding a value is not the same thing as just changing it, since it also 
prevents further automatic updates of the value if the underlying system 
preference is changed. Since the semantics of overrides are quite different, I 
wonder if it’s a good idea to squeeze this into the `put` box, instead of just 
choosing a box with a different name.

There are additional problems: if the map is fully mutable, it stands to reason 
that mappings should also be removable. But what if a mapping was removed, and 
the underlying system preference is changed? Do we add the mapping again 
(probably not)? In the end, we have a map where special rules apply for some of 
its mappings, even mappings that are not even in the map. All that just to 
avoid the `override` name?

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1519456140
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1521710571

Re: RFR: 8301302: Platform preferences API [v3]

[John Hendrikx]

On Sun, 9 Apr 2023 17:33:42 GMT, Michael Strauß  wrote:

> In general, platform preferences correspond to OS-level settings and are 
> updated dynamically. Third-party themes might integrate platform preferences 
> into their look and feel, which is often what users expect to see. But 
> consider a scenario where an application uses a third-party theme that adapts 
> to the OS appearance, but the application author only wants to support a dark 
> appearance (independent from the OS appearance).
> 
> For this scenario, platform preferences should be overridable from 
> application code. I've considered several potential approaches:

Would an approach similar to how Stylesheets do it be useful here?  A property 
of a control can be overridden programmatically as well as set by CSS. In 
either case, the getter returns the current value (overridden or set by CSS).  
When set to a non-null value, the value takes precedence over CSS, and when set 
to `null` the CSS provided value takes over again.

With the preferences Map, this could work similar perhaps; set a key to 
whatever you want with put, and restore it to its original value by setting it 
to null.

> I think I've come around to the idea of dropping the `override` method in 
> favor of `put`. However, I don't like the idea of using `put("key", null)` to 
> reset an overridden mapping to its default value. Having "reset" be its own 
> operation seems to work quite well, though:

I'm a lot happier with this API.  I wasn't sure about using `put("key", null)` 
either -- I was trying to compare it with how properties that can also be 
styled with CSS work, so we could use a consistent mechanism.  But after some 
testing, I got the impression that CSS doesn't always respect programmatically 
set values, and still overwrites such values when pseudo classes are involved.

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1501206250
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1532490625

Re: RFR: 8301302: Platform preferences API [v3]

[Michael Strauß]

On Thu, 2 Feb 2023 19:54:33 GMT, Michael Strauß  wrote:

>> Please read [this 
>> document](https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548) 
>> for an introduction to the Platform Preferences API, and how it interacts 
>> with the proposed style theme and stage appearance features.
>
> Michael Strauß has updated the pull request with a new target base due to a 
> merge or a rebase.

In general, platform preferences correspond to OS-level settings and are 
updated dynamically. Third-party themes might integrate platform preferences 
into their look and feel, which is often what users expect to see. But consider 
a scenario where an application uses a third-party theme that adapts to the OS 
appearance, but the application author only wants to support a dark appearance 
(independent from the OS appearance).

For this scenario, platform preferences should be overridable from application 
code. I've considered several potential approaches:

 1. Provide two sets of preferences:

class Platform {
Preferences getUserPreferences();
Preferences getPlatformPreferences();
}

In this idea, `getUserPreferences` returns a mutable version of the immutable 
`getPlatformPreferences`. However, this requires theme authors to always use 
user preferences instead of platform preferences (or combine both); if they 
fail to do so, application authors are out of luck.

 2. Add a separate, overridable property for each of the convenience API 
properties:

interface Preferences {
...
ReadOnlyObjectProperty appearanceProperty();
ObjectProperty appearanceOverrideProperty();
...
}

The value of the read-only `appearanceProperty()` then corresponds to the value 
of `appearanceOverrideProperty()` if the property is set to a non-null value. 
Otherwise, it corresponds to the platform-provided value.

 3. Add a special setter for each of the convenience API properties:

interface Preferences {
...
ReadOnlyObjectProperty appearanceProperty();
void setAppearance(Appearance appearance);
...
}

The `setAppearance` method can be used to _override_ the value of 
`appearanceProperty()`. When `null` is passed into this method, the platform 
default value is restored (or, put differently, the override is cleared). There 
is some precedence in JavaFX for a "special setter" pattern: for example, while 
the `Stage.widthProperty()` is read-only, `Stage.setWidth` is a special setter 
that attempts to set the stage width to the specified value (and may fail to do 
so).

I prefer the third option (the special setter), as it seems to be the cleanest 
approach that doesn't unnecessarily expand the API.

I think I've come around to the idea of dropping the `override` method in favor 
of `put`. However, I don't like the idea of using `put("key", null)` to reset 
an overridden mapping to its default value. Having "reset" be its own operation 
seems to work quite well, though:


interface Preferences extends ObservableMap {
...

/**
 * Overrides a preference mapping.
 * 
 * If a platform-provided mapping for the key already exists, calling this 
method overrides
 * the value that is mapped to the key. If a platform-provided mapping for 
the key doesn't
 * exist, this method creates a new mapping.
 *
 * @param key the key
 * @param value the new value
 * @throws NullPointerException if {@code key} or {@code value} is null
 * @throws IllegalArgumentException if a platform-provided mapping for the 
key exists, and
 *  the specified value is an instance of a 
different class
 *  than the platform-provided value
 * @return the previous value associated with {@code key}
 */
Object put(String key, Object value);

/**
 * Resets an overridden preference mapping to its platform-provided value.
 * 
 * If the preference is overridden, but the platform does not provide a 
mapping for the
 * specified key, the mapping will be removed. If no mapping exists for the 
specified
 * key, calling this method has no effect.
 *
 * @param key the key
 * @throws NullPointerException if {@code key} is null
 */
void reset(String key);

/**
 * Resets all overridden preference mappings to their platform-provided 
values and removes
 * all mappings for which the platform does not provide a default value.
 */
void reset();
}

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1501177237
PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1531375810

Re: RFR: 8301302: Platform preferences API [v2]

[Michael Strauß]

On Tue, 31 Jan 2023 20:18:05 GMT, Michael Strauß  wrote:

> 2. ObservableMap.  Similarly to Node.getProperties(), I wonder if there might 
> be a better way to observe the changes.  May be a different metaphor 
> (subscription?), like adding a value change listener to a specific key.  We 
> do need a set of keys (perhaps that can be an ObservableSet).  Having said 
> that, ObservableMap is good enough solution, and forgive me for stating the 
> obvious, it should not initialize anything if the platform properties have 
> not been requested by the application code.

I've pulled on that string a little more:

 1. `Optional` API + listeners
This would remove the `ObservableMap` implementation, and add the following 
methods instead:

interface Preferences {
...
void addListener(String key, InvalidationListener listener);
void removeListener(String key, InvalidationListener listener);

void addListener(String key, ChangeListener listener);
void removeListener(String key, ChangeListener listener);

Optional getInteger(String key);
Optional getDouble(String key);
...
}

I don't quite like this idea though, since it would allow developers to either 
add listeners to non-existent keys, or require developers to probe whether a 
key exists before adding a listener for it (maybe by also adding a `boolean 
keyExists(String key)` method. It also doesn't allow developers to enumerate 
the keys that are available on a given platform.
If we added a set of keys (maybe as an `ObservableSet`), the result is 
basically a map. We're better off implementing `ObservableMap` in this case.

 2. `Property` API

interface Preferences {
...
ReadOnlyIntegerProperty getIntegerProperty(String key);
ReadOnlyDoubleProperty getDoubleProperty(String key);
...
}

With this idea, instead of having manual listener management and an API to 
query value mappings, we'd expose read-only properties for keys. We might also 
need a method like `boolean keyExists(String key)`, but we can't enumerate all 
platform preferences with this approach.

 3. `Optional` + `Property` API

interface Preferences {
...
Optional getIntegerProperty(String key);
Optional getDoubleProperty(String key);
...
}

This API combines all aspects into a single method call. We also can't 
enumerate the platform preferences.


I still think that implementing `ObservableMap` is preferable to all of these 
alternatives.

-

PR Comment: https://git.openjdk.org/jfx/pull/1014#issuecomment-1500535347

Re: RFR: 8301302: Platform preferences API [v3]

[John Hendrikx]

On Thu, 2 Feb 2023 19:54:33 GMT, Michael Strauß  wrote:

>> Please read [this 
>> document](https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548) 
>> for an introduction to the Platform Preferences API, and how it interacts 
>> with the proposed style theme and stage appearance features.
>
> Michael Strauß has updated the pull request with a new target base due to a 
> merge or a rebase.

I think this would still be good to add to JavaFX as it will allow better 
integration with the platforms it runs on, without requiring the user to query 
native libraries to good integration.

> > 2. ObservableMap.  Similarly to Node.getProperties(), I wonder if there 
> > might be a better way to observe the changes.  May be a different metaphor 
> > (subscription?), like adding a value change listener to a specific key.  We 
> > do need a set of keys (perhaps that can be an ObservableSet).  Having said 
> > that, ObservableMap is good enough solution, and forgive me for stating the 
> > obvious, it should not initialize anything if the platform properties have 
> > not been requested by the application code.
> 
> I've pulled on that string a little more:
> 
>  1. `Optional` API + listeners
> This would remove the `ObservableMap` implementation, and add the following 
> methods instead:
> 
> ```java
> interface Preferences {
> ...
> void addListener(String key, InvalidationListener listener);
> void removeListener(String key, InvalidationListener listener);
> 
> void addListener(String key, ChangeListener listener);
> void removeListener(String key, ChangeListener listener);
> 
> Optional getInteger(String key);
> Optional getDouble(String key);
> ...
> }
> ```
> 
> I don't quite like this idea though, since it would allow developers to 
> either add listeners to non-existent keys, or require developers to probe 
> whether a key exists before adding a listener for it (maybe by also adding a 
> `boolean keyExists(String key)` method. 

This kind of functionality already exists, see for example 
`Bindings#stringValueAt`; you can bind a key of a map (even if it doesn't 
exist, in which case it will be `null`). It's provided as separate from 
`ObservableMap`, although it could have been integrated as well.

> It also doesn't allow developers to enumerate the keys that are available on 
> a given platform. If we added a set of keys (maybe as an `ObservableSet`), 
> the result is basically a map. We're better off implementing `ObservableMap` 
> in this case.
>  
> This API combines all aspects into a single method call. We also can't 
> enumerate the platform preferences.
> 
> I still think that implementing `ObservableMap` is preferable to all of these 
> alternatives.

I was on the fence about this before as well, but I think `ObservableMap` may 
be the way to go now.  The thing that mainly irked me is all the mutation 
methods that will also be available, but I suppose that's inherent in the 
design of collections in Java.

> > With the preferences Map, this could work similar perhaps; set a key to 
> > whatever you want with put, and restore it to its original value by setting 
> > it to null.
> 
> This works when the changes originate from the OS, but it doesn't work when 
> an application overrides preference mappings manually. `ObservableMap` 
> doesn't support bulk changes, so repeatedly calling `override(...)` would end 
> up firing multiple change notifications, and subscribers would have no way of 
> knowing when the bulk change transaction has ended.
> 
> That's where the concept of _uncommitted modifications_ comes into play: 
> calling `override(...)` or any of the property setters like 
> `setAppearance(...)` doesn't apply the changes immediately, it delays those 
> changes until `commit()` is called or until an OS event causes the preference 
> to be recomputed. When that happens, a single invalidation notification is 
> fired, the same as it would have if the change originated from the OS.

I'm not convinced that a delayed change + commit system is the correct way to 
do this. Properties should behave the same everywhere in JavaFX and this seems 
to change how they work quite a bit.

Instead, I propose to look at how layout in JavaFX is handling this problem. 
Layout looks at thousands of properties, yet changing one or many of the 
involved properties does not involve an expensive layout recalculation per 
change. Instead, changes are tracked by marking certain aspects of the involved 
controls dirty. On the next pulse, the layout code notices that something that 
would influence layout and CSS decisions has changed, and performs the required 
changes. The properties involved are all normal properties, that can be changed 
quickly, reflect their current value immediately and that can be overridden by 
the user or reset back to defaults. There is no override or commit system 
needed.

Have you considered allowing users to change preference values directly, but 

Re: RFR: 8301302: Platform preferences API [v4]

[Michael Strauß]

> Please read [this 
> document](https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548) for 
> an introduction to the Platform Preferences API, and how it interacts with 
> the proposed style theme and stage appearance features.

Michael Strauß has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains ten commits:

 - Added test
 - doc changes
 - Ensure that ColorProperty changes are atomic when observed
 - removed unused code
 - Move Appearance enum to javafx.application
 - doc changes
 - Removed platform-independent preference keys
 - documentation
 - Platform preferences implementation

-

Changes: https://git.openjdk.org/jfx/pull/1014/files
 Webrev: https://webrevs.openjdk.org/?repo=jfx=1014=03
  Stats: 3259 lines in 35 files changed: 3159 ins; 58 del; 42 mod
  Patch: https://git.openjdk.org/jfx/pull/1014.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1014/head:pull/1014

PR: https://git.openjdk.org/jfx/pull/1014

Re: RFR: 8255248: NullPointerException in JFXPanel due to race condition in HostContainer [v3]

[Andy Goryachev]

On Mon, 24 Jul 2023 16:08:07 GMT, Michael Strauß  wrote:

>> this code looks much better - eliminates TOC/TOU concern
>> https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use
>
> This code is still accessing shared state (`dnd`, `pWidth`, `pHeight`, 
> `scaleFactorX`, `scaleFactorY`). Without further analysis of when and where 
> these values are written, it is unclear whether this is the right move. In 
> general, allowing interleaved code execution on multiple threads is very hard 
> to get right. Better to not do it, if at all possible.

No doubt.  `dnd`, even though it's commented as "// Accessed on EDT only" (line 
152) seems to be accessed from FX thread via setFxEnabled line 866.
I tend to agree with @mstr2 that this component deserves a re-think and perhaps 
a re-design.  I do recall seeing a number of issues with swing/fx interop in 
jdk8.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1272538884

Re: RFR: 8255248: NullPointerException in JFXPanel due to race condition in HostContainer [v3]

[Michael Strauß]

On Mon, 24 Jul 2023 14:48:57 GMT, Andy Goryachev  wrote:

>> ok
>
> this code looks much better - eliminates TOC/TOU concern
> https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use

This code is still accessing shared state (`dnd`, `pWidth`, `pHeight`, 
`scaleFactorX`, `scaleFactorY`). Without further analysis of when and where 
these values are written, it is unclear whether this is the right move. In 
general, allowing interleaved code execution on multiple threads is very hard 
to get right. Better to not do it, if at all possible.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1272472480

Re: RFR: 8255248: NullPointerException in JFXPanel due to race condition in HostContainer [v3]

[Andy Goryachev]

On Mon, 24 Jul 2023 06:55:19 GMT, Prasanta Sadhukhan  
wrote:

>> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 
>> 1069:
>> 
>>> 1067: 
>>> 1068: @Override
>>> 1069: public void setEmbeddedScene(EmbeddedSceneInterface 
>>> embeddedScene) {
>> 
>> Same comment as with `setEmbeddedStage`; surrounding all accesses to 
>> `scenePeer` and `stagePeer` like this is not the way to do it, and looks 
>> incorrect.  I think this is more in the right direction:
>> 
>> public void setEmbeddedScene(EmbeddedSceneInterface embeddedScene) {
>> synchronized(LOCK) {
>> if (scenePeer == embeddedScene) {
>> return;
>> }
>> scenePeer = embeddedScene;
>> }
>> 
>> if (embeddedScene == null) {
>> invokeOnClientEDT(() -> {
>> if (dnd != null) {
>> dnd.removeNotify();
>> dnd = null;
>> }
>> });
>> return;
>> }
>> if (pWidth > 0 && pHeight > 0) {
>> embeddedScene.setSize(pWidth, pHeight);
>> }
>> embeddedScene.setPixelScaleFactors((float) scaleFactorX, (float) 
>> scaleFactorY);
>> 
>> invokeOnClientEDT(() -> {
>> dnd = new SwingDnD(JFXPanel.this, scenePeer);
>> dnd.addNotify();
>> 
>> embeddedScene.setDragStartListener(dnd.getDragStartListener());
>> });
>> }
>
> ok

this code looks much better - eliminates TOC/TOU concern
https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1272372993

Withdrawn: 8269907 memory leak - Dirty Nodes / Parent removed

[duke]

On Wed, 21 Jul 2021 11:29:38 GMT, Florian Kirmaier  
wrote:

> After thinking about this issue for some time, I've now got a solution.
> I know put the scene in the state it is, before is was shown, when the 
> dirtyNodes are unset, the whole scene is basically considered dirty. 
> This has the drawback of rerendering, whenever a window is "reshown", but it 
> restores sanity about memory behaviour, which should be considered more 
> important.

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.org/jfx/pull/584

Re: RFR: JDK-8310681: Update WebKit to 616.1 [v2]

[Kevin Rushforth]

On Mon, 24 Jul 2023 12:43:57 GMT, Hima Bindu Meda  wrote:

>> Updated JavaFX Webkit to GTK WebKit 2.40 (616.1).
>> Verified the updated version build, sanity tests and stability. No issues 
>> have been observed.
>
> Hima Bindu Meda has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove check for PLATFORM(JAVA) as the change corresponds to webkit upstream

Marked as reviewed by kcr (Lead).

-

PR Review: https://git.openjdk.org/jfx/pull/1180#pullrequestreview-1543405625

Re: RFR: JDK-8310681: Update WebKit to 616.1

[Hima Bindu Meda]

On Mon, 24 Jul 2023 02:50:30 GMT, Jay Bhaskar  wrote:

> > Oh, I see that now. What led me to ask this question was that there is an 
> > alternative implementation in `#if PLATFORM(JAVA)`, which implies that we 
> > removed the call to `stripLeadingAndTrailingHTMLSpaces` and that it is 
> > still present in the upstream.
> > Given that this code block now matches the upstream, the `if..else..endif` 
> > block should simply be replaced with:
> > ```
> > return document().completeURL(linkAttribute);
> > ```
> > 
> > 
> > 
> >   
> > 
> > 
> >   
> > 
> > 
> > 
> >   
> > removing the `#if PLATFORM(JAVA)` and the entire `#else` block (including 
> > the old call to `stripLeadingAndTrailingHTMLSpaces`). Otherwise it will be 
> > confusing and possibly lead to merge conflicts later.
> 
> I agree with this. Is there a reason this can't be done in the current PR?

Pushed the change.

-

PR Comment: https://git.openjdk.org/jfx/pull/1180#issuecomment-1647811198

Re: RFR: JDK-8310681: Update WebKit to 616.1 [v2]

[Hima Bindu Meda]

> Updated JavaFX Webkit to GTK WebKit 2.40 (616.1).
> Verified the updated version build, sanity tests and stability. No issues 
> have been observed.

Hima Bindu Meda has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove check for PLATFORM(JAVA) as the change corresponds to webkit upstream

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1180/files
  - new: https://git.openjdk.org/jfx/pull/1180/files/5039b04b..3362baa6

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1180=01
 - incr: https://webrevs.openjdk.org/?repo=jfx=1180=00-01

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

PR: https://git.openjdk.org/jfx/pull/1180

Aw: Re: CSS Transitions

[Marius Hanl]

I think this is a good feature to have in JavaFX, especially as it makes it much easier to animate the UI.This looks good to me API wise. Since you implemented this as in modern CSS, this is also very easy for people coming from the Web.-- Marius

 
 Am 17.07.23, 21:24 schrieb "Michael Strauß" :

  Here's an updated summary of the proposed feature:
   https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a
   
   And here's the full implementation:
   https://github.com/openjdk/jfx/pull/870
   
   I'm interested in hearing from you whether you think this is a useful
   feature and whether the proposed API is a good match for JavaFX.
   
 

Re: Platform preferences API

[John Hendrikx]

I think Appearance is exactly correct as it is.  It encodes a 
fundamental binary aspect of visual presentations that all themes, 
styles, displays, posters, prints, etc. must choose between: do I 
present text on a light background (and so the text is dark), or do I 
present text on a dark background (and so text is light)? There are no 
other choices without text becoming unreadable, and there never will be.


So IMHO, the enum is correct, and will never need to extended. Encoding 
more than one dimension into an enum should be avoided in favor of 
another enum at all times, so encoding something like contrast into it 
would not be a sensible move.


--John

On 24/07/2023 01:46, Michael Strauß wrote:

I'd like to wrap up the discussion around the new `Appearance`
enumeration proposed for the Platform Preferences API:

 public enum javafx.application.Appearance {
 LIGHT, DARK
 }

When viewed with the goal of eventually supporting style themes, this
might seem a bit limiting at first. Obvious questions are:
1. Can you have more modes than light or dark? What about blue or purple mode?
2. Can you have variations of these modes? For example: high-contrast
light and high-contrast dark.
3. Should "appearance" include other aspects, for example rounded vs.
non-rounded window corners, or title bar tint.

I think the answer to all of these questions is: no. `Appearance`
represents the binary light/dark mode distinction that most operating
systems have settled on. If, at some point in the future, operating
systems support additional modes, we can always consider adding more
enumeration constants.

This doesn't mean that JavaFX applications and style themes are
limited to only supporting one light and one dark theme. A theme might
have different variations for each appearance, or it might not use the
platform's appearance information at all and use other sources of
information. The question we need to solve is whether we should
anticipate all of these future requirements and bake them into the
Platform Preferences API (answer: no).

As proposed, the `Appearance` information provided by the Platform
Preferences API fits nicely with the upcoming Stage Appearance
feature. This allows users to simply bind the two properties to sync
up stage appearance with platform appearance:

 stage.appearanceProperty().bind(
 Application.getPreferences().appearanceProperty());

Basically, we're taking the light/dark information that we've received
from the OS and forwarding this information to the windowing system to
indicate whether we'd like to have light or dark window decorations
(which are the only two options to choose from).

I think this information is useful for JavaFX applications on its own,
and doesn't need to wait for the Stage Appearance feature.

Re: RFR: 8255248: NullPointerException in JFXPanel due to race condition in HostContainer [v4]

[Prasanta Sadhukhan]

> Due to transient datatype of scenePeer, it can become null which can result 
> in NPE in scenarios where scene is continuously been reset and set, which 
> warrants a null check, as is done in other places for the same variable.

Prasanta Sadhukhan has updated the pull request incrementally with one 
additional commit since the last revision:

  Modifications as per review comments

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1178/files
  - new: https://git.openjdk.org/jfx/pull/1178/files/c5c300db..f078f0a2

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1178=03
 - incr: https://webrevs.openjdk.org/?repo=jfx=1178=02-03

  Stats: 44 lines in 1 file changed: 1 ins; 25 del; 18 mod
  Patch: https://git.openjdk.org/jfx/pull/1178.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1178/head:pull/1178

PR: https://git.openjdk.org/jfx/pull/1178

Re: RFR: 8255248: NullPointerException in JFXPanel due to race condition in HostContainer [v3]

[Prasanta Sadhukhan]

On Fri, 21 Jul 2023 18:17:20 GMT, Kevin Rushforth  wrote:

> > The key word is **if done correctly**. One thing to look for is the 
> > possibility of introducing a deadlock and regression, so we may need to 
> > analyze the callers and perform more extensive testing.
> 
> That is exactly my concern. It would need a lot more very careful analysis 
> before making this synchronous.

I guess if we have to make all public methods in JFXPanel synchronous, then the 
same might be called for SwingNode too, ..

-

PR Comment: https://git.openjdk.org/jfx/pull/1178#issuecomment-1647320399

Re: RFR: 8255248: NullPointerException in JFXPanel due to race condition in HostContainer [v3]

[Prasanta Sadhukhan]

On Fri, 21 Jul 2023 21:30:08 GMT, Andy Goryachev  wrote:

>> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelNPETest.java line 
>> 98:
>> 
>>> 96: });
>>> 97: SwingUtilities.invokeAndWait(JFXPanelNPETest::createUI);
>>> 98: for (int i = 0; i < 300; i++) {
>> 
>> I think this is way too long for a unit test.  300 iterations, 200 ms each = 
>> 60 seconds
>
> good point!
> 
> at the same time, we probably want to cover both extremes (short and long 
> delays).
> what would be a reasonable upper limit for a test?  10 seconds?

It used to be 3000 for JBS reproducer which I reduced to 300 for consistent 
issue reproduction, below which the issue might not be seen so consistently 
even without the fix and I guess the test is supposed to fail without the fix 
and pass with it..

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1271828106

Re: RFR: 8255248: NullPointerException in JFXPanel due to race condition in HostContainer [v3]

[Prasanta Sadhukhan]

On Fri, 21 Jul 2023 21:45:56 GMT, John Hendrikx  wrote:

>> Prasanta Sadhukhan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Check FXEnabled initially
>
> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 1069:
> 
>> 1067: 
>> 1068: @Override
>> 1069: public void setEmbeddedScene(EmbeddedSceneInterface 
>> embeddedScene) {
> 
> Same comment as with `setEmbeddedStage`; surrounding all accesses to 
> `scenePeer` and `stagePeer` like this is not the way to do it, and looks 
> incorrect.  I think this is more in the right direction:
> 
> public void setEmbeddedScene(EmbeddedSceneInterface embeddedScene) {
> synchronized(LOCK) {
> if (scenePeer == embeddedScene) {
> return;
> }
> scenePeer = embeddedScene;
> }
> 
> if (embeddedScene == null) {
> invokeOnClientEDT(() -> {
> if (dnd != null) {
> dnd.removeNotify();
> dnd = null;
> }
> });
> return;
> }
> if (pWidth > 0 && pHeight > 0) {
> embeddedScene.setSize(pWidth, pHeight);
> }
> embeddedScene.setPixelScaleFactors((float) scaleFactorX, (float) 
> scaleFactorY);
> 
> invokeOnClientEDT(() -> {
> dnd = new SwingDnD(JFXPanel.this, scenePeer);
> dnd.addNotify();
> 
> embeddedScene.setDragStartListener(dnd.getDragStartListener());
> });
> }

ok

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1271826532