Re: [External] : Re: consistent naming for tests

[Nir Lisker]

My Eclipse never had this long filename problem, and I reviewed the fluent
bindings PR when it was written so I would have seen it. You can try the
most basic version of Eclipse (
https://download.eclipse.org/eclipse/downloads/) to see if it still happens
if you want to dig into it.

On Tue, Jul 9, 2024 at 9:09 PM Andy Goryachev 
wrote:

> I wonder what other filesystems do?  I just want our code to compile in
> Eclipse on Linux Mint.
>
>
>
> -andy
>
>
>
>
>
>
>
> *From: *John Hendrikx 
> *Date: *Tuesday, July 9, 2024 at 11:04
> *To: *Andy Goryachev , Johan Vos <
> johan@gluonhq.com>, openjfx-dev 
> *Subject: *Re: [External] : Re: consistent naming for tests
>
> Perhaps, and I guess we're lucky the classes don't fully overlap then...
> if encfs just cuts off too long names when reading/writing, then as long as
> the filename is still unique enough that is going to work.  As soon as two
> file names would overlap, they would overwrite each other and there's no
> way that the code would still work then.
>
> I doubt however this is reasonable to fix in Eclipse; the filesystem is
> not behaving correctly -- encfs should error out instead of silently
> truncating too long names.
>
> --John
>
> On 09/07/2024 19:50, Andy Goryachev wrote:
>
> or gradle may not be verifying that the file is actually deleted.
>
>
>
> Eclipse allows for online replacement (? or whatever that feature is
> called when it can recompile and replace classes in a running vm), so
> perhaps it is more diligent when it comes to deleting.
>
>
>
> -andy
>
>
>
> *From: *John Hendrikx  
> *Date: *Tuesday, July 9, 2024 at 10:47
> *To: *Andy Goryachev 
> , Johan Vos 
> , openjfx-dev 
> 
> *Subject: *Re: [External] : Re: consistent naming for tests
>
> Then I can't explain why it doesn't fail on Gradle; it must be generating
> similar named classes then, but perhaps at a different location (not on
> encfs) ?.
>
> --John
>
> On 09/07/2024 19:35, Andy Goryachev wrote:
>
> Anonymous classes are named $1.  Nested classes retain their name.
>
>
>
> From the ticket:
>
>
>
> https://bugs.openjdk.org/browse/JDK-8334497
>
>
>
> Could not delete:
> /home/ag/Projects/jfx-2/jfx/rt/modules/javafx.base/testbin/test/javafx/beans/value/ObservableValueFluentBindingsTest$When_flatMap_Called$WithNotNullReturns_ObservableValue_Which$WhenObservedForInvalidations$AndWhenUnobserved.class.
>
>
>
> -andy
>
>
>
>
>
> *From: *John Hendrikx  
> *Date: *Tuesday, July 9, 2024 at 10:31
> *To: *Andy Goryachev 
> , Johan Vos 
> , openjfx-dev 
> 
> *Subject: *Re: [External] : Re: consistent naming for tests
>
> Perhaps it is something Eclipse does differently.  Normally nested classed
> are numbered ($1, $2), so perhaps ecj is compiling these with differently
> filenames.
>
> --John
>
> On 09/07/2024 17:37, Andy Goryachev wrote:
>
> Have you tried building in Eclipse on the latest Linux Mint?  Or building
> on an EncFS mount?
>
>
>
> I don't know why Mint decided to use EncFS knowing its issues, and I
> suppose I can try fixing my setup (it's a default Mint installation), but I
> was quite surprised myself and thought that it might be just as easy to fix
> the tests... here is how the fix might look:
>
>
>
> https://github.com/andy-goryachev-oracle/jfx/pull/9
> 
>
>
>
> -andy
>
>
>
> *From: *John Hendrikx  
> *Date: *Tuesday, July 9, 2024 at 08:22
> *To: *Andy Goryachev 
> , Johan Vos 
> , openjfx-dev 
> 
> *Subject: *[External] : Re: consistent naming for tests
>
>
>
> On 09/07/2024 16:52, Andy Goryachev wrote:
>
>
>
> Two test files consistently generate an error in Eclipse
>
> - ObservableValueFluentBindingsTest
> - LazyObjectBindingTest
>
>
>
> I admit I have a weird setup (EncFS on Linux Mint running on MacBook Pro),
> and it only manifests itself in Eclipse and not in the gradle build -
> perhaps Eclipse actually verifies the removal of files?
>
>
>
> Anyway, a suggestion - if you use @Nested, please keep the class names
> *short*.
>
> This is not an Eclipse bug as I never encounter such issues.  143
> characters is rather short these days, but I suppose we could limit the
> nesting a bit.  Still, I'd look into a way to alleviate this problem in
> your setup, sooner or later this is going to be a problem.
>
> --John
>
>

Re: consistent naming for tests

[Nir Lisker]

>
> * in some cases, tests are always prefixed with `test` (e.g. `testFoo()`)
> * in some cases, tests have a concise but somehow meaningful name (e.g.
> `testScrollBarStaysVisible`)


Prefixing 'test' was an old convention for testing frameworks. I have been
dropping that prefix in my projects since I'm in a test
class/package/source folder anyway, and it's not like there're methods in a
test class that aren't used for testing. I also use long descriptive names,
like 'newValueNotSetIfOldValueWasInvalid()' or, alternatively,
'doNotSetNewValueIfOldValueWasInvalid()'. John's nesting names are also
good when nesting is appropriate.

* in some cases, tests refer to JBS issues (e.g. testJDK8309935)

* in some cases, the test is explained in comments.


I don't like JBS numbers as names, but I like them as links in a comment. I
prefer the name of the test and methods to be self-explanatory, like in
non-test code, rather than comments. However, sometimes comments are needed
because of tricky or non-trivial situations, which is part of what tests
are for.


On Tue, Jul 9, 2024 at 6:30 PM Kevin Rushforth 
wrote:

> This might be a combination of Eclipse and eCryptfs. I agree that 143
> chars is very short for a max length.
>
> -- Kevin
>
>
> On 7/9/2024 8:22 AM, John Hendrikx wrote:
>
>
> On 09/07/2024 16:52, Andy Goryachev wrote:
>
>
>
> Two test files consistently generate an error in Eclipse
>
> - ObservableValueFluentBindingsTest
> - LazyObjectBindingTest
>
>
>
> I admit I have a weird setup (EncFS on Linux Mint running on MacBook Pro),
> and it only manifests itself in Eclipse and not in the gradle build -
> perhaps Eclipse actually verifies the removal of files?
>
>
>
> Anyway, a suggestion - if you use @Nested, please keep the class names
> *short*.
>
> This is not an Eclipse bug as I never encounter such issues.  143
> characters is rather short these days, but I suppose we could limit the
> nesting a bit.  Still, I'd look into a way to alleviate this problem in
> your setup, sooner or later this is going to be a problem.
>
> --John
>
>
>

Re: [External] : Re: UI elements sizing in Modena.css

[Andy Goryachev]

Yeah, I thought that it might be possible to define properties for sizes 
similarly to colors, and some earlier attempts seemed to indicate that it might 
work.

It does not, unless the sizes are fixed (e.g. -fx-size-1px: 1px; )

The real solution, as you mentioned, would be to add capability to add 
variables, though it is much more complex task.

-andy



From: Pedro Duque Vieira 
Date: Tuesday, July 9, 2024 at 15:04
To: Andy Goryachev 
Cc: openjfx-dev@openjdk.org 
Subject: [External] : Re: UI elements sizing in Modena.css
Andy, if I understand the suggestion correctly, you want to set the Modena 
sizes using variables that can be easily overridden. To that effect variables 
must first be able to hold numeric values as, as it stands today
JavaFX only allows colors to be used for CSS variables.
So, you're also suggesting adding the ability for CSS variables to define inset 
values, is that correct?

I think that's a good idea. I would, however, generalize that to allow CSS 
variables to hold numeric values that could be used anywhere. Or at least, 
starting to pave the ground for that.

I would go even further and start to pave ground to add the CSS variable spec 
into javafx css: 
https://codersblock.com/blog/what-can-you-put-in-a-css-variable/

Why use the css web spec? Because of the following reasons:
1 - Designers already know how to work with the css web spec
2 - there are already many tools available that work with the css web spec
3 - there are already many examples on the web using CSS so developers can just 
copy paste those examples
4 - Developers coming from the web can easily start using javafx css with no 
friction and no need to learn it

Thanks





On Tue, Jul 9, 2024 at 9:04 PM Andy Goryachev 
mailto:andy.goryac...@oracle.com>> wrote:
Since we touched the modena.css, I would like to ask the group's opinion on 
whether we should fix the way modena.css sizes UI elements.  Please see 
https://bugs.openjdk.org/browse/JDK-8314683 for reference, where changing the 
font size also unexpectedly changed the scrollbar.

What do you think about introducing a set of variables similar to -fx-base but 
for sizing/padding, placing them early on to depend on the font size in the 
.root selector instead of the font in the actual control?  Something along the 
lines of


.root {

-fx-size-3px: 0.25em;

...

}



.scroll-bar:horizontal > .increment-button > .increment-arrow {

-fx-padding: -fx-size-3px -fx-size-3px -fx-size-3px -fx-size-3px;

}



instead of



.scroll-bar:horizontal > .increment-button > .increment-arrow {

-fx-padding: 0.333em 0.167em 0.333em 0.167em; /* 4 2 4 2 */

}



This way we still permit the UI components resize with the main font, while 
keeping the sizes of all the control surfaces consistent?

This will require a trivial change in InsetsConverter.

What do you think?

-andy


--
Pedro Duque Vieira (Duke) - 
https://www.pixelduke.com

Re: UI elements sizing in Modena.css

[Pedro Duque Vieira]

.. one more reason to use the CSS variable spec that I forgot to mention:
 5 - It's a spec that has been thoroughly thought of before it was added to
CSS and it has stood the test of time.

On Tue, Jul 9, 2024 at 11:04 PM Pedro Duque Vieira <
pedro.duquevie...@gmail.com> wrote:

> Andy, if I understand the suggestion correctly, you want to set the Modena
> sizes using variables that can be easily overridden. To that effect
> variables must first be able to hold numeric values as, as it stands today
> JavaFX only allows colors to be used for CSS variables.
> So, you're also suggesting adding the ability for CSS variables to define
> inset values, is that correct?
>
> I think that's a good idea. I would, however, generalize that to allow CSS
> variables to hold numeric values that could be used anywhere. Or at least,
> starting to pave the ground for that.
>
> I would go even further and start to pave ground to add the CSS
> variable spec into javafx css:
> https://codersblock.com/blog/what-can-you-put-in-a-css-variable/
>
> Why use the css web spec? Because of the following reasons:
> 1 - Designers already know how to work with the css web spec
> 2 - there are already many tools available that work with the css web spec
> 3 - there are already many examples on the web using CSS so developers can
> just copy paste those examples
> 4 - Developers coming from the web can easily start using javafx css with
> no friction and no need to learn it
>
> Thanks
>
>
>
>
>
> On Tue, Jul 9, 2024 at 9:04 PM Andy Goryachev 
> wrote:
>
>> Since we touched the modena.css, I would like to ask the group's opinion
>> on whether we should fix the way modena.css sizes UI elements.  Please see
>> https://bugs.openjdk.org/browse/JDK-8314683 for reference, where
>> changing the font size also unexpectedly changed the scrollbar.
>>
>>
>>
>> What do you think about introducing a set of variables similar to
>> -fx-base but for sizing/padding, placing them early on to depend on the
>> font size in the .root selector instead of the font in the actual control?
>> Something along the lines of
>>
>>
>>
>> .root {
>>
>> -fx-size-3px: 0.25em;
>>
>> ...
>>
>> }
>>
>>
>>
>> .scroll-bar:horizontal > .increment-button > .increment-arrow {
>>
>> -fx-padding: -fx-size-3px -fx-size-3px -fx-size-3px -fx-size-3px;
>>
>> }
>>
>>
>>
>> instead of
>>
>>
>>
>> .scroll-bar:horizontal > .increment-button > .increment-arrow {
>>
>> -fx-padding: 0.333em 0.167em 0.333em 0.167em; /* 4 2 4 2 */
>>
>> }
>>
>>
>>
>> This way we still permit the UI components resize with the main font,
>> while keeping the sizes of all the control surfaces consistent?
>>
>>
>>
>> This will require a trivial change in InsetsConverter.
>>
>>
>>
>> What do you think?
>>
>>
>>
>> -andy
>>
>
>
> --
> Pedro Duque Vieira (Duke) - https://www.pixelduke.com
>


-- 
Pedro Duque Vieira (Duke) - https://www.pixelduke.com

Re: UI elements sizing in Modena.css

[Pedro Duque Vieira]

Andy, if I understand the suggestion correctly, you want to set the Modena
sizes using variables that can be easily overridden. To that effect
variables must first be able to hold numeric values as, as it stands today
JavaFX only allows colors to be used for CSS variables.
So, you're also suggesting adding the ability for CSS variables to define
inset values, is that correct?

I think that's a good idea. I would, however, generalize that to allow CSS
variables to hold numeric values that could be used anywhere. Or at least,
starting to pave the ground for that.

I would go even further and start to pave ground to add the CSS
variable spec into javafx css:
https://codersblock.com/blog/what-can-you-put-in-a-css-variable/

Why use the css web spec? Because of the following reasons:
1 - Designers already know how to work with the css web spec
2 - there are already many tools available that work with the css web spec
3 - there are already many examples on the web using CSS so developers can
just copy paste those examples
4 - Developers coming from the web can easily start using javafx css with
no friction and no need to learn it

Thanks





On Tue, Jul 9, 2024 at 9:04 PM Andy Goryachev 
wrote:

> Since we touched the modena.css, I would like to ask the group's opinion
> on whether we should fix the way modena.css sizes UI elements.  Please see
> https://bugs.openjdk.org/browse/JDK-8314683 for reference, where changing
> the font size also unexpectedly changed the scrollbar.
>
>
>
> What do you think about introducing a set of variables similar to -fx-base
> but for sizing/padding, placing them early on to depend on the font size in
> the .root selector instead of the font in the actual control?  Something
> along the lines of
>
>
>
> .root {
>
> -fx-size-3px: 0.25em;
>
> ...
>
> }
>
>
>
> .scroll-bar:horizontal > .increment-button > .increment-arrow {
>
> -fx-padding: -fx-size-3px -fx-size-3px -fx-size-3px -fx-size-3px;
>
> }
>
>
>
> instead of
>
>
>
> .scroll-bar:horizontal > .increment-button > .increment-arrow {
>
> -fx-padding: 0.333em 0.167em 0.333em 0.167em; /* 4 2 4 2 */
>
> }
>
>
>
> This way we still permit the UI components resize with the main font,
> while keeping the sizes of all the control surfaces consistent?
>
>
>
> This will require a trivial change in InsetsConverter.
>
>
>
> What do you think?
>
>
>
> -andy
>


-- 
Pedro Duque Vieira (Duke) - https://www.pixelduke.com

Re: [External] : Re: CSS Lookups and their origins (possible regression)

[Pedro Duque Vieira]

Yes, you're right.

Need to find time to file those issues. Though I want to finish the theme
conversion to user agent stylesheets and then be sure it is a javafx bug.

On Tue, Jul 9, 2024 at 8:34 PM Andy Goryachev 
wrote:

> Thank you for clarification!
>
>
>
> Glad you were able to do what you wanted.  But really, if there are bugs,
> I would rather have them filed and fixed.
>
>
>
> -andy
>
>
>
>
>
>
>
> *From: *Pedro Duque Vieira 
> *Date: *Tuesday, July 9, 2024 at 12:26
> *To: *Andy Goryachev 
> *Cc: *openjfx-dev@openjdk.org 
> *Subject: *Re: [External] : Re: CSS Lookups and their origins (possible
> regression)
>
> Perhaps I should have been clearer. I mentioned that example merely to
> make a point on how having a stylesheet set as an AUTHOR stylesheet can be
> a problem (Modena or a custom theme library like JMetro)  :-) .
> It wasn't actually to create a new separate discussion.
>
>
>
> As for the other discussion not exactly related to the one in this thread
> (having custom themes be user agent stylesheets) I think I have found a way
> to make multiple stylesheets be a user agent stylesheet.
>
> That was my main problem as JMetro is composed of more than 1 stylesheet.
> I'm also doing it while still just using the javafx standard API. Thus far
> it's working except for some minor bugs (which I'm inclined to think are
> bugs in JavaFX itself).
>
>
>
> Thanks!
>
>
>
> On Tue, Jul 9, 2024 at 8:17 PM Andy Goryachev 
> wrote:
>
> If your stylesheet defines the necessary variables, the "users" should be
> able to redefine them, correct?
>
>
>
> Or maybe allow for programmatic control of the stylesheet, similar to
>
>
> https://github.com/andy-goryachev/AppFramework/blob/7f74f58ecd4de239be923c4384e10142e48ade7c/src/goryachev/fx/FxFramework.java#L31
> 
>
>
> https://github.com/andy-goryachev/AppFramework/blob/main/src/demo/appfw/Styles.java
> 
>
>
>
> Alternatively, we would need a new public API to allow you to do what you
> want how you want.  Perhaps if you could tell us about the problem you are
> trying to solve, exactly, and the APIs that are missing.
>
>
>
> -andy
>
>
>
>
>
>
>
>
>
> *From: *Pedro Duque Vieira 
> *Date: *Tuesday, July 9, 2024 at 12:00
> *To: *Andy Goryachev 
> *Cc: *openjfx-dev@openjdk.org 
> *Subject: *Re: [External] : Re: CSS Lookups and their origins (possible
> regression)
>
> >>  That's why now in the new theme I'm creating I'm setting everything
> to be an user agent stylesheet.
>
>
>
> > and this is probably the right approach.
>
>
>
> Correct. That's why I agree with John and why the current behavior is
> likely undesired. :-)
>
>
>
> On Tue, Jul 9, 2024 at 7:40 PM Andy Goryachev 
> wrote:
>
> >  That's why now in the new theme I'm creating I'm setting everything to
> be an user agent stylesheet.
>
>
>
> and this is probably the right approach.
>
>
>
> -andy
>
>
>
>
>
>
>
> *From: *openjfx-dev  on behalf of Pedro
> Duque Vieira 
> *Date: *Tuesday, July 9, 2024 at 11:28
> *To: *openjfx-dev@openjdk.org 
> *Subject: *Re: [External] : Re: CSS Lookups and their origins (possible
> regression)
>
> Hi guys,
>
>
>
> I agree with John Hendrikx on this.
>
>
>
> The thing is not that you override the "css variable" value but that you
> end up overriding the priority of the rules in Modena which the developer
> won't likely want to.
>
>
>
> One other thing I'd add is that developers also like to use css
> themselves. If modena rules suddenly start to have the priority of AUTHOR
> this becomes much harder. They have to make their rules always more
> specific than Modena's that now have increased priority besides the fact
> that they need to be aware that this is actually happening and is the
> problem (in my experience many developers won't know this).
>
>
>
> On a related note, I created a theme called JMetro. When implementing it I
> made it so that it was composed of author stylesheets (there wasn't a way
> to set it as a user agent stylesheet back when I started). That's also how
> 90% of themes work.
>
> However this is an issue as developers wanting to override styles set by
> JMetro will have a hard time figuring out how to make their rules
> specificity in their CSS higher than JMetro's so they get overridden (I've
> had complaints on this). That's why now in the new theme I'm creating I'm
> setting everything to be an user agent stylesheet.
>
>
>
> Thanks,
>
>
>
> --
>
> Pedro Duque Vieira (Duke) - https://www.pixelduke.com
> 

Re: [External] : Re: CSS Lookups and their origins (possible regression)

[Kevin Rushforth]

My reading of this is that the fix for the caching bug, JDK-8245919 [1] 
via PR #1072 [2], has inadvertently exposed previously hidden behavior 
that is at best an undesirable feature and at worst a bug (I'd call it a 
bug). It seems quite unexpected to me that overriding a variable defined 
in a user agent stylesheet in an  AUTHOR stylesheet would elevate all 
properties derived from that variable to AUTHOR stylesheet status.


Assuming I'm not missing something, we ought to consider fixing this.

-- Kevin

[1] https://bugs.openjdk.org/browse/JDK-8245919
[2] https://github.com/openjdk/jfx/pull/1072


On 7/9/2024 11:21 AM, John Hendrikx wrote:


Hi Andy,

I'm confused, nowhere do I propose to remove or otherwise make the CSS 
reference system implemented by FX unusable.


I'm first trying to ascertain if this would be expected behavior (it 
is indeed unspecified, and the default currently seems to have been 
chosen for implementation ease, not for user friendliness).


**IF** we're considering this worth changing, the change would simply 
be that when you override a variable (like -fx-base) that this is done 
WITHOUT elevating all styles that use it to the level of an AUTHOR 
stylesheet (ie. they remain at USER_AGENT level as they're specified 
by Modena).  This is not a bad view, because in a sense, we're not 
really specifying a style, we're only overriding a variable.  The 
actual style is still specified in Modena, which is a USER_AGENT level 
stylesheet.


As for the bug fix, please read up a bit more on what was fixed, and 
what this is now exposing.  The fix is almost completely unrelated (it 
fixed accidental changes to unrelated controls at the same level (ie. 
siblings) due to cache sharing where one has had a programmatic 
change, and the other didn't). This was caused by a CSS calculation 
bug (calculation was skipped for all styleable properties that already 
had a setter change, if they were encountered first by the CSS 
system).  Now that this isn't the case anymore, set values are 
overwritten with CSS styles more aggressively.  Normally those however 
are only styles that originate from an AUTHOR stylesheet, so this can 
be seen as expected by the user (after all, they WROTE that 
stylesheet).  But because all styles that use a variable are being 
promoted to AUTHOR level, this also includes all unseen styles in 
Modena if you specify the variable in your AUTHOR stylesheet.


> Nowhere did we **actualy** override -fx-text-fill " is not 
technically correct since this color depends on -fx-base.


That really depends on your view point.  Is overriding a variable the 
same as defining all styles (in your AUTHOR stylesheet) that use that 
variable?  If it was a pre-processor, that created a fully resolved 
Modena.css, then this would not be the case.  But it is not 
implemented as such.


> And I would not want to change how it works currently because this 
is the only way (short of overwriting the whole modena.css 
styleshseet) for an application to effect a system-wide change like 
reacting to changes in the user preferences or the platform theme.


To be clear, I'm not proposing to change that at all.

--John

On 09/07/2024 20:00, Andy Goryachev wrote:


1) a buggy implementation coupled with lack of specification creates 
a certain expectation


2) bug gets fixed

3) people complain because the feature now works as it should?

I think (and this is my personal opinion, in the absence of a formal 
specification) that this works as expected now.  The statement " 
Nowhere did we **actualy** override -fx-text-fill" is not technically 
correct since this color depends on -fx-base.


And I would not want to change how it works currently because this is 
the only way (short of overwriting the whole modena.css styleshseet) 
for an application to effect a system-wide change like reacting to 
changes in the user preferences or the platform theme.


-andy

*From: *John Hendrikx 
*Date: *Tuesday, July 9, 2024 at 10:45
*To: *Andy Goryachev , openjfx-dev 

*Subject: *Re: [External] : Re: CSS Lookups and their origins 
(possible regression)


Well, it is coming as a surprise to many. With the fix for the CSS 
caching bug since JavaFX 21, this "normal" behavior is becoming much 
more obvious.


Let me repeat one more time:

If I have a Label, and I manually set its text fill with a setter to 
YELLOW. In JavaFX 17, when I now add a stylesheet that is empty aside 
from `-fx-base: WHITE`, the label's text fill stays YELLOW.


Now do this in JavaFX 21.  As soon as you add the stylesheet with 
`-fx-base: WHITE` in it, the set value to YELLOW is overridden, even 
though technically this value for -fx-text-fill is defined by Modena 
(which should not be overriding set values).  Nowhere did we 
**actualy** override -fx-text-fill, yet the CSS subsystem now sees 
**all** values defined by Modena that are somehow linked to -fx-base 
as defined directly by the developer...


The reason this didn't happen in JavaFX pri

UI elements sizing in Modena.css

[Andy Goryachev]

Since we touched the modena.css, I would like to ask the group's opinion on 
whether we should fix the way modena.css sizes UI elements.  Please see 
https://bugs.openjdk.org/browse/JDK-8314683 for reference, where changing the 
font size also unexpectedly changed the scrollbar.

What do you think about introducing a set of variables similar to -fx-base but 
for sizing/padding, placing them early on to depend on the font size in the 
.root selector instead of the font in the actual control?  Something along the 
lines of


.root {

-fx-size-3px: 0.25em;

...

}



.scroll-bar:horizontal > .increment-button > .increment-arrow {

-fx-padding: -fx-size-3px -fx-size-3px -fx-size-3px -fx-size-3px;

}



instead of



.scroll-bar:horizontal > .increment-button > .increment-arrow {

-fx-padding: 0.333em 0.167em 0.333em 0.167em; /* 4 2 4 2 */

}



This way we still permit the UI components resize with the main font, while 
keeping the sizes of all the control surfaces consistent?

This will require a trivial change in InsetsConverter.

What do you think?

-andy

Re: [External] : Re: CSS Lookups and their origins (possible regression)

[Andy Goryachev]

Thank you for clarification!

Glad you were able to do what you wanted.  But really, if there are bugs, I 
would rather have them filed and fixed.

-andy



From: Pedro Duque Vieira 
Date: Tuesday, July 9, 2024 at 12:26
To: Andy Goryachev 
Cc: openjfx-dev@openjdk.org 
Subject: Re: [External] : Re: CSS Lookups and their origins (possible 
regression)
Perhaps I should have been clearer. I mentioned that example merely to make a 
point on how having a stylesheet set as an AUTHOR stylesheet can be a problem 
(Modena or a custom theme library like JMetro)  :-) .
It wasn't actually to create a new separate discussion.

As for the other discussion not exactly related to the one in this thread 
(having custom themes be user agent stylesheets) I think I have found a way to 
make multiple stylesheets be a user agent stylesheet.
That was my main problem as JMetro is composed of more than 1 stylesheet. I'm 
also doing it while still just using the javafx standard API. Thus far it's 
working except for some minor bugs (which I'm inclined to think are bugs in 
JavaFX itself).

Thanks!

On Tue, Jul 9, 2024 at 8:17 PM Andy Goryachev 
mailto:andy.goryac...@oracle.com>> wrote:
If your stylesheet defines the necessary variables, the "users" should be able 
to redefine them, correct?

Or maybe allow for programmatic control of the stylesheet, similar to
https://github.com/andy-goryachev/AppFramework/blob/7f74f58ecd4de239be923c4384e10142e48ade7c/src/goryachev/fx/FxFramework.java#L31
https://github.com/andy-goryachev/AppFramework/blob/main/src/demo/appfw/Styles.java

Alternatively, we would need a new public API to allow you to do what you want 
how you want.  Perhaps if you could tell us about the problem you are trying to 
solve, exactly, and the APIs that are missing.

-andy




From: Pedro Duque Vieira 
mailto:pedro.duquevie...@gmail.com>>
Date: Tuesday, July 9, 2024 at 12:00
To: Andy Goryachev mailto:andy.goryac...@oracle.com>>
Cc: openjfx-dev@openjdk.org 
mailto:openjfx-dev@openjdk.org>>
Subject: Re: [External] : Re: CSS Lookups and their origins (possible 
regression)
>>  That's why now in the new theme I'm creating I'm setting everything to be 
>> an user agent stylesheet.

> and this is probably the right approach.

Correct. That's why I agree with John and why the current behavior is likely 
undesired. :-)

On Tue, Jul 9, 2024 at 7:40 PM Andy Goryachev 
mailto:andy.goryac...@oracle.com>> wrote:
>  That's why now in the new theme I'm creating I'm setting everything to be an 
> user agent stylesheet.

and this is probably the right approach.

-andy



From: openjfx-dev 
mailto:openjfx-dev-r...@openjdk.org>> on behalf 
of Pedro Duque Vieira 
mailto:pedro.duquevie...@gmail.com>>
Date: Tuesday, July 9, 2024 at 11:28
To: openjfx-dev@openjdk.org 
mailto:openjfx-dev@openjdk.org>>
Subject: Re: [External] : Re: CSS Lookups and their origins (possible 
regression)
Hi guys,

I agree with John Hendrikx on this.

The thing is not that you override the "css variable" value but that you end up 
overriding the priority of the rules in Modena which the developer won't likely 
want to.

One other thing I'd add is that developers also like to use css themselves. If 
modena rules suddenly start to have the priority of AUTHOR this becomes much 
harder. They have to make their rules always more specific than Modena's that 
now have increased priority besides the fact that they need to be aware that 
this is actually happening and is the problem (in my experience many developers 
won't know this).

On a related note, I created a theme called JMetro. When implementing it I made 
it so that it was composed of author stylesheets (there wasn't a way to set it 
as a user agent stylesheet back when I started). That's also how 90% of themes 
work.
However this is an issue as developers wanting to override styles set by JMetro 
will have a hard time figuring out how to make their rules specificity in their 
CSS higher than JMetro's so they get overridden (I've had complaints on this). 
That's why now in the new theme I'm creating I'm setting everything to be an 
user agent stylesheet.

Thanks,

--
Pedro Duque Vieira (Duke) - 
https://www.pixelduke.com


--
Pedro Duque Vieira (Duke) - 
https://www.pixelduke.com

Re: [External] : Re: CSS Lookups and their origins (possible regression)

[Pedro Duque Vieira]

Perhaps I should have been clearer. I mentioned that example merely to make
a point on how having a stylesheet set as an AUTHOR stylesheet can be a
problem (Modena or a custom theme library like JMetro)  :-) .
It wasn't actually to create a new separate discussion.

As for the other discussion not exactly related to the one in this thread
(having custom themes be user agent stylesheets) I think I have found a way
to make multiple stylesheets be a user agent stylesheet.
That was my main problem as JMetro is composed of more than 1 stylesheet.
I'm also doing it while still just using the javafx standard API. Thus far
it's working except for some minor bugs (which I'm inclined to think are
bugs in JavaFX itself).

Thanks!

On Tue, Jul 9, 2024 at 8:17 PM Andy Goryachev 
wrote:

> If your stylesheet defines the necessary variables, the "users" should be
> able to redefine them, correct?
>
>
>
> Or maybe allow for programmatic control of the stylesheet, similar to
>
>
> https://github.com/andy-goryachev/AppFramework/blob/7f74f58ecd4de239be923c4384e10142e48ade7c/src/goryachev/fx/FxFramework.java#L31
>
>
> https://github.com/andy-goryachev/AppFramework/blob/main/src/demo/appfw/Styles.java
>
>
>
> Alternatively, we would need a new public API to allow you to do what you
> want how you want.  Perhaps if you could tell us about the problem you are
> trying to solve, exactly, and the APIs that are missing.
>
>
>
> -andy
>
>
>
>
>
>
>
>
>
> *From: *Pedro Duque Vieira 
> *Date: *Tuesday, July 9, 2024 at 12:00
> *To: *Andy Goryachev 
> *Cc: *openjfx-dev@openjdk.org 
> *Subject: *Re: [External] : Re: CSS Lookups and their origins (possible
> regression)
>
> >>  That's why now in the new theme I'm creating I'm setting everything
> to be an user agent stylesheet.
>
>
>
> > and this is probably the right approach.
>
>
>
> Correct. That's why I agree with John and why the current behavior is
> likely undesired. :-)
>
>
>
> On Tue, Jul 9, 2024 at 7:40 PM Andy Goryachev 
> wrote:
>
> >  That's why now in the new theme I'm creating I'm setting everything to
> be an user agent stylesheet.
>
>
>
> and this is probably the right approach.
>
>
>
> -andy
>
>
>
>
>
>
>
> *From: *openjfx-dev  on behalf of Pedro
> Duque Vieira 
> *Date: *Tuesday, July 9, 2024 at 11:28
> *To: *openjfx-dev@openjdk.org 
> *Subject: *Re: [External] : Re: CSS Lookups and their origins (possible
> regression)
>
> Hi guys,
>
>
>
> I agree with John Hendrikx on this.
>
>
>
> The thing is not that you override the "css variable" value but that you
> end up overriding the priority of the rules in Modena which the developer
> won't likely want to.
>
>
>
> One other thing I'd add is that developers also like to use css
> themselves. If modena rules suddenly start to have the priority of AUTHOR
> this becomes much harder. They have to make their rules always more
> specific than Modena's that now have increased priority besides the fact
> that they need to be aware that this is actually happening and is the
> problem (in my experience many developers won't know this).
>
>
>
> On a related note, I created a theme called JMetro. When implementing it I
> made it so that it was composed of author stylesheets (there wasn't a way
> to set it as a user agent stylesheet back when I started). That's also how
> 90% of themes work.
>
> However this is an issue as developers wanting to override styles set by
> JMetro will have a hard time figuring out how to make their rules
> specificity in their CSS higher than JMetro's so they get overridden (I've
> had complaints on this). That's why now in the new theme I'm creating I'm
> setting everything to be an user agent stylesheet.
>
>
>
> Thanks,
>
>
>
> --
>
> Pedro Duque Vieira (Duke) - https://www.pixelduke.com
> 
>
>
>
>
> --
>
> Pedro Duque Vieira (Duke) - https://www.pixelduke.com
> 
>


-- 
Pedro Duque Vieira (Duke) - https://www.pixelduke.com

Re: [External] : Re: CSS Lookups and their origins (possible regression)

[Andy Goryachev]

If your stylesheet defines the necessary variables, the "users" should be able 
to redefine them, correct?

Or maybe allow for programmatic control of the stylesheet, similar to
https://github.com/andy-goryachev/AppFramework/blob/7f74f58ecd4de239be923c4384e10142e48ade7c/src/goryachev/fx/FxFramework.java#L31
https://github.com/andy-goryachev/AppFramework/blob/main/src/demo/appfw/Styles.java

Alternatively, we would need a new public API to allow you to do what you want 
how you want.  Perhaps if you could tell us about the problem you are trying to 
solve, exactly, and the APIs that are missing.

-andy




From: Pedro Duque Vieira 
Date: Tuesday, July 9, 2024 at 12:00
To: Andy Goryachev 
Cc: openjfx-dev@openjdk.org 
Subject: Re: [External] : Re: CSS Lookups and their origins (possible 
regression)
>>  That's why now in the new theme I'm creating I'm setting everything to be 
>> an user agent stylesheet.

> and this is probably the right approach.

Correct. That's why I agree with John and why the current behavior is likely 
undesired. :-)

On Tue, Jul 9, 2024 at 7:40 PM Andy Goryachev 
mailto:andy.goryac...@oracle.com>> wrote:
>  That's why now in the new theme I'm creating I'm setting everything to be an 
> user agent stylesheet.

and this is probably the right approach.

-andy



From: openjfx-dev 
mailto:openjfx-dev-r...@openjdk.org>> on behalf 
of Pedro Duque Vieira 
mailto:pedro.duquevie...@gmail.com>>
Date: Tuesday, July 9, 2024 at 11:28
To: openjfx-dev@openjdk.org 
mailto:openjfx-dev@openjdk.org>>
Subject: Re: [External] : Re: CSS Lookups and their origins (possible 
regression)
Hi guys,

I agree with John Hendrikx on this.

The thing is not that you override the "css variable" value but that you end up 
overriding the priority of the rules in Modena which the developer won't likely 
want to.

One other thing I'd add is that developers also like to use css themselves. If 
modena rules suddenly start to have the priority of AUTHOR this becomes much 
harder. They have to make their rules always more specific than Modena's that 
now have increased priority besides the fact that they need to be aware that 
this is actually happening and is the problem (in my experience many developers 
won't know this).

On a related note, I created a theme called JMetro. When implementing it I made 
it so that it was composed of author stylesheets (there wasn't a way to set it 
as a user agent stylesheet back when I started). That's also how 90% of themes 
work.
However this is an issue as developers wanting to override styles set by JMetro 
will have a hard time figuring out how to make their rules specificity in their 
CSS higher than JMetro's so they get overridden (I've had complaints on this). 
That's why now in the new theme I'm creating I'm setting everything to be an 
user agent stylesheet.

Thanks,

--
Pedro Duque Vieira (Duke) - 
https://www.pixelduke.com


--
Pedro Duque Vieira (Duke) - 
https://www.pixelduke.com

Re: [External] : Re: CSS Lookups and their origins (possible regression)

[Pedro Duque Vieira]

>>  That's why now in the new theme I'm creating I'm setting everything to
be an user agent stylesheet.



> and this is probably the right approach.


Correct. That's why I agree with John and why the current behavior is
likely undesired. :-)

On Tue, Jul 9, 2024 at 7:40 PM Andy Goryachev 
wrote:

> >  That's why now in the new theme I'm creating I'm setting everything to
> be an user agent stylesheet.
>
>
>
> and this is probably the right approach.
>
>
>
> -andy
>
>
>
>
>
>
>
> *From: *openjfx-dev  on behalf of Pedro
> Duque Vieira 
> *Date: *Tuesday, July 9, 2024 at 11:28
> *To: *openjfx-dev@openjdk.org 
> *Subject: *Re: [External] : Re: CSS Lookups and their origins (possible
> regression)
>
> Hi guys,
>
>
>
> I agree with John Hendrikx on this.
>
>
>
> The thing is not that you override the "css variable" value but that you
> end up overriding the priority of the rules in Modena which the developer
> won't likely want to.
>
>
>
> One other thing I'd add is that developers also like to use css
> themselves. If modena rules suddenly start to have the priority of AUTHOR
> this becomes much harder. They have to make their rules always more
> specific than Modena's that now have increased priority besides the fact
> that they need to be aware that this is actually happening and is the
> problem (in my experience many developers won't know this).
>
>
>
> On a related note, I created a theme called JMetro. When implementing it I
> made it so that it was composed of author stylesheets (there wasn't a way
> to set it as a user agent stylesheet back when I started). That's also how
> 90% of themes work.
>
> However this is an issue as developers wanting to override styles set by
> JMetro will have a hard time figuring out how to make their rules
> specificity in their CSS higher than JMetro's so they get overridden (I've
> had complaints on this). That's why now in the new theme I'm creating I'm
> setting everything to be an user agent stylesheet.
>
>
>
> Thanks,
>
>
>
> --
>
> Pedro Duque Vieira (Duke) - https://www.pixelduke.com
>


-- 
Pedro Duque Vieira (Duke) - https://www.pixelduke.com

Re: [External] : Re: CSS Lookups and their origins (possible regression)

[Andy Goryachev]

>  That's why now in the new theme I'm creating I'm setting everything to be an 
> user agent stylesheet.

and this is probably the right approach.

-andy



From: openjfx-dev  on behalf of Pedro Duque 
Vieira 
Date: Tuesday, July 9, 2024 at 11:28
To: openjfx-dev@openjdk.org 
Subject: Re: [External] : Re: CSS Lookups and their origins (possible 
regression)
Hi guys,

I agree with John Hendrikx on this.

The thing is not that you override the "css variable" value but that you end up 
overriding the priority of the rules in Modena which the developer won't likely 
want to.

One other thing I'd add is that developers also like to use css themselves. If 
modena rules suddenly start to have the priority of AUTHOR this becomes much 
harder. They have to make their rules always more specific than Modena's that 
now have increased priority besides the fact that they need to be aware that 
this is actually happening and is the problem (in my experience many developers 
won't know this).

On a related note, I created a theme called JMetro. When implementing it I made 
it so that it was composed of author stylesheets (there wasn't a way to set it 
as a user agent stylesheet back when I started). That's also how 90% of themes 
work.
However this is an issue as developers wanting to override styles set by JMetro 
will have a hard time figuring out how to make their rules specificity in their 
CSS higher than JMetro's so they get overridden (I've had complaints on this). 
That's why now in the new theme I'm creating I'm setting everything to be an 
user agent stylesheet.

Thanks,

--
Pedro Duque Vieira (Duke) - https://www.pixelduke.com

Re: [External] : Re: CSS Lookups and their origins (possible regression)

[Andy Goryachev]

Yes, sorry, I read it as if you were proposing to change the behavior.  My 
mistake.  I still think this is a good discussion to have.  Perhaps we ought to 
clarify the behavior somewhere?

My understanding of CSS is that it does not work as a pre-processor.  Rather, 
it's a single set of global things where everyone can change everything at run 
time, using the simple set of rules to determine whose changes are on top.  I 
think (guessing here) it works the same in WWW CSS (there is no well-defined 
standard there either, but that is a different story, ).

I wish there was a public API to create stylesheets programmatically, bypassing 
the parser altogether.  But this is unlikely to happen.

-andy


From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 11:21
To: Andy Goryachev , openjfx-dev 

Subject: Re: [External] : Re: CSS Lookups and their origins (possible 
regression)

Hi Andy,

I'm confused, nowhere do I propose to remove or otherwise make the CSS 
reference system implemented by FX unusable.

I'm first trying to ascertain if this would be expected behavior (it is indeed 
unspecified, and the default currently seems to have been chosen for 
implementation ease, not for user friendliness).

**IF** we're considering this worth changing, the change would simply be that 
when you override a variable (like -fx-base) that this is done WITHOUT 
elevating all styles that use it to the level of an AUTHOR stylesheet (ie. they 
remain at USER_AGENT level as they're specified by Modena).  This is not a bad 
view, because in a sense, we're not really specifying a style, we're only 
overriding a variable.  The actual style is still specified in Modena, which is 
a USER_AGENT level stylesheet.

As for the bug fix, please read up a bit more on what was fixed, and what this 
is now exposing.  The fix is almost completely unrelated (it fixed accidental 
changes to unrelated controls at the same level (ie. siblings) due to cache 
sharing where one has had a programmatic change, and the other didn't).  This 
was caused by a CSS calculation bug (calculation was skipped for all styleable 
properties that already had a setter change, if they were encountered first by 
the CSS system).  Now that this isn't the case anymore, set values are 
overwritten with CSS styles more aggressively.  Normally those however are only 
styles that originate from an AUTHOR stylesheet, so this can be seen as 
expected by the user (after all, they WROTE that stylesheet).  But because all 
styles that use a variable are being promoted to AUTHOR level, this also 
includes all unseen styles in Modena if you specify the variable in your AUTHOR 
stylesheet.

> Nowhere did we **actualy** override -fx-text-fill " is not technically 
> correct since this color depends on -fx-base.

That really depends on your view point.  Is overriding a variable the same as 
defining all styles (in your AUTHOR stylesheet) that use that variable?  If it 
was a pre-processor, that created a fully resolved Modena.css, then this would 
not be the case.  But it is not implemented as such.

> And I would not want to change how it works currently because this is the 
> only way (short of overwriting the whole modena.css styleshseet) for an 
> application to effect a system-wide change like reacting to changes in the 
> user preferences or the platform theme.

To be clear, I'm not proposing to change that at all.

--John
On 09/07/2024 20:00, Andy Goryachev wrote:
1) a buggy implementation coupled with lack of specification creates a certain 
expectation
2) bug gets fixed
3) people complain because the feature now works as it should?

I think (and this is my personal opinion, in the absence of a formal 
specification) that this works as expected now.  The statement " Nowhere did we 
**actualy** override -fx-text-fill " is not technically correct since this 
color depends on -fx-base.

And I would not want to change how it works currently because this is the only 
way (short of overwriting the whole modena.css styleshseet) for an application 
to effect a system-wide change like reacting to changes in the user preferences 
or the platform theme.

-andy



From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 10:45
To: Andy Goryachev 
, openjfx-dev 

Subject: Re: [External] : Re: CSS Lookups and their origins (possible 
regression)

Well, it is coming as a surprise to many. With the fix for the CSS caching bug 
since JavaFX 21, this "normal" behavior is becoming much more obvious.

Let me repeat one more time:

If I have a Label, and I manually set its text fill with a setter to YELLOW. In 
JavaFX 17, when I now add a stylesheet that is empty aside from `-fx-base: 
WHITE`, the label's text fill stays YELLOW.

Now do this in JavaFX 21.  As soon as you add the stylesheet with `-fx-base: 
WHITE` in it, the set value to YELLOW is overridden, even though technically 
this value for -fx-tex

Integrated: 8331603: Cleanup native AbstractSurface methods getRGBImpl, setRGBImpl

[Ambarish Rapte]

On Mon, 8 Jul 2024 13:39:49 GMT, Ambarish Rapte  wrote:

> The parameter "offset" is not validated in the 2 native methods getRGBImpl() 
> and setRGBImpl() of com.sun.pisces.AbstractSurface (in JAbstractSurface.c).
> The PR adds the "offset < 0" check to both the methods.

This pull request has now been integrated.

Changeset: e0fdb42b
Author:Ambarish Rapte 
URL:   
https://git.openjdk.org/jfx/commit/e0fdb42b9fc0c09d8d7bf751f5adb9d8f07c0c4c
Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod

8331603: Cleanup native AbstractSurface methods getRGBImpl, setRGBImpl

Reviewed-by: kcr, angorya

-

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

Re: [External] : Re: CSS Lookups and their origins (possible regression)

[Pedro Duque Vieira]

Hi guys,

I agree with John Hendrikx on this.

The thing is not that you override the "css variable" value but that you
end up overriding the priority of the rules in Modena which the developer
won't likely want to.

One other thing I'd add is that developers also like to use css themselves.
If modena rules suddenly start to have the priority of AUTHOR this becomes
much harder. They have to make their rules always more specific than
Modena's that now have increased priority besides the fact that they need
to be aware that this is actually happening and is the problem (in my
experience many developers won't know this).

On a related note, I created a theme called JMetro. When implementing it I
made it so that it was composed of author stylesheets (there wasn't a way
to set it as a user agent stylesheet back when I started). That's also how
90% of themes work.
However this is an issue as developers wanting to override styles set by
JMetro will have a hard time figuring out how to make their rules
specificity in their CSS higher than JMetro's so they get overridden (I've
had complaints on this). That's why now in the new theme I'm creating I'm
setting everything to be an user agent stylesheet.

Thanks,

-- 
Pedro Duque Vieira (Duke) - https://www.pixelduke.com

Re: RFR: 8331603: Cleanup native AbstractSurface methods getRGBImpl, setRGBImpl [v2]

[Ambarish Rapte]

On Tue, 9 Jul 2024 17:10:07 GMT, Andy Goryachev  wrote:

>> Ambarish Rapte has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   const vars
>
> modules/javafx.graphics/src/main/native-prism-sw/JAbstractSurface.c line 70:
> 
>> 68: jint x, jint y, jint width, jint height) {
>> 69: const jint dstX = 0;
>> 70: const jint dstY = 0;
> 
> if it were up to me, I'd remove these as part of this PR.

Thank you Andy, We shall revisit this later under another issue.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1497#discussion_r1670979183

Re: [External] : Re: CSS Lookups and their origins (possible regression)

[John Hendrikx]

Hi Andy,

I'm confused, nowhere do I propose to remove or otherwise make the CSS 
reference system implemented by FX unusable.


I'm first trying to ascertain if this would be expected behavior (it is 
indeed unspecified, and the default currently seems to have been chosen 
for implementation ease, not for user friendliness).


**IF** we're considering this worth changing, the change would simply be 
that when you override a variable (like -fx-base) that this is done 
WITHOUT elevating all styles that use it to the level of an AUTHOR 
stylesheet (ie. they remain at USER_AGENT level as they're specified by 
Modena).  This is not a bad view, because in a sense, we're not really 
specifying a style, we're only overriding a variable.  The actual style 
is still specified in Modena, which is a USER_AGENT level stylesheet.


As for the bug fix, please read up a bit more on what was fixed, and 
what this is now exposing.  The fix is almost completely unrelated (it 
fixed accidental changes to unrelated controls at the same level (ie. 
siblings) due to cache sharing where one has had a programmatic change, 
and the other didn't).  This was caused by a CSS calculation bug 
(calculation was skipped for all styleable properties that already had a 
setter change, if they were encountered first by the CSS system).  Now 
that this isn't the case anymore, set values are overwritten with CSS 
styles more aggressively.  Normally those however are only styles that 
originate from an AUTHOR stylesheet, so this can be seen as expected by 
the user (after all, they WROTE that stylesheet).  But because all 
styles that use a variable are being promoted to AUTHOR level, this also 
includes all unseen styles in Modena if you specify the variable in your 
AUTHOR stylesheet.


> Nowhere did we **actualy** override -fx-text-fill " is not 
technically correct since this color depends on -fx-base.


That really depends on your view point.  Is overriding a variable the 
same as defining all styles (in your AUTHOR stylesheet) that use that 
variable?  If it was a pre-processor, that created a fully resolved 
Modena.css, then this would not be the case.  But it is not implemented 
as such.


> And I would not want to change how it works currently because this is 
the only way (short of overwriting the whole modena.css styleshseet) for 
an application to effect a system-wide change like reacting to changes 
in the user preferences or the platform theme.


To be clear, I'm not proposing to change that at all.

--John

On 09/07/2024 20:00, Andy Goryachev wrote:


1) a buggy implementation coupled with lack of specification creates a 
certain expectation


2) bug gets fixed

3) people complain because the feature now works as it should?

I think (and this is my personal opinion, in the absence of a formal 
specification) that this works as expected now.  The statement " 
Nowhere did we **actualy** override -fx-text-fill" is not technically 
correct since this color depends on -fx-base.


And I would not want to change how it works currently because this is 
the only way (short of overwriting the whole modena.css styleshseet) 
for an application to effect a system-wide change like reacting to 
changes in the user preferences or the platform theme.


-andy

*From: *John Hendrikx 
*Date: *Tuesday, July 9, 2024 at 10:45
*To: *Andy Goryachev , openjfx-dev 

*Subject: *Re: [External] : Re: CSS Lookups and their origins 
(possible regression)


Well, it is coming as a surprise to many. With the fix for the CSS 
caching bug since JavaFX 21, this "normal" behavior is becoming much 
more obvious.


Let me repeat one more time:

If I have a Label, and I manually set its text fill with a setter to 
YELLOW. In JavaFX 17, when I now add a stylesheet that is empty aside 
from `-fx-base: WHITE`, the label's text fill stays YELLOW.


Now do this in JavaFX 21.  As soon as you add the stylesheet with 
`-fx-base: WHITE` in it, the set value to YELLOW is overridden, even 
though technically this value for -fx-text-fill is defined by Modena 
(which should not be overriding set values).  Nowhere did we 
**actualy** override -fx-text-fill, yet the CSS subsystem now sees 
**all** values defined by Modena that are somehow linked to -fx-base 
as defined directly by the developer...


The reason this didn't happen in JavaFX prior to 21 is because there 
was a bug where a CSS value was not fully calculated if the property 
it encountered was overridden via a set value. That was a bug however 
as cache entries are shared amongst similar styled nodes, and so not 
calculating it fully could have effects on other nodes that shared 
that cache entry but did NOT have a property set directly.  Now that 
this bug is fixed, this problem is odd behavior is popping up where 
simply specifying -fx-base in an empty stylesheet is somehow 
overriding a programmatically set text fill.  Users are confused by 
this, as nowhere in their stylesheet do they themselves override text 
fill.

Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v4]

[Kevin Rushforth]

On Tue, 9 Jul 2024 18:06:55 GMT, Andy Goryachev  wrote:

>> I can see if I can externalize this or if that would run into issues.  Also 
>> please note, although technically an API change, it is NOT an accessible API 
>> (and so can be removed at any time) because only the permitted types can 
>> access this API.
>> 
>> In other words, this could wait and be done separately or never.
>
> moving it to an internal class would eliminate the public API.
> 
> I agree with you that the risk is rather low because because the probability 
> of people ever touching these classes is nearly zero.

I am fine with either approach. If it is easy, you can move it now, but there 
are no compatibility concerns with doing it later since a protected method of a 
sealed class without any exported subclasses is not accessible.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1670971671

Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v4]

[Andy Goryachev]

On Tue, 9 Jul 2024 17:58:30 GMT, John Hendrikx  wrote:

>> it looks to me readBinary should be static, and writeBinary an instance 
>> method - this is a normal pattern.  the asymmetry is dictated by the fact 
>> that we don't have an instance when reading, so typically read() methods 
>> read the stream and create an instance at the last moment with the specific 
>> constructor.  unless, of course, the design allows for mutation and the 
>> fields can be set().
>> 
>> Alternatively, readBinary() could be moved to another class (helper? 
>> reader?) to avoid making this an API change.
>> 
>> what do you think?
>
> I can see if I can externalize this or if that would run into issues.  Also 
> please note, although technically an API change, it is NOT an accessible API 
> (and so can be removed at any time) because only the permitted types can 
> access this API.
> 
> In other words, this could wait and be done separately or never.

moving it to an internal class would eliminate the public API.

I agree with you that the risk is rather low because because the probability of 
people ever touching these classes is nearly zero.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1670959871

Re: [External] : Re: consistent naming for tests

[Andy Goryachev]

I wonder what other filesystems do?  I just want our code to compile in Eclipse 
on Linux Mint.

-andy



From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 11:04
To: Andy Goryachev , Johan Vos 
, openjfx-dev 
Subject: Re: [External] : Re: consistent naming for tests

Perhaps, and I guess we're lucky the classes don't fully overlap then... if 
encfs just cuts off too long names when reading/writing, then as long as the 
filename is still unique enough that is going to work.  As soon as two file 
names would overlap, they would overwrite each other and there's no way that 
the code would still work then.

I doubt however this is reasonable to fix in Eclipse; the filesystem is not 
behaving correctly -- encfs should error out instead of silently truncating too 
long names.

--John
On 09/07/2024 19:50, Andy Goryachev wrote:
or gradle may not be verifying that the file is actually deleted.

Eclipse allows for online replacement (? or whatever that feature is called 
when it can recompile and replace classes in a running vm), so perhaps it is 
more diligent when it comes to deleting.

-andy

From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 10:47
To: Andy Goryachev 
, Johan Vos 
, openjfx-dev 

Subject: Re: [External] : Re: consistent naming for tests

Then I can't explain why it doesn't fail on Gradle; it must be generating 
similar named classes then, but perhaps at a different location (not on encfs) 
?.

--John
On 09/07/2024 19:35, Andy Goryachev wrote:
Anonymous classes are named $1.  Nested classes retain their name.

>From the ticket:

https://bugs.openjdk.org/browse/JDK-8334497

Could not delete: 
/home/ag/Projects/jfx-2/jfx/rt/modules/javafx.base/testbin/test/javafx/beans/value/ObservableValueFluentBindingsTest$When_flatMap_Called$WithNotNullReturns_ObservableValue_Which$WhenObservedForInvalidations$AndWhenUnobserved.class.

-andy


From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 10:31
To: Andy Goryachev 
, Johan Vos 
, openjfx-dev 

Subject: Re: [External] : Re: consistent naming for tests

Perhaps it is something Eclipse does differently.  Normally nested classed are 
numbered ($1, $2), so perhaps ecj is compiling these with differently filenames.

--John

On 09/07/2024 17:37, Andy Goryachev wrote:
Have you tried building in Eclipse on the latest Linux Mint?  Or building on an 
EncFS mount?

I don't know why Mint decided to use EncFS knowing its issues, and I suppose I 
can try fixing my setup (it's a default Mint installation), but I was quite 
surprised myself and thought that it might be just as easy to fix the tests... 
here is how the fix might look:

https://github.com/andy-goryachev-oracle/jfx/pull/9

-andy

From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 08:22
To: Andy Goryachev 
, Johan Vos 
, openjfx-dev 

Subject: [External] : Re: consistent naming for tests


On 09/07/2024 16:52, Andy Goryachev wrote:

Two test files consistently generate an error in Eclipse
- ObservableValueFluentBindingsTest
- LazyObjectBindingTest

I admit I have a weird setup (EncFS on Linux Mint running on MacBook Pro), and 
it only manifests itself in Eclipse and not in the gradle build - perhaps 
Eclipse actually verifies the removal of files?

Anyway, a suggestion - if you use @Nested, please keep the class names short.

This is not an Eclipse bug as I never encounter such issues.  143 characters is 
rather short these days, but I suppose we could limit the nesting a bit.  
Still, I'd look into a way to alleviate this problem in your setup, sooner or 
later this is going to be a problem.
--John

Re: [External] : Re: consistent naming for tests

[John Hendrikx]

Perhaps, and I guess we're lucky the classes don't fully overlap then... 
if encfs just cuts off too long names when reading/writing, then as long 
as the filename is still unique enough that is going to work.  As soon 
as two file names would overlap, they would overwrite each other and 
there's no way that the code would still work then.


I doubt however this is reasonable to fix in Eclipse; the filesystem is 
not behaving correctly -- encfs should error out instead of silently 
truncating too long names.


--John

On 09/07/2024 19:50, Andy Goryachev wrote:


or gradle may not be verifying that the file is actually deleted.

Eclipse allows for online replacement (? or whatever that feature is 
called when it can recompile and replace classes in a running vm), so 
perhaps it is more diligent when it comes to deleting.


-andy

*From: *John Hendrikx 
*Date: *Tuesday, July 9, 2024 at 10:47
*To: *Andy Goryachev , Johan Vos 
, openjfx-dev 

*Subject: *Re: [External] : Re: consistent naming for tests

Then I can't explain why it doesn't fail on Gradle; it must be 
generating similar named classes then, but perhaps at a different 
location (not on encfs) ?.


--John

On 09/07/2024 19:35, Andy Goryachev wrote:

Anonymous classes are named $1. Nested classes retain their name.

From the ticket:

https://bugs.openjdk.org/browse/JDK-8334497

Could not delete:

/home/ag/Projects/jfx-2/jfx/rt/modules/javafx.base/testbin/test/javafx/beans/value/ObservableValueFluentBindingsTest$When_flatMap_Called$WithNotNullReturns_ObservableValue_Which$WhenObservedForInvalidations$AndWhenUnobserved.class.

-andy

*From: *John Hendrikx 

*Date: *Tuesday, July 9, 2024 at 10:31
*To: *Andy Goryachev 
, Johan Vos
 ,
openjfx-dev  
*Subject: *Re: [External] : Re: consistent naming for tests

Perhaps it is something Eclipse does differently.  Normally nested
classed are numbered ($1, $2), so perhaps ecj is compiling these
with differently filenames.

--John

On 09/07/2024 17:37, Andy Goryachev wrote:

Have you tried building in Eclipse on the latest Linux Mint? 
Or building on an EncFS mount?

I don't know why Mint decided to use EncFS knowing its issues,
and I suppose I can try fixing my setup (it's a default Mint
installation), but I was quite surprised myself and thought
that it might be just as easy to fix the tests... here is how
the fix might look:

https://github.com/andy-goryachev-oracle/jfx/pull/9



-andy

*From: *John Hendrikx 

*Date: *Tuesday, July 9, 2024 at 08:22
*To: *Andy Goryachev 
, Johan Vos
 ,
openjfx-dev 

*Subject: *[External] : Re: consistent naming for tests

On 09/07/2024 16:52, Andy Goryachev wrote:

Two test files consistently generate an error in Eclipse

- ObservableValueFluentBindingsTest
- LazyObjectBindingTest

I admit I have a weird setup (EncFS on Linux Mint running
on MacBook Pro), and it only manifests itself in Eclipse
and not in the gradle build - perhaps Eclipse actually
verifies the removal of files?

Anyway, a suggestion - if you use @Nested, please keep the
class names /short/.

This is not an Eclipse bug as I never encounter such issues. 
143 characters is rather short these days, but I suppose we
could limit the nesting a bit.  Still, I'd look into a way to
alleviate this problem in your setup, sooner or later this is
going to be a problem.

--John

Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v4]

[John Hendrikx]

On Tue, 9 Jul 2024 17:32:29 GMT, Andy Goryachev  wrote:

>> I missed something in my above evaluation.  The counterpart method 
>> `writeBinary` is not `static`.  Although that makes sense as in that case 
>> you do have an instance you wish to write, it does make the read/write API 
>> asymmetric.
>> 
>> It's not possible to make `readBinary` an instance method though as it is 
>> the method that is creating those instances.
>> 
>> The other way around is possible (make `writeBinary` static), but it would 
>> then again need a pattern match to determine the correct static 
>> `writeBinary` to call when writing an arbitrary `Selector`.  However, this 
>> would have allowed `CompoundSelector` to call a static version of 
>> `SimpleSelector#writeBinary` and `readBinary`, avoiding the extra 
>> identifying byte in the binary format, and avoiding the cast when reading it 
>> back.
>> 
>> The read/write loops below:
>> 
>> +
>> final int nSelectors = is.readShort();
>> final List selectors = new ArrayList<>();
>> for (int n=0; n> selectors.add((SimpleSelector)Selector.readBinary(bssVersion, 
>> is,strings));
>> }
>> +
>> os.writeShort(selectors.size());   // writes the number of 
>> SimpleSelectors to the binary stream
>> for (int n=0; n< selectors.size(); n++) 
>> selectors.get(n).writeBinary(os,stringStore);  // writes each individually
>> 
>> Would then have become:
>> 
>> +
>> final int nSelectors = is.readShort();
>> final List selectors = new ArrayList<>();
>> for (int n=0; n> selectors.add(SimpleSelector.readBinary(bssVersion, 
>> is,strings));  // cast is gone
>> }
>> +
>> os.writeShort(selectors.size());   // writes the number of 
>> SimpleSelectors to the binary stream
>> for (int n=0; n< selectors.size(); n++) 
>> SimpleSelector.writeBinaryStatic(selectors.get(n), os, stringStore);  // 
>> writes each individually
>
> it looks to me readBinary should be static, and writeBinary an instance 
> method - this is a normal pattern.  the asymmetry is dictated by the fact 
> that we don't have an instance when reading, so typically read() methods read 
> the stream and create an instance at the last moment with the specific 
> constructor.  unless, of course, the design allows for mutation and the 
> fields can be set().
> 
> Alternatively, readBinary() could be moved to another class (helper? reader?) 
> to avoid making this an API change.
> 
> what do you think?

I can see if I can externalize this or if that would run into issues.  Also 
please note, although technically an API change, it is NOT an accessible API 
(and so can be removed at any time) because only the permitted types can access 
this API.

In other words, this could wait and be done separately or never.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1670949974

Re: [External] : Re: CSS Lookups and their origins (possible regression)

[Andy Goryachev]

1) a buggy implementation coupled with lack of specification creates a certain 
expectation
2) bug gets fixed
3) people complain because the feature now works as it should?

I think (and this is my personal opinion, in the absence of a formal 
specification) that this works as expected now.  The statement " Nowhere did we 
**actualy** override -fx-text-fill " is not technically correct since this 
color depends on -fx-base.

And I would not want to change how it works currently because this is the only 
way (short of overwriting the whole modena.css styleshseet) for an application 
to effect a system-wide change like reacting to changes in the user preferences 
or the platform theme.

-andy



From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 10:45
To: Andy Goryachev , openjfx-dev 

Subject: Re: [External] : Re: CSS Lookups and their origins (possible 
regression)

Well, it is coming as a surprise to many. With the fix for the CSS caching bug 
since JavaFX 21, this "normal" behavior is becoming much more obvious.

Let me repeat one more time:

If I have a Label, and I manually set its text fill with a setter to YELLOW. In 
JavaFX 17, when I now add a stylesheet that is empty aside from `-fx-base: 
WHITE`, the label's text fill stays YELLOW.

Now do this in JavaFX 21.  As soon as you add the stylesheet with `-fx-base: 
WHITE` in it, the set value to YELLOW is overridden, even though technically 
this value for -fx-text-fill is defined by Modena (which should not be 
overriding set values).  Nowhere did we **actualy** override -fx-text-fill, yet 
the CSS subsystem now sees **all** values defined by Modena that are somehow 
linked to -fx-base as defined directly by the developer...

The reason this didn't happen in JavaFX prior to 21 is because there was a bug 
where a CSS value was not fully calculated if the property it encountered was 
overridden via a set value. That was a bug however as cache entries are shared 
amongst similar styled nodes, and so not calculating it fully could have 
effects on other nodes that shared that cache entry but did NOT have a property 
set directly.  Now that this bug is fixed, this problem is odd behavior is 
popping up where simply specifying -fx-base in an empty stylesheet is somehow 
overriding a programmatically set text fill.  Users are confused by this, as 
nowhere in their stylesheet do they themselves override text fill.

This entire mechanism is not specified by CSS, but is unique to FX.  The most 
similar mechanism in CSS (see Michael's answer) says the priority of a style 
should not be changed when it is using a reference.

--John
On 09/07/2024 17:43, Andy Goryachev wrote:

> all styles used in Modena that rely on -fx-base directly or indirectly 
> suddenly have a higher priority

I think it works as designed (and as expected).

-andy



From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 08:25
To: Andy Goryachev 
, openjfx-dev 

Subject: [External] : Re: CSS Lookups and their origins (possible regression)

It's not that you can't use -fx-base, but that as it is currently that all 
styles used in Modena that rely on -fx-base directly or indirectly suddenly 
have a higher priority (above setters) even though you didn't specifically 
specify them in your own stylesheet.  All such styles are being elevated from 
USER_AGENT to AUTHOR level (which is above USER level which is used for 
setters).

--John
On 09/07/2024 17:03, Andy Goryachev wrote:
I've used this feature in the past to change the colors in all the controls, so 
to me this is the expected behavior.

So in your case (if I got it right), you need to set the direct style on the 
label (.setStyle("-fx-text-fill:yellow")) instead of setting the text fill 
programmatically.  Right?

-andy




From: openjfx-dev 
 on behalf 
of John Hendrikx 
Date: Monday, July 8, 2024 at 17:11
To: openjfx-dev 
Subject: Re: CSS Lookups and their origins (possible regression)

I realized I worded the TLDR poorly.

Let me try again:

TLDR; should styles which use references (like -fx-base used in Modena) become 
AUTHOR level styles if -fx-base is specified in an AUTHOR stylesheet?  The act 
of simply specifying -fx-base in your own AUTHOR stylesheet elevates hundreds 
of styles from Modena to AUTHOR level, as if you specified them directly...

--John
On 09/07/2024 02:07, John Hendrikx wrote:

Hi List,

TLDR; should a CSS reference like -fx-base convert all styles that use this 
value (or derive from it) become AUTHOR level styles (higher priority than 
setters) ?

Long version:

In JavaFX 21, I did a fix (see #1072) to solve a problem where a CSS value 
could be reset on an unrelated control.

This happened when the CSS engine encountered a stylable that is overridden by 
the user (with a setter), and decided NOT to proceed with the f

Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v4]

[John Hendrikx]

> Moves `SimpleSelector` and `CompoundSelector` to internal packages.
> 
> This can be done with only a minor API break, as `SimpleSelector` and 
> `CompoundSelector` were public before.  However, these classes could not be 
> constructed by 3rd parties.  The only way to access them was by doing a cast 
> (generally they're accessed via `Selector` not by their sub types).  The 
> reason they were public at all was because the CSS engine needs to be able to 
> access them from internal packages.
> 
> This change fixes a mistake (or possibly something that couldn't be modelled 
> at the time) when the CSS API was first made public. The intention was always 
> to have a `Selector` interface/abstract class, with private implementations 
> (`SimpleSelector` and `CompoundSelector`).
> 
> This PR as said has a small API break.  The other changes are (AFAICS) source 
> and binary compatible:
> 
> - Made `Selector` `sealed` only permitting `SimpleSelector` and 
> `CompoundSelector` -- as `Selector` had a package private constructor, there 
> are no concerns with pre-existing subclasses
> - `Selector` has a few more methods that are now `protected` -- given that 
> the class is now sealed, these modified methods are not accessible (they may 
> still require rudimentary documentation I suppose)
> - `Selector` now has a `public` default constructor -- as the class is 
> sealed, it is inaccessible
> - `SimpleSelector` and `CompoundSelector` have a few more `public` methods, 
> but they're internal now, so it is irrelevant
> - `createMatch` was implemented directly in `Selector` to avoid having to 
> expose package private fields in `Match` for use by `CompoundSelector`
> - No need anymore for the `SimpleSelectorShim`

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

  Fix review comments

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1333/files
  - new: https://git.openjdk.org/jfx/pull/1333/files/e177ef16..4702972d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx&pr=1333&range=03
 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1333&range=02-03

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

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

Re: [External] : Re: consistent naming for tests

[Andy Goryachev]

or gradle may not be verifying that the file is actually deleted.

Eclipse allows for online replacement (? or whatever that feature is called 
when it can recompile and replace classes in a running vm), so perhaps it is 
more diligent when it comes to deleting.

-andy

From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 10:47
To: Andy Goryachev , Johan Vos 
, openjfx-dev 
Subject: Re: [External] : Re: consistent naming for tests

Then I can't explain why it doesn't fail on Gradle; it must be generating 
similar named classes then, but perhaps at a different location (not on encfs) 
?.

--John
On 09/07/2024 19:35, Andy Goryachev wrote:
Anonymous classes are named $1.  Nested classes retain their name.

>From the ticket:

https://bugs.openjdk.org/browse/JDK-8334497

Could not delete: 
/home/ag/Projects/jfx-2/jfx/rt/modules/javafx.base/testbin/test/javafx/beans/value/ObservableValueFluentBindingsTest$When_flatMap_Called$WithNotNullReturns_ObservableValue_Which$WhenObservedForInvalidations$AndWhenUnobserved.class.

-andy


From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 10:31
To: Andy Goryachev 
, Johan Vos 
, openjfx-dev 

Subject: Re: [External] : Re: consistent naming for tests

Perhaps it is something Eclipse does differently.  Normally nested classed are 
numbered ($1, $2), so perhaps ecj is compiling these with differently filenames.

--John

On 09/07/2024 17:37, Andy Goryachev wrote:
Have you tried building in Eclipse on the latest Linux Mint?  Or building on an 
EncFS mount?

I don't know why Mint decided to use EncFS knowing its issues, and I suppose I 
can try fixing my setup (it's a default Mint installation), but I was quite 
surprised myself and thought that it might be just as easy to fix the tests... 
here is how the fix might look:

https://github.com/andy-goryachev-oracle/jfx/pull/9

-andy

From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 08:22
To: Andy Goryachev 
, Johan Vos 
, openjfx-dev 

Subject: [External] : Re: consistent naming for tests


On 09/07/2024 16:52, Andy Goryachev wrote:

Two test files consistently generate an error in Eclipse
- ObservableValueFluentBindingsTest
- LazyObjectBindingTest

I admit I have a weird setup (EncFS on Linux Mint running on MacBook Pro), and 
it only manifests itself in Eclipse and not in the gradle build - perhaps 
Eclipse actually verifies the removal of files?

Anyway, a suggestion - if you use @Nested, please keep the class names short.

This is not an Eclipse bug as I never encounter such issues.  143 characters is 
rather short these days, but I suppose we could limit the nesting a bit.  
Still, I'd look into a way to alleviate this problem in your setup, sooner or 
later this is going to be a problem.
--John

Re: [External] : Re: consistent naming for tests

[John Hendrikx]

Then I can't explain why it doesn't fail on Gradle; it must be 
generating similar named classes then, but perhaps at a different 
location (not on encfs) ?.


--John

On 09/07/2024 19:35, Andy Goryachev wrote:


Anonymous classes are named $1.  Nested classes retain their name.

From the ticket:

https://bugs.openjdk.org/browse/JDK-8334497

Could not delete: 
/home/ag/Projects/jfx-2/jfx/rt/modules/javafx.base/testbin/test/javafx/beans/value/ObservableValueFluentBindingsTest$When_flatMap_Called$WithNotNullReturns_ObservableValue_Which$WhenObservedForInvalidations$AndWhenUnobserved.class.


-andy

*From: *John Hendrikx 
*Date: *Tuesday, July 9, 2024 at 10:31
*To: *Andy Goryachev , Johan Vos 
, openjfx-dev 

*Subject: *Re: [External] : Re: consistent naming for tests

Perhaps it is something Eclipse does differently. Normally nested 
classed are numbered ($1, $2), so perhaps ecj is compiling these with 
differently filenames.


--John

On 09/07/2024 17:37, Andy Goryachev wrote:

Have you tried building in Eclipse on the latest Linux Mint?  Or
building on an EncFS mount?

I don't know why Mint decided to use EncFS knowing its issues, and
I suppose I can try fixing my setup (it's a default Mint
installation), but I was quite surprised myself and thought that
it might be just as easy to fix the tests... here is how the fix
might look:

https://github.com/andy-goryachev-oracle/jfx/pull/9



-andy

*From: *John Hendrikx 

*Date: *Tuesday, July 9, 2024 at 08:22
*To: *Andy Goryachev 
, Johan Vos
 ,
openjfx-dev  
*Subject: *[External] : Re: consistent naming for tests

On 09/07/2024 16:52, Andy Goryachev wrote:

Two test files consistently generate an error in Eclipse

- ObservableValueFluentBindingsTest
- LazyObjectBindingTest

I admit I have a weird setup (EncFS on Linux Mint running on
MacBook Pro), and it only manifests itself in Eclipse and not
in the gradle build - perhaps Eclipse actually verifies the
removal of files?

Anyway, a suggestion - if you use @Nested, please keep the
class names /short/.

This is not an Eclipse bug as I never encounter such issues.  143
characters is rather short these days, but I suppose we could
limit the nesting a bit.  Still, I'd look into a way to alleviate
this problem in your setup, sooner or later this is going to be a
problem.

--John

Re: [External] : Re: CSS Lookups and their origins (possible regression)

[John Hendrikx]

Well, it is coming as a surprise to many. With the fix for the CSS 
caching bug since JavaFX 21, this "normal" behavior is becoming much 
more obvious.


Let me repeat one more time:

If I have a Label, and I manually set its text fill with a setter to 
YELLOW. In JavaFX 17, when I now add a stylesheet that is empty aside 
from `-fx-base: WHITE`, the label's text fill stays YELLOW.


Now do this in JavaFX 21.  As soon as you add the stylesheet with 
`-fx-base: WHITE` in it, the set value to YELLOW is overridden, even 
though technically this value for -fx-text-fill is defined by Modena 
(which should not be overriding set values).  Nowhere did we **actualy** 
override -fx-text-fill, yet the CSS subsystem now sees **all** values 
defined by Modena that are somehow linked to -fx-base as defined 
directly by the developer...


The reason this didn't happen in JavaFX prior to 21 is because there was 
a bug where a CSS value was not fully calculated if the property it 
encountered was overridden via a set value. That was a bug however as 
cache entries are shared amongst similar styled nodes, and so not 
calculating it fully could have effects on other nodes that shared that 
cache entry but did NOT have a property set directly.  Now that this bug 
is fixed, this problem is odd behavior is popping up where simply 
specifying -fx-base in an empty stylesheet is somehow overriding a 
programmatically set text fill.  Users are confused by this, as nowhere 
in their stylesheet do they themselves override text fill.


This entire mechanism is not specified by CSS, but is unique to FX.  The 
most similar mechanism in CSS (see Michael's answer) says the priority 
of a style should not be changed when it is using a reference.


--John

On 09/07/2024 17:43, Andy Goryachev wrote:


> all styles used in Modena that rely on -fx-base directly or indirectly 
suddenly have a higher priority

I think it works as designed (and as expected).

-andy

*From: *John Hendrikx 
*Date: *Tuesday, July 9, 2024 at 08:25
*To: *Andy Goryachev , openjfx-dev 

*Subject: *[External] : Re: CSS Lookups and their origins (possible 
regression)


It's not that you can't use -fx-base, but that as it is currently that 
all styles used in Modena that rely on -fx-base directly or indirectly 
suddenly have a higher priority (above setters) even though you didn't 
specifically specify them in your own stylesheet.  All such styles are 
being elevated from USER_AGENT to AUTHOR level (which is above USER 
level which is used for setters).


--John

On 09/07/2024 17:03, Andy Goryachev wrote:

I've used this feature in the past to change the colors in all the
controls, so to me this is the expected behavior.

So in your case (if I got it right), you need to set the direct
style on the label (.setStyle("-fx-text-fill:yellow")) instead of
setting the text fill programmatically.  Right?

-andy

*From: *openjfx-dev 
 on behalf of John Hendrikx
 
*Date: *Monday, July 8, 2024 at 17:11
*To: *openjfx-dev 

*Subject: *Re: CSS Lookups and their origins (possible regression)

I realized I worded the TLDR poorly.

Let me try again:

TLDR; should styles which use references (like -fx-base used in
Modena) become AUTHOR level styles if -fx-base is specified in an
AUTHOR stylesheet?  The act of simply specifying -fx-base in your
own AUTHOR stylesheet elevates hundreds of styles from Modena to
AUTHOR level, as if you specified them directly...

--John

On 09/07/2024 02:07, John Hendrikx wrote:

Hi List,

TLDR; should a CSS reference like -fx-base convert all styles
that use this value (or derive from it) become AUTHOR level
styles (higher priority than setters) ?

Long version:

In JavaFX 21, I did a fix (see #1072) to solve a problem where
a CSS value could be reset on an unrelated control.

This happened when the CSS engine encountered a stylable that
is overridden by the user (with a setter), and decided NOT to
proceed with the full CSS value calculation (as it could not
override the user setting if that CSS value had lower
priority).  However, not proceeding with the calculation meant
that a "SKIP" was stored in a shared cache which was
incorrect.  This is because when this "SKIP" is later
encountered for an unrelated control (the cache entries are
shared for controls with the same styles at the same level),
they could get their values reset because they were assumed to
be unstyled.

However, this fix has exposed what seems to be a deeper bug or
perhaps an unfortunate default:

JavaFX has a special feature where you can refer to certain
other styles by using a reference (which is resolved,
recursively, to 

Re: RFR: 8323706: Move SimpleSelector and CompoundSelector to internal packages [v3]

[Andy Goryachev]

On Thu, 18 Jan 2024 23:57:05 GMT, Kevin Rushforth  wrote:

>> John Hendrikx has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains three commits:
>> 
>>  - Merge branch 'master' of https://git.openjdk.org/jfx into 
>> feature/selectors-to-private-api-standalone
>>  - Add since tags
>>  - Move SimpleSelector and CompoundSelector to private classes
>
> modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 96:
> 
>> 94:  * @return a match, never {@code null}
>> 95:  */
>> 96: public final Match createMatch() {
> 
> This is a compatible change only because this class is sealed. I do have a 
> question, though, about whether it should remain abstract.

I agree with Kevin here: the implementation should be moved to respective child 
classes.

If Match constructor is not public, then we should create a factory method in a 
helper, for example.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1670911089

Re: RFR: 8323706: Move SimpleSelector and CompoundSelector to internal packages [v3]

[Andy Goryachev]

On Fri, 19 Jan 2024 09:33:15 GMT, John Hendrikx  wrote:

>> modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 102:
>> 
>>> 100: 
>>> 101: return new Match(this, s.getPseudoClassStates(), idCount, 
>>> styleClassCount);
>>> 102: }
>> 
>> I presume you are moving the implementations to this base class because some 
>> of the types, constructors (e.g., Match), or methods only have package 
>> visibility? Using the accessor pattern via a Helper class is usually how we 
>> deal with this. Have you considered that? It would allow the implementation 
>> to remain in the subclasses.
>
> Yes, correct, `CompoundSelector` accesses the package private fields 
> `idCount` and `styleClassCount` of `Match` directly, which it can't do 
> anymore after being moved to a different package; these lines:
> 
> idCount += match.idCount;
> styleClassCount += match.styleClassCount;
> 
> I'm aware of the Helper classes, but I feel that they are a much more 
> invasive concept (and also harder to follow) to achieve this than doing a 
> pattern match with `instanceof` (which can be replaced with pattern matches 
> for switch once we can use Java 21).  However, if you think this is a 
> requirement, I'm happy to change it -- that said, we're not locked in either 
> choice as far as I can see.
> 
> Alternatively, with everything needed in `Selector` being publicly 
> accessible, I'm not sure if the match creation really needed to be in 
> `Selector` or its subtypes at all.  It feels like more a job for an external 
> type to handle (like how you don't write serialization logic for JSON or XML 
> in each subtype).  If it were up to me, I'd probably create a static method 
> in `Match` which given a `Selector` creates the match.  That way, no `Match` 
> internals need exposing at all.  I could still do this, as the method needed 
> could be package private, and then all the match fields can be made fully 
> private.

what do you think of

Match MatchHelper.create(SimpleSelector)
Match MatchHelper.create(CompoundSelector)

?

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1670912593

Re: RFR: 8323706: Move SimpleSelector and CompoundSelector to internal packages [v3]

[Andy Goryachev]

On Fri, 19 Jan 2024 10:36:57 GMT, John Hendrikx  wrote:

>> I'll add the `@since 23`, however it can't be called by anyone except FX 
>> itself.
>> 
>> Some background: the readBinary/writeBinary methods are ultimately called 
>> via the publicly accessible methods `loadBinary` and `saveBinary` in 
>> `javafx.css.Stylesheet`.  
>> 
>> The reason it needs to be `protected` now is because `CompoundSelector` is 
>> using this (but IMHO, it shouldn't have).  Why I say it shouldn't?  That's 
>> because it already knows what it will be reading will all be 
>> `SimpleSelector`s, as it stores a counter (a `short`) and then loads that 
>> many `SimpleSelector`s.  However, by not taking the direct route of using 
>> `SimpleSelector#readBinary` (and the corresponding 
>> `SimpleSelector#writeBinary`) there is an additional `byte` prefix 
>> indicating the type of selector -- this isn't needed and wastes some space 
>> in the binary format.
>> 
>> Changing that now however would make the binary format incompatible with 
>> earlier versions, so I think making this `protected` is a reasonable 
>> solution.  It also mirrors the `writeBinary` method then, which was already 
>> `protected`.
>
> I missed something in my above evaluation.  The counterpart method 
> `writeBinary` is not `static`.  Although that makes sense as in that case you 
> do have an instance you wish to write, it does make the read/write API 
> asymmetric.
> 
> It's not possible to make `readBinary` an instance method though as it is the 
> method that is creating those instances.
> 
> The other way around is possible (make `writeBinary` static), but it would 
> then again need a pattern match to determine the correct static `writeBinary` 
> to call when writing an arbitrary `Selector`.  However, this would have 
> allowed `CompoundSelector` to call a static version of 
> `SimpleSelector#writeBinary` and `readBinary`, avoiding the extra identifying 
> byte in the binary format, and avoiding the cast when reading it back.
> 
> The read/write loops below:
> 
> +
> final int nSelectors = is.readShort();
> final List selectors = new ArrayList<>();
> for (int n=0; n selectors.add((SimpleSelector)Selector.readBinary(bssVersion, 
> is,strings));
> }
> +
> os.writeShort(selectors.size());   // writes the number of 
> SimpleSelectors to the binary stream
> for (int n=0; n< selectors.size(); n++) 
> selectors.get(n).writeBinary(os,stringStore);  // writes each individually
> 
> Would then have become:
> 
> +
> final int nSelectors = is.readShort();
> final List selectors = new ArrayList<>();
> for (int n=0; n selectors.add(SimpleSelector.readBinary(bssVersion, is,strings)); 
>  // cast is gone
> }
> +
> os.writeShort(selectors.size());   // writes the number of 
> SimpleSelectors to the binary stream
> for (int n=0; n< selectors.size(); n++) 
> SimpleSelector.writeBinaryStatic(selectors.get(n), os, stringStore);  // 
> writes each individually

it looks to me readBinary should be static, and writeBinary an instance method 
- this is a normal pattern.  the asymmetry is dictated by the fact that we 
don't have an instance when reading, so typically read() methods read the 
stream and create an instance at the last moment with the specific constructor. 
 unless, of course, the design allows for mutation and the fields can be set().

Alternatively, readBinary() could be moved to another class (helper? reader?) 
to avoid making this an API change.

what do you think?

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1670922573

Re: [External] : Re: consistent naming for tests

[Andy Goryachev]

Anonymous classes are named $1.  Nested classes retain their name.

>From the ticket:

https://bugs.openjdk.org/browse/JDK-8334497

Could not delete: 
/home/ag/Projects/jfx-2/jfx/rt/modules/javafx.base/testbin/test/javafx/beans/value/ObservableValueFluentBindingsTest$When_flatMap_Called$WithNotNullReturns_ObservableValue_Which$WhenObservedForInvalidations$AndWhenUnobserved.class.

-andy


From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 10:31
To: Andy Goryachev , Johan Vos 
, openjfx-dev 
Subject: Re: [External] : Re: consistent naming for tests

Perhaps it is something Eclipse does differently.  Normally nested classed are 
numbered ($1, $2), so perhaps ecj is compiling these with differently filenames.

--John

On 09/07/2024 17:37, Andy Goryachev wrote:
Have you tried building in Eclipse on the latest Linux Mint?  Or building on an 
EncFS mount?

I don't know why Mint decided to use EncFS knowing its issues, and I suppose I 
can try fixing my setup (it's a default Mint installation), but I was quite 
surprised myself and thought that it might be just as easy to fix the tests... 
here is how the fix might look:

https://github.com/andy-goryachev-oracle/jfx/pull/9

-andy

From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 08:22
To: Andy Goryachev 
, Johan Vos 
, openjfx-dev 

Subject: [External] : Re: consistent naming for tests


On 09/07/2024 16:52, Andy Goryachev wrote:

Two test files consistently generate an error in Eclipse
- ObservableValueFluentBindingsTest
- LazyObjectBindingTest

I admit I have a weird setup (EncFS on Linux Mint running on MacBook Pro), and 
it only manifests itself in Eclipse and not in the gradle build - perhaps 
Eclipse actually verifies the removal of files?

Anyway, a suggestion - if you use @Nested, please keep the class names short.

This is not an Eclipse bug as I never encounter such issues.  143 characters is 
rather short these days, but I suppose we could limit the nesting a bit.  
Still, I'd look into a way to alleviate this problem in your setup, sooner or 
later this is going to be a problem.
--John

Re: [External] : Re: consistent naming for tests

[John Hendrikx]

Perhaps it is something Eclipse does differently.  Normally nested 
classed are numbered ($1, $2), so perhaps ecj is compiling these with 
differently filenames.


--John

On 09/07/2024 17:37, Andy Goryachev wrote:

Have you tried building in Eclipse on the latest Linux Mint?  Or 
building on an EncFS mount?


I don't know why Mint decided to use EncFS knowing its issues, and I 
suppose I can try fixing my setup (it's a default Mint installation), 
but I was quite surprised myself and thought that it might be just as 
easy to fix the tests... here is how the fix might look:


https://github.com/andy-goryachev-oracle/jfx/pull/9

-andy

*From: *John Hendrikx 
*Date: *Tuesday, July 9, 2024 at 08:22
*To: *Andy Goryachev , Johan Vos 
, openjfx-dev 

*Subject: *[External] : Re: consistent naming for tests

On 09/07/2024 16:52, Andy Goryachev wrote:

Two test files consistently generate an error in Eclipse

- ObservableValueFluentBindingsTest
- LazyObjectBindingTest

I admit I have a weird setup (EncFS on Linux Mint running on
MacBook Pro), and it only manifests itself in Eclipse and not in
the gradle build - perhaps Eclipse actually verifies the removal
of files?

Anyway, a suggestion - if you use @Nested, please keep the class
names /short/.

This is not an Eclipse bug as I never encounter such issues.  143 
characters is rather short these days, but I suppose we could limit 
the nesting a bit.  Still, I'd look into a way to alleviate this 
problem in your setup, sooner or later this is going to be a problem.


--John

Re: RFR: 8331603: Cleanup native AbstractSurface methods getRGBImpl, setRGBImpl [v2]

[Andy Goryachev]

On Tue, 9 Jul 2024 07:23:09 GMT, Ambarish Rapte  wrote:

>> The parameter "offset" is not validated in the 2 native methods getRGBImpl() 
>> and setRGBImpl() of com.sun.pisces.AbstractSurface (in JAbstractSurface.c).
>> The PR adds the "offset < 0" check to both the methods.
>
> Ambarish Rapte has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   const vars

lgtm

modules/javafx.graphics/src/main/native-prism-sw/JAbstractSurface.c line 70:

> 68: jint x, jint y, jint width, jint height) {
> 69: const jint dstX = 0;
> 70: const jint dstY = 0;

if it were up to me, I'd remove these as part of this PR.

-

Marked as reviewed by angorya (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1497#pullrequestreview-2166934862
PR Review Comment: https://git.openjdk.org/jfx/pull/1497#discussion_r1670898504

Integrated: 8295945: Revert unintended change to copyright start year

[Ambarish Rapte]

On Mon, 8 Jul 2024 13:31:03 GMT, Ambarish Rapte  wrote:

> A quick fix to correct the created copyright year. 
> The created year in the header was unintentionally modified previously from 
> 2021 to 2017.

This pull request has now been integrated.

Changeset: dbda2cce
Author:Ambarish Rapte 
URL:   
https://git.openjdk.org/jfx/commit/dbda2cce0352692c622e6cc1bc664d779c013fcb
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8295945: Revert unintended change to copyright start year

Reviewed-by: kcr

-

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

Re: [External] : Re: consistent naming for tests

[Andy Goryachev]

I know - very few people would like that solution.  I would welcome other 
suggestions.

The only thing I can say in my defense is that this is only a test, so as long 
as the intent is clear we should be fine.

-andy

From: openjfx-dev  on behalf of Kevin Rushforth 

Date: Tuesday, July 9, 2024 at 09:03
To: openjfx-dev@openjdk.org 
Subject: Re: [External] : Re: consistent naming for tests


https://github.com/andy-goryachev-oracle/jfx/pull/9

Hmm. I don't really like the abbreviations. They would change something that is 
self-explanatory to something completely opaque, although I see you did leave 
the descriptive name as a comment.

-- Kevin

On 7/9/2024 8:37 AM, Andy Goryachev wrote:
Have you tried building in Eclipse on the latest Linux Mint?  Or building on an 
EncFS mount?

I don't know why Mint decided to use EncFS knowing its issues, and I suppose I 
can try fixing my setup (it's a default Mint installation), but I was quite 
surprised myself and thought that it might be just as easy to fix the tests... 
here is how the fix might look:

https://github.com/andy-goryachev-oracle/jfx/pull/9

-andy

From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 08:22
To: Andy Goryachev 
, Johan Vos 
, openjfx-dev 

Subject: [External] : Re: consistent naming for tests


On 09/07/2024 16:52, Andy Goryachev wrote:

Two test files consistently generate an error in Eclipse
- ObservableValueFluentBindingsTest
- LazyObjectBindingTest

I admit I have a weird setup (EncFS on Linux Mint running on MacBook Pro), and 
it only manifests itself in Eclipse and not in the gradle build - perhaps 
Eclipse actually verifies the removal of files?

Anyway, a suggestion - if you use @Nested, please keep the class names short.

This is not an Eclipse bug as I never encounter such issues.  143 characters is 
rather short these days, but I suppose we could limit the nesting a bit.  
Still, I'd look into a way to alleviate this problem in your setup, sooner or 
later this is going to be a problem.
--John

Re: [External] : Re: consistent naming for tests

[Kevin Rushforth]

https://github.com/andy-goryachev-oracle/jfx/pull/9


Hmm. I don't really like the abbreviations. They would change something 
that is self-explanatory to something completely opaque, although I see 
you did leave the descriptive name as a comment.


-- Kevin


On 7/9/2024 8:37 AM, Andy Goryachev wrote:


Have you tried building in Eclipse on the latest Linux Mint?  Or 
building on an EncFS mount?


I don't know why Mint decided to use EncFS knowing its issues, and I 
suppose I can try fixing my setup (it's a default Mint installation), 
but I was quite surprised myself and thought that it might be just as 
easy to fix the tests... here is how the fix might look:


https://github.com/andy-goryachev-oracle/jfx/pull/9

-andy

*From: *John Hendrikx 
*Date: *Tuesday, July 9, 2024 at 08:22
*To: *Andy Goryachev , Johan Vos 
, openjfx-dev 

*Subject: *[External] : Re: consistent naming for tests

On 09/07/2024 16:52, Andy Goryachev wrote:

Two test files consistently generate an error in Eclipse

- ObservableValueFluentBindingsTest
- LazyObjectBindingTest

I admit I have a weird setup (EncFS on Linux Mint running on
MacBook Pro), and it only manifests itself in Eclipse and not in
the gradle build - perhaps Eclipse actually verifies the removal
of files?

Anyway, a suggestion - if you use @Nested, please keep the class
names /short/.

This is not an Eclipse bug as I never encounter such issues.  143 
characters is rather short these days, but I suppose we could limit 
the nesting a bit.  Still, I'd look into a way to alleviate this 
problem in your setup, sooner or later this is going to be a problem.


--John

Re: RFR: 8335218: Eclipse Config: Remove Gradle Integration

[Andy Goryachev]

On Wed, 26 Jun 2024 21:23:34 GMT, Andy Goryachev  wrote:

> This might be controversial. I am proposing to remove the Gradle integration 
> in the Eclipse config files.
> 
> Problem
> ===
> Eclipse Gradle integration (Buildship) cannot import the OpenJFX build.gradle 
> cleanly. Every time the project is imported into a new workspace (or 
> re-opened after being closed) it executes Gradle, creates and modifies a 
> number of Eclipse .project and .classpath files, all of which need to be 
> reverted for Eclipse workspace to become usable again.
> 
> Solution
> ==
> Remove Gradle nature from the Eclipse project files. This change only affects 
> Eclipse config files and does not impact build.gradle or other IDEs.
> 
> Advantages
> =
> 1. The multiple nested projects in the repo will get imported cleanly on the 
> first attempt, will not require additional steps to clear the Buildship 
> changes.
> 2. completely removes the dependency on the Eclipse Buildship and its 
> idiosyncrasies.
> 
> NOTES:
> - even though the reverse was done for IntelliJ, but its gradle import still 
> does not import tests cleanly, see 
> [JDK-8223373](https://bugs.openjdk.org/browse/JDK-8223373)
> - this improvement contradicts 
> [JDK-8223374](https://bugs.openjdk.org/browse/JDK-8223374) as without Eclipse 
> files in the repo, it will be impossible to use Eclipse in a meaningful way 
> without the fully functional Buildship support, and that is a big IF.
> - once integrated, Eclipse users would only need to re-import the main 
> project with 'search for nested projects' enabled

thank you for reviewing!  will integrate after the fork.

-

PR Comment: https://git.openjdk.org/jfx/pull/1491#issuecomment-2218054668

Re: [External] : Re: CSS Lookups and their origins (possible regression)

[Andy Goryachev]

> all styles used in Modena that rely on -fx-base directly or indirectly 
> suddenly have a higher priority

I think it works as designed (and as expected).

-andy



From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 08:25
To: Andy Goryachev , openjfx-dev 

Subject: [External] : Re: CSS Lookups and their origins (possible regression)

It's not that you can't use -fx-base, but that as it is currently that all 
styles used in Modena that rely on -fx-base directly or indirectly suddenly 
have a higher priority (above setters) even though you didn't specifically 
specify them in your own stylesheet.  All such styles are being elevated from 
USER_AGENT to AUTHOR level (which is above USER level which is used for 
setters).

--John
On 09/07/2024 17:03, Andy Goryachev wrote:
I've used this feature in the past to change the colors in all the controls, so 
to me this is the expected behavior.

So in your case (if I got it right), you need to set the direct style on the 
label (.setStyle("-fx-text-fill:yellow")) instead of setting the text fill 
programmatically.  Right?

-andy




From: openjfx-dev 
 on behalf 
of John Hendrikx 
Date: Monday, July 8, 2024 at 17:11
To: openjfx-dev 
Subject: Re: CSS Lookups and their origins (possible regression)

I realized I worded the TLDR poorly.

Let me try again:

TLDR; should styles which use references (like -fx-base used in Modena) become 
AUTHOR level styles if -fx-base is specified in an AUTHOR stylesheet?  The act 
of simply specifying -fx-base in your own AUTHOR stylesheet elevates hundreds 
of styles from Modena to AUTHOR level, as if you specified them directly...

--John
On 09/07/2024 02:07, John Hendrikx wrote:

Hi List,

TLDR; should a CSS reference like -fx-base convert all styles that use this 
value (or derive from it) become AUTHOR level styles (higher priority than 
setters) ?

Long version:

In JavaFX 21, I did a fix (see #1072) to solve a problem where a CSS value 
could be reset on an unrelated control.

This happened when the CSS engine encountered a stylable that is overridden by 
the user (with a setter), and decided NOT to proceed with the full CSS value 
calculation (as it could not override the user setting if that CSS value had 
lower priority).  However, not proceeding with the calculation meant that a 
"SKIP" was stored in a shared cache which was incorrect.  This is because when 
this "SKIP" is later encountered for an unrelated control (the cache entries 
are shared for controls with the same styles at the same level), they could get 
their values reset because they were assumed to be unstyled.

However, this fix has exposed what seems to be a deeper bug or perhaps an 
unfortunate default:

JavaFX has a special feature where you can refer to certain other styles by 
using a reference (which is resolved, recursively, to a final value).  This 
does not seem to be a CSS standard, but is a feature only FX has.

It works by saying something like:

-fx-base: RED;

And then using it like this:

-fx-text-fill: -fx-base;

This feature works accross stylesheets of different origins, so an AUTHOR 
stylesheet can specify -fx-base, and when a USER_AGENT refers to -fx-base, the 
value comes from the AUTHOR stylesheet.

JavaFX then changes the origin of the style to the highest priority encountered 
while resolving the reference.  This means that Modena can specify 
"-fx-text-fill: -fx-base", and when "-fx-base" is then part of the AUTHOR style 
sheet, that ALL Modena styles that use -fx-base will be considered AUTHOR level 
styles, as per this comment:

// The origin of this parsed value is the greatest of

// any of the resolved reference. If a resolved reference

// comes from an inline style, for example, then the value

// calculated from the resolved lookup should have inline

// as its origin. Otherwise, an inline style could be

// stored in shared cache.

I feel that this is a really unfortunate choice.  The style after all was 
specified by Modena, only its value came from another (higher priority) style 
sheet.  I think a more logical choice would have been to not change the 
priority at all, unless a "-fx-text-fill" is explicitly made part of the AUTHOR 
stylesheet.

A consequence of this (and which is much more visible after the fix) is that 
creating a Label with a setTextFill(Color.YELLOW) in its constructor will only 
result in a yellow text fill if the AUTHOR stylesheet did not override any of 
the Modena colors involved in calculating the Modena -fx-text-fill default.  
Overriding -fx-base in any way will result in the text fill of the label to be 
overridden (as the reference came from an AUTHOR stylesheet, which trumps a 
setter which is of a lower style origin).

The comment also alludes to a potential problem.  If an inline style would 
specify "-fx-base", but would be treated as if it came from Modena (USER_AGENT 
level), then thi

Re: RFR: 8323706: Move SimpleSelector and CompoundSelector to internal packages [v3]

[Kevin Rushforth]

On Tue, 9 Jul 2024 12:25:07 GMT, John Hendrikx  wrote:

>> Moves `SimpleSelector` and `CompoundSelector` to internal packages.
>> 
>> This can be done with only a minor API break, as `SimpleSelector` and 
>> `CompoundSelector` were public before.  However, these classes could not be 
>> constructed by 3rd parties.  The only way to access them was by doing a cast 
>> (generally they're accessed via `Selector` not by their sub types).  The 
>> reason they were public at all was because the CSS engine needs to be able 
>> to access them from internal packages.
>> 
>> This change fixes a mistake (or possibly something that couldn't be modelled 
>> at the time) when the CSS API was first made public. The intention was 
>> always to have a `Selector` interface/abstract class, with private 
>> implementations (`SimpleSelector` and `CompoundSelector`).
>> 
>> This PR as said has a small API break.  The other changes are (AFAICS) 
>> source and binary compatible:
>> 
>> - Made `Selector` `sealed` only permitting `SimpleSelector` and 
>> `CompoundSelector` -- as `Selector` had a package private constructor, there 
>> are no concerns with pre-existing subclasses
>> - `Selector` has a few more methods that are now `protected` -- given that 
>> the class is now sealed, these modified methods are not accessible (they may 
>> still require rudimentary documentation I suppose)
>> - `Selector` now has a `public` default constructor -- as the class is 
>> sealed, it is inaccessible
>> - `SimpleSelector` and `CompoundSelector` have a few more `public` methods, 
>> but they're internal now, so it is irrelevant
>> - `createMatch` was implemented directly in `Selector` to avoid having to 
>> expose package private fields in `Match` for use by `CompoundSelector`
>> - No need anymore for the `SimpleSelectorShim`
>
> John Hendrikx has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains three commits:
> 
>  - Merge branch 'master' of https://git.openjdk.org/jfx into 
> feature/selectors-to-private-api-standalone
>  - Add since tags
>  - Move SimpleSelector and CompoundSelector to private classes

I left a couple quick comments on the API changes. I'll review the rest after 
the fork.

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

> 49:  * Explicit constructor for subtype use.
> 50:  *
> 51:  * @since 23

The pattern we use elsewhere is:

"Constructor for subclasses to call."

Also, that should be `@since 24`.

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

> 195:  * @return a selector, never {@code null}
> 196:  * @throws IOException if reading from {@code DataInputStream} fails
> 197:  * @since 23

That should be `@since 24`.

-

PR Review: https://git.openjdk.org/jfx/pull/1333#pullrequestreview-2166691276
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1670750269
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1670751417

Re: [External] : Re: consistent naming for tests

[Andy Goryachev]

Have you tried building in Eclipse on the latest Linux Mint?  Or building on an 
EncFS mount?

I don't know why Mint decided to use EncFS knowing its issues, and I suppose I 
can try fixing my setup (it's a default Mint installation), but I was quite 
surprised myself and thought that it might be just as easy to fix the tests... 
here is how the fix might look:

https://github.com/andy-goryachev-oracle/jfx/pull/9

-andy

From: John Hendrikx 
Date: Tuesday, July 9, 2024 at 08:22
To: Andy Goryachev , Johan Vos 
, openjfx-dev 
Subject: [External] : Re: consistent naming for tests


On 09/07/2024 16:52, Andy Goryachev wrote:

Two test files consistently generate an error in Eclipse
- ObservableValueFluentBindingsTest
- LazyObjectBindingTest

I admit I have a weird setup (EncFS on Linux Mint running on MacBook Pro), and 
it only manifests itself in Eclipse and not in the gradle build - perhaps 
Eclipse actually verifies the removal of files?

Anyway, a suggestion - if you use @Nested, please keep the class names short.

This is not an Eclipse bug as I never encounter such issues.  143 characters is 
rather short these days, but I suppose we could limit the nesting a bit.  
Still, I'd look into a way to alleviate this problem in your setup, sooner or 
later this is going to be a problem.
--John

Re: consistent naming for tests

[Kevin Rushforth]

This might be a combination of Eclipse and eCryptfs. I agree that 143 
chars is very short for a max length.


-- Kevin


On 7/9/2024 8:22 AM, John Hendrikx wrote:



On 09/07/2024 16:52, Andy Goryachev wrote:


Two test files consistently generate an error in Eclipse

- ObservableValueFluentBindingsTest
- LazyObjectBindingTest

I admit I have a weird setup (EncFS on Linux Mint running on MacBook 
Pro), and it only manifests itself in Eclipse and not in the gradle 
build - perhaps Eclipse actually verifies the removal of files?


Anyway, a suggestion - if you use @Nested, please keep the class 
names /short/.


This is not an Eclipse bug as I never encounter such issues. 143 
characters is rather short these days, but I suppose we could limit 
the nesting a bit.  Still, I'd look into a way to alleviate this 
problem in your setup, sooner or later this is going to be a problem.


--John

Re: CSS Lookups and their origins (possible regression)

[John Hendrikx]

It's not that you can't use -fx-base, but that as it is currently that 
all styles used in Modena that rely on -fx-base directly or indirectly 
suddenly have a higher priority (above setters) even though you didn't 
specifically specify them in your own stylesheet.  All such styles are 
being elevated from USER_AGENT to AUTHOR level (which is above USER 
level which is used for setters).


--John

On 09/07/2024 17:03, Andy Goryachev wrote:


I've used this feature in the past to change the colors in all the 
controls, so to me this is the expected behavior.


So in your case (if I got it right), you need to set the direct style 
on the label (.setStyle("-fx-text-fill:yellow")) instead of setting 
the text fill programmatically.  Right?


-andy

*From: *openjfx-dev  on behalf of John 
Hendrikx 

*Date: *Monday, July 8, 2024 at 17:11
*To: *openjfx-dev 
*Subject: *Re: CSS Lookups and their origins (possible regression)

I realized I worded the TLDR poorly.

Let me try again:

TLDR; should styles which use references (like -fx-base used in 
Modena) become AUTHOR level styles if -fx-base is specified in an 
AUTHOR stylesheet?  The act of simply specifying -fx-base in your own 
AUTHOR stylesheet elevates hundreds of styles from Modena to AUTHOR 
level, as if you specified them directly...


--John

On 09/07/2024 02:07, John Hendrikx wrote:

Hi List,

TLDR; should a CSS reference like -fx-base convert all styles that
use this value (or derive from it) become AUTHOR level styles
(higher priority than setters) ?

Long version:

In JavaFX 21, I did a fix (see #1072) to solve a problem where a
CSS value could be reset on an unrelated control.

This happened when the CSS engine encountered a stylable that is
overridden by the user (with a setter), and decided NOT to proceed
with the full CSS value calculation (as it could not override the
user setting if that CSS value had lower priority).  However, not
proceeding with the calculation meant that a "SKIP" was stored in
a shared cache which was incorrect.  This is because when this
"SKIP" is later encountered for an unrelated control (the cache
entries are shared for controls with the same styles at the same
level), they could get their values reset because they were
assumed to be unstyled.

However, this fix has exposed what seems to be a deeper bug or
perhaps an unfortunate default:

JavaFX has a special feature where you can refer to certain other
styles by using a reference (which is resolved, recursively, to a
final value).  This does not seem to be a CSS standard, but is a
feature only FX has.

It works by saying something like:

    -fx-base: RED;

And then using it like this:

    -fx-text-fill: -fx-base;

This feature works accross stylesheets of different origins, so an
AUTHOR stylesheet can specify -fx-base, and when a USER_AGENT
refers to -fx-base, the value comes from the AUTHOR stylesheet.

JavaFX then changes the origin of the style to the highest
priority encountered while resolving the reference.  This means
that Modena can specify "-fx-text-fill: -fx-base", and when
"-fx-base" is then part of the AUTHOR style sheet, that ALL Modena
styles that use -fx-base will be considered AUTHOR level styles,
as per this comment:

// The origin of this parsed value is the greatest of

// any of the resolved reference. If a resolved reference

// comes from an inline style, for example, then the value

// calculated from the resolved lookup should have inline

// as its origin. Otherwise, an inline style could be

// stored in shared cache.

I feel that this is a really unfortunate choice.  The style after
all was specified by Modena, only its value came from another
(higher priority) style sheet.  I think a more logical choice
would have been to not change the priority at all, unless a
"-fx-text-fill" is explicitly made part of the AUTHOR stylesheet.

A consequence of this (and which is much more visible after the
fix) is that creating a Label with a setTextFill(Color.YELLOW) in
its constructor will only result in a yellow text fill if the
AUTHOR stylesheet did not override any of the Modena colors
involved in calculating the Modena -fx-text-fill default.
Overriding -fx-base in any way will result in the text fill of the
label to be overridden (as the reference came from an AUTHOR
stylesheet, which trumps a setter which is of a lower style origin).

The comment also alludes to a potential problem.  If an inline
style would specify "-fx-base", but would be treated as if it came
from Modena (USER_AGENT level), then this value could get stored
in the cache as everything except INLINE styles can be cached. 
However, I feel that the changing of style origin level was then
probably done to solve a CACHING problem, instead of what ma

Re: consistent naming for tests

[John Hendrikx]

On 09/07/2024 16:52, Andy Goryachev wrote:


Two test files consistently generate an error in Eclipse

- ObservableValueFluentBindingsTest
- LazyObjectBindingTest

I admit I have a weird setup (EncFS on Linux Mint running on MacBook 
Pro), and it only manifests itself in Eclipse and not in the gradle 
build - perhaps Eclipse actually verifies the removal of files?


Anyway, a suggestion - if you use @Nested, please keep the class names 
/short/.


This is not an Eclipse bug as I never encounter such issues.  143 
characters is rather short these days, but I suppose we could limit the 
nesting a bit.  Still, I'd look into a way to alleviate this problem in 
your setup, sooner or later this is going to be a problem.


--John

Re: RFR: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty [v7]

[Kevin Rushforth]

On Fri, 28 Jun 2024 10:34:58 GMT, Florian Kirmaier  
wrote:

>> In some situations, a part of the SG is no longer rendered.
>> I created a test program that showcases this problem.
>> 
>> Explanation:
>> 
>> This can happen, when a part of the SG, is covered by another Node.
>> In this part, one node is totally covered, and the other node is visible.
>> 
>> When the totally covered Node is changed, then it is marked dirty and it's 
>> parent, recursively until an already dirty node is found.
>> Due to the Culling, this totally covered Node is not rendered - with the 
>> effect that the tree is never marked as Clean.
>> 
>> In this state, a Node is Dirty but not It's parent. Based on my CodeReview, 
>> this is an invalid state which should never happen.
>> 
>> In this invalid state, when the other Node is changed, which is visible, 
>> then the dirty state is no longer propagated upwards - because the recursive 
>> "NGNode.markTreeDirty" algorithm encounters a dirty node early.
>> 
>> This has the effect, that any SG changes in the visible Node are no longer 
>> rendered. Sometimes the situation repairs itself.
>> 
>> Useful parameters for further investigations:
>> -Djavafx.pulseLogger=true
>> -Dprism.printrendergraph=true
>> -Djavafx.pulseLogger.threshold=0
>> 
>> PR:
>> This PR ensures the dirty flag is set to false of the tree when the culling 
>> is used.
>> It doesn't seem to break any existing tests - but I'm not sure whether this 
>> is the right way to fix it.
>> It would be great to have some feedback on this solution - maybe guiding me 
>> to a better solution.
>> 
>> I could write a test, that just does the same thing as the test application, 
>> but checks every frame that these nodes are not dirty - but maybe there is a 
>> better way to test this.
>
> Florian Kirmaier has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains seven additional 
> commits since the last revision:
> 
>  - Merge branch 'openjdk:master' into JDK-8322544-render-culling
>  - JDK-8322619: Adjust test: remove Thread.sleep(), runAndWait()
>  - JDK-8322619: Add test
>  - Revert "JDK-8322619: Clear dirty flag on the node and all its children if 
> they are skipped due to visible==false or opacity==0"
>
>This reverts commit 5f9f1574515c078c1fd0dccd476325090a0b284d.
>  - JDK-8322619: Clear dirty flag on the node and all its children if they are 
> skipped due to visible==false or opacity==0
>  - reverted accidental change in the .idea folder
>  - JDK-8322619 Fix for rendering bug, related to overlap - culling - 
> dirtynodes

This PR should be closed now that PR #1451 is integrated.

-

Changes requested by kcr (Lead).

PR Review: https://git.openjdk.org/jfx/pull/1310#pullrequestreview-2166623300

Re: consistent naming for tests

[Kevin Rushforth]

I generally like descriptive names for test methods (and classes) rather 
than encoding the bug ID in the name. A comment with the bug ID is very 
helpful, and I would support making that a best practice. If the purpose 
of a test is non-obvious, a comment explaining it is a good idea.


-- Kevin

On 7/9/2024 2:33 AM, Johan Vos wrote:

Hi,

An interesting question from John Hendrikx 
(https://github.com/openjdk/jfx/pull/1283/#discussion_r1637684395) 
probably needs some general discussion on this list.
Afaik, we don't have guidelines for how to name tests (in 
https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md#coding-style-and-testing-guidelines)


In the different test files we have, I see different patterns:
* in some cases, tests are always prefixed with `test` (e.g. `testFoo()`)
* in some cases, tests have a concise but somehow meaningful name 
(e.g. `testScrollBarStaysVisible`)

* in some cases, tests refer to JBS issues (e.g. testJDK8309935)
* in some cases, the test is explained in comments.

I think it would be good to have some consistency going forward. (I'm 
not advocating we need to rename existing tests!)
I don't have a strong preference myself, but I think the link to the 
JBS issue that triggered the creation of a specific test is always 
good to have. I am also very ok with comments, but I learned not 
everyone likes that.


Thoughts?

- Johan

Re: CSS Lookups and their origins (possible regression)

[Andy Goryachev]

I've used this feature in the past to change the colors in all the controls, so 
to me this is the expected behavior.

So in your case (if I got it right), you need to set the direct style on the 
label (.setStyle("-fx-text-fill:yellow")) instead of setting the text fill 
programmatically.  Right?

-andy




From: openjfx-dev  on behalf of John Hendrikx 

Date: Monday, July 8, 2024 at 17:11
To: openjfx-dev 
Subject: Re: CSS Lookups and their origins (possible regression)

I realized I worded the TLDR poorly.

Let me try again:

TLDR; should styles which use references (like -fx-base used in Modena) become 
AUTHOR level styles if -fx-base is specified in an AUTHOR stylesheet?  The act 
of simply specifying -fx-base in your own AUTHOR stylesheet elevates hundreds 
of styles from Modena to AUTHOR level, as if you specified them directly...

--John
On 09/07/2024 02:07, John Hendrikx wrote:

Hi List,

TLDR; should a CSS reference like -fx-base convert all styles that use this 
value (or derive from it) become AUTHOR level styles (higher priority than 
setters) ?

Long version:

In JavaFX 21, I did a fix (see #1072) to solve a problem where a CSS value 
could be reset on an unrelated control.

This happened when the CSS engine encountered a stylable that is overridden by 
the user (with a setter), and decided NOT to proceed with the full CSS value 
calculation (as it could not override the user setting if that CSS value had 
lower priority).  However, not proceeding with the calculation meant that a 
"SKIP" was stored in a shared cache which was incorrect.  This is because when 
this "SKIP" is later encountered for an unrelated control (the cache entries 
are shared for controls with the same styles at the same level), they could get 
their values reset because they were assumed to be unstyled.

However, this fix has exposed what seems to be a deeper bug or perhaps an 
unfortunate default:

JavaFX has a special feature where you can refer to certain other styles by 
using a reference (which is resolved, recursively, to a final value).  This 
does not seem to be a CSS standard, but is a feature only FX has.

It works by saying something like:

-fx-base: RED;

And then using it like this:

-fx-text-fill: -fx-base;

This feature works accross stylesheets of different origins, so an AUTHOR 
stylesheet can specify -fx-base, and when a USER_AGENT refers to -fx-base, the 
value comes from the AUTHOR stylesheet.

JavaFX then changes the origin of the style to the highest priority encountered 
while resolving the reference.  This means that Modena can specify 
"-fx-text-fill: -fx-base", and when "-fx-base" is then part of the AUTHOR style 
sheet, that ALL Modena styles that use -fx-base will be considered AUTHOR level 
styles, as per this comment:

// The origin of this parsed value is the greatest of

// any of the resolved reference. If a resolved reference

// comes from an inline style, for example, then the value

// calculated from the resolved lookup should have inline

// as its origin. Otherwise, an inline style could be

// stored in shared cache.

I feel that this is a really unfortunate choice.  The style after all was 
specified by Modena, only its value came from another (higher priority) style 
sheet.  I think a more logical choice would have been to not change the 
priority at all, unless a "-fx-text-fill" is explicitly made part of the AUTHOR 
stylesheet.

A consequence of this (and which is much more visible after the fix) is that 
creating a Label with a setTextFill(Color.YELLOW) in its constructor will only 
result in a yellow text fill if the AUTHOR stylesheet did not override any of 
the Modena colors involved in calculating the Modena -fx-text-fill default.  
Overriding -fx-base in any way will result in the text fill of the label to be 
overridden (as the reference came from an AUTHOR stylesheet, which trumps a 
setter which is of a lower style origin).

The comment also alludes to a potential problem.  If an inline style would 
specify "-fx-base", but would be treated as if it came from Modena (USER_AGENT 
level), then this value could get stored in the cache as everything except 
INLINE styles can be cached.  However, I feel that the changing of style origin 
level was then probably done to solve a CACHING problem, instead of what made 
logical sense for users.  If we agree that a resolved reference should not 
change the style origin level, then this would need to be addressed, by perhaps 
marking such a calculated value as uncacheable, instead of overloading the 
meaning of style origin.

I'd like to hear your thoughts, and also how to proceed.  JavaFX versions 
before 21 seemingly allowed overriding reference without much consequence 
because if the user overrode the value manually, the cache entry would be set 
to "SKIP".  Now that this is no longer the case, JavaFX more aggressively 
overrides user set values if they happen to use a referenced value.  See code 
below.

--John

.root

Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v18]

[Kevin Rushforth]

On Wed, 19 Jun 2024 14:25:52 GMT, Ambarish Rapte  wrote:

>> Johan Vos has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - process more reviewer comments
>>  - Process reviewer comments
>
> tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
>  line 321:
> 
>> 319: Stage stage = new Stage();
>> 320: stage.setScene(new Scene(root));
>> 321: stage.show();
> 
> I think, a CountDownLatch should be added to make sure that stage is shown 
> before proceeding.

One correction: `Stage.show` is not a blocking call. I haven't looked at it 
closely, but perhaps what @arapte meant was to suggest that the test wait on a 
latch that is triggered when the Stage::showing event is delivered? Many of our 
headful tests do that, but I don't know whether that would be applicable here 
or not.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1670667153

Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v18]

[Kevin Rushforth]

On Tue, 9 Jul 2024 09:13:13 GMT, Johan Vos  wrote:

>> tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
>>  line 1:
>> 
>>> 1: /*
>> 
>> This test file is moved from a different location, could do `git mv` instead 
>> removing and adding.
>
> I manually reverted the add/remove part, and replaced it with `git mv`. I 
> assume/hope that by squashing the commits, the add/remove will not be part of 
> the change.

git doesn't actually track renames and copies, so there is ultimately no 
difference between a "git mv" and "git rm; git add", at least not when done as 
part of a single commit. What git does is a heuristic when presenting diffs 
based on how similar the two files are.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1670664270

Re: consistent naming for tests

[Andy Goryachev]

+1 for comments, links to the JS tickets, and descriptive names.

Speaking of the test names, I wanted to bring up an issue that I've encountered 
recently which concerns the use of @Nested -
https://bugs.openjdk.org/browse/JDK-8334497

Two test files consistently generate an error in Eclipse
- ObservableValueFluentBindingsTest
- LazyObjectBindingTest

I admit I have a weird setup (EncFS on Linux Mint running on MacBook Pro), and 
it only manifests itself in Eclipse and not in the gradle build - perhaps 
Eclipse actually verifies the removal of files?

Anyway, a suggestion - if you use @Nested, please keep the class names short.

Thank you
-andy





From: openjfx-dev  on behalf of Johan Vos 

Date: Tuesday, July 9, 2024 at 02:33
To: openjfx-dev 
Subject: consistent naming for tests
Hi,

An interesting question from John Hendrikx 
(https://github.com/openjdk/jfx/pull/1283/#discussion_r1637684395) probably 
needs some general discussion on this list.
Afaik, we don't have guidelines for how to name tests (in 
https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md#coding-style-and-testing-guidelines)

In the different test files we have, I see different patterns:
* in some cases, tests are always prefixed with `test` (e.g. `testFoo()`)
* in some cases, tests have a concise but somehow meaningful name (e.g. 
`testScrollBarStaysVisible`)
* in some cases, tests refer to JBS issues (e.g. testJDK8309935)
* in some cases, the test is explained in comments.

I think it would be good to have some consistency going forward. (I'm not 
advocating we need to rename existing tests!)
I don't have a strong preference myself, but I think the link to the JBS issue 
that triggered the creation of a specific test is always good to have. I am 
also very ok with comments, but I learned not everyone likes that.

Thoughts?

- Johan

Re: RFR: 8323706: Move SimpleSelector and CompoundSelector to internal packages [v2]

[Kevin Rushforth]

On Tue, 9 Jul 2024 13:03:18 GMT, John Hendrikx  wrote:

> I think this is ready for a closer inspection again

I'll put it on my review queue for after Thursday's fork.

> in a strict sense there are API additions in this PR, but because of how 
> `sealed` works, they're not accessible except to permitted sub types (and 
> none of those permitted types are public API).

Right, the API additions are the package-scope constructor and the `readBinary` 
method, although as you say, they are only accessible to internal JavaFX 
classes. Since `Selector` will be a sealed class with all permitted classes 
being internal, no package-scope methods can be accessed by an application.

The main API change is the removal of the terminally-deprecated 
`SimpleSelector` and `CompoundSelector` classes. With that in mind, a better 
title for this bug would be:

"Remove SimpleSelector and CompoundSelector classes"

You could add the existing title as a summary:

"/summary Move SimpleSelector and CompoundSelector to internal packages"

-

PR Comment: https://git.openjdk.org/jfx/pull/1333#issuecomment-2217830387

Re: RFR: 8323706: Move SimpleSelector and CompoundSelector to internal packages [v2]

[John Hendrikx]

On Fri, 2 Feb 2024 17:22:22 GMT, Kevin Rushforth  wrote:

>> John Hendrikx has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Add since tags
>
> This PR should probably be moved to draft. Now that the deprecation for 
> removal is targeted for 23, this won't happen until JavaFX 24.

@kevinrushforth I think this is ready for a closer inspection again; in a 
strict sense there are API additions in this PR, but because of how `sealed` 
works, they're not accessible except to permitted sub types (and none of those 
permitted types are public API).

-

PR Comment: https://git.openjdk.org/jfx/pull/1333#issuecomment-2217687797

Re: RFR: 8335218: Eclipse Config: Remove Gradle Integration

[John Hendrikx]

On Wed, 26 Jun 2024 21:23:34 GMT, Andy Goryachev  wrote:

> This might be controversial. I am proposing to remove the Gradle integration 
> in the Eclipse config files.
> 
> Problem
> ===
> Eclipse Gradle integration (Buildship) cannot import the OpenJFX build.gradle 
> cleanly. Every time the project is imported into a new workspace (or 
> re-opened after being closed) it executes Gradle, creates and modifies a 
> number of Eclipse .project and .classpath files, all of which need to be 
> reverted for Eclipse workspace to become usable again.
> 
> Solution
> ==
> Remove Gradle nature from the Eclipse project files. This change only affects 
> Eclipse config files and does not impact build.gradle or other IDEs.
> 
> Advantages
> =
> 1. The multiple nested projects in the repo will get imported cleanly on the 
> first attempt, will not require additional steps to clear the Buildship 
> changes.
> 2. completely removes the dependency on the Eclipse Buildship and its 
> idiosyncrasies.
> 
> NOTES:
> - even though the reverse was done for IntelliJ, but its gradle import still 
> does not import tests cleanly, see 
> [JDK-8223373](https://bugs.openjdk.org/browse/JDK-8223373)
> - this improvement contradicts 
> [JDK-8223374](https://bugs.openjdk.org/browse/JDK-8223374) as without Eclipse 
> files in the repo, it will be impossible to use Eclipse in a meaningful way 
> without the fully functional Buildship support, and that is a big IF.
> - once integrated, Eclipse users would only need to re-import the main 
> project with 'search for nested projects' enabled

Marked as reviewed by jhendrikx (Reviewer).

-

PR Review: https://git.openjdk.org/jfx/pull/1491#pullrequestreview-2166250937

Integrated: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty

[eduardsdv]

On Mon, 6 May 2024 14:14:05 GMT, eduardsdv  wrote:

> This is an alternative solution to the PR: 
> https://github.com/openjdk/jfx/pull/1310.
> 
> This solution is based on the invariant that if a node is marked as dirty, 
> all ancestors must also be marked as dirty and that if an ancestor is marked 
> as clean, all descendants must also be marked as clean. 
> Therefore I removed the ``clearDirtyTree()`` method and put its content to 
> the ``clearDirty()`` method.
> 
> Furthermore, since dirty flag is only used when rendering by ``ViewPainter``, 
> it should also be deleted by ``ViewPainter`` only. 
> This guarantees:
> 1. that all dirty flags are removed after rendering, and 
> 2. that no dirty flags are removed when a node is rendered, e.g. by creating 
> a snapshot or printing.
> Therefore I removed all calls of the methods ``clearDirty()`` and 
> ``clearDirtyTree()`` from all other classes except the ``ViewerPainter``.
> 
> The new version of the ``clearDirty()`` method together with calling it from 
> the ``ViewerPainter`` needs to visit far fewer nodes compared to the version 
> prior this PR.
> 
> The supplied test checks that the nodes are updated even if they are 
> partially covered, which led to the error in the version before the PR. The 
> test can be started with:
> ``gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests 
> NGNodeDirtyFlagTest``

This pull request has now been integrated.

Changeset: 9723a9c4
Author:Eduard Sedov 
Committer: Lukasz Kostyra 
URL:   
https://git.openjdk.org/jfx/commit/9723a9c4ce3a4490df962b83da0df71b1e5145f1
Stats: 243 lines in 11 files changed: 172 ins; 47 del; 24 mod

8322619: Parts of SG no longer update during rendering - overlapping - culling 
- dirty

Reviewed-by: arapte, lkostyra

-

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

Re: RFR: 8323706: Move SimpleSelector and CompoundSelector to internal packages [v3]

[John Hendrikx]

> Moves `SimpleSelector` and `CompoundSelector` to internal packages.
> 
> This can be done with only a minor API break, as `SimpleSelector` and 
> `CompoundSelector` were public before.  However, these classes could not be 
> constructed by 3rd parties.  The only way to access them was by doing a cast 
> (generally they're accessed via `Selector` not by their sub types).  The 
> reason they were public at all was because the CSS engine needs to be able to 
> access them from internal packages.
> 
> This change fixes a mistake (or possibly something that couldn't be modelled 
> at the time) when the CSS API was first made public. The intention was always 
> to have a `Selector` interface/abstract class, with private implementations 
> (`SimpleSelector` and `CompoundSelector`).
> 
> This PR as said has a small API break.  The other changes are (AFAICS) source 
> and binary compatible:
> 
> - Made `Selector` `sealed` only permitting `SimpleSelector` and 
> `CompoundSelector` -- as `Selector` had a package private constructor, there 
> are no concerns with pre-existing subclasses
> - `Selector` has a few more methods that are now `protected` -- given that 
> the class is now sealed, these modified methods are not accessible (they may 
> still require rudimentary documentation I suppose)
> - `Selector` now has a `public` default constructor -- as the class is 
> sealed, it is inaccessible
> - `SimpleSelector` and `CompoundSelector` have a few more `public` methods, 
> but they're internal now, so it is irrelevant
> - `createMatch` was implemented directly in `Selector` to avoid having to 
> expose package private fields in `Match` for use by `CompoundSelector`
> - No need anymore for the `SimpleSelectorShim`

John Hendrikx has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains three commits:

 - Merge branch 'master' of https://git.openjdk.org/jfx into 
feature/selectors-to-private-api-standalone
 - Add since tags
 - Move SimpleSelector and CompoundSelector to private classes

-

Changes: https://git.openjdk.org/jfx/pull/1333/files
  Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1333&range=02
  Stats: 178 lines in 10 files changed: 49 ins; 90 del; 39 mod
  Patch: https://git.openjdk.org/jfx/pull/1333.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1333/head:pull/1333

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

Re: consistent naming for tests

[John Hendrikx]

On 09/07/2024 11:33, Johan Vos wrote:

Hi,

An interesting question from John Hendrikx 
(https://github.com/openjdk/jfx/pull/1283/#discussion_r1637684395) 
probably needs some general discussion on this list.
Afaik, we don't have guidelines for how to name tests (in 
https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md#coding-style-and-testing-guidelines)


In the different test files we have, I see different patterns:
* in some cases, tests are always prefixed with `test` (e.g. `testFoo()`)
* in some cases, tests have a concise but somehow meaningful name 
(e.g. `testScrollBarStaysVisible`)

* in some cases, tests refer to JBS issues (e.g. testJDK8309935)
* in some cases, the test is explained in comments.

I think it would be good to have some consistency going forward. (I'm 
not advocating we need to rename existing tests!)
I don't have a strong preference myself, but I think the link to the 
JBS issue that triggered the creation of a specific test is always 
good to have. I am also very ok with comments, but I learned not 
everyone likes that.


Hi Johan,

I didn't like the test name primarily because it has an indirection in 
it -- to know what it is supposed to do, I need to go to another system, 
log in, and read the issue.  The issue often however also won't really 
have just one problem or just a tiny description.  So although I think 
it is fine to refer to an issue number, I think the test itself would 
still benefit of a reminder what it is testing (and I see you added a 
comment now, so that helps).


Same goes for comments in the code; I often just see code lines marked 
with "RT-xyz" or "JDK-xyz" -- sometimes referring a non-public issue.  
Just a small description in the code what the problem was is way more 
helpful to (future) maintainers, like ourselves, then a random number 
without further explanation.


As for test naming, it will depend a bit on what you're testing. 
Sometimes using a nested class pattern like this is good:


    class ListTest {
    @Nested
    class WhenEmpty {
  void thenIsEmptyShouldReturnTrue() {
   assertThat(list.isEmpty()).isTrue();
  }
  void thenSizeShouldBe0() {
   assertThat(list.size()).isEqualTo(0);
  }
  void thenGettingFirstElementShouldThrowException() {
   assertThatThrownBy(() -> list.get(0))
.isInstanceOf(IndexOutOfBoundsException.class)
    .hasMessage("index 0");
  }

  @Nested
  class AndFiveElementsAreAdded {
    // add 5 elements in beforeEach

    void thenIsEmptyShouldReturnFalse() {}
                    // etc
  }
    }

    // or if nesting becomes too crazy, start from base again:
    @Nested
    class WhenContainingFiveElements {
  void thenIterationShouldReturnThoseFiveElements() {
    assertThat(list).containsExactly(List.of("A", "B", 
"C", "D", "E"));

  }
  // etc.
    }
    }

The above pattern has top level nested classes start with "When" 
followed by a given state, nested classes start with "And" and a 
modification of the state, and test method naming follows the pattern 
"then  should ".  When combined with a 
DisplayNameGenerator that transforms camel case names to spaced names, 
the tests form more or less normal English sentences.


(Note: I used AssertJ here for much more fluently readable asserts)

That said, I wouldn't go for a fixed naming pattern for test methods as 
it can be a bit situational, but I do think they should be descriptive.


--John



Thoughts?

- Johan

Re: RFR: 8331603: Cleanup native AbstractSurface methods getRGBImpl, setRGBImpl [v2]

[Kevin Rushforth]

On Tue, 9 Jul 2024 07:23:09 GMT, Ambarish Rapte  wrote:

>> The parameter "offset" is not validated in the 2 native methods getRGBImpl() 
>> and setRGBImpl() of com.sun.pisces.AbstractSurface (in JAbstractSurface.c).
>> The PR adds the "offset < 0" check to both the methods.
>
> Ambarish Rapte has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   const vars

Marked as reviewed by kcr (Lead).

-

PR Review: https://git.openjdk.org/jfx/pull/1497#pullrequestreview-2166070380

Re: RFR: 8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed [v9]

[Kevin Rushforth]

On Mon, 8 Jul 2024 22:53:24 GMT, Andy Goryachev  wrote:

>> We have many tests that will occasionally fail with a heavily loaded system.
>> 
>> The question is whether we can come up with a max delta that we can reliably 
>> use that is less than the difference between the default value and the test 
>> value. Otherwise, we can't distinguish them.
>> 
>> On two different test systems, both of which fail pretty consistently with  
>> 50 msec, I see a 100% pass rate over several tries with 150 msec. More 
>> testing is needed.
>
> Another possibility is the code that measures the time it takes to show the 
> tooltip.
> 
> The current code uses Util.waitForLatch(), L244 which was written for a 
> different use case and actually introduces a small measurement error as it 
> includes the time needed for the context switch.  Perhaps a better solution 
> would be to note the timestamp in L271 where the tooltip listener gets called 
> (similar to the way the start moment is captured in L241)

Good catch. Capturing the current time in the listener will be more accurate 
than doing it after the latch wait.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1394#discussion_r1670360729

Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v16]

[John Hendrikx]

On Tue, 9 Jul 2024 09:09:30 GMT, Johan Vos  wrote:

>> tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
>>  line 287:
>> 
>>> 285: 
>>> 286: @Test
>>> 287: public void testJDK8309935() {
>> 
>> minor: Not sure what our policies are, but I find test names like this 
>> pretty useless, perhaps a short indication what this is doing?
>
> It is a good question. We don't have a consistent approach yet. I personally 
> like the link to the JBS issue as that has (or should have) all info. I added 
> a short indication, but the real info should be on the issue, imho.
> (this is something we should discuss on the mailing list)

I see that you added a comment now, that helps a lot, thanks.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1670352389

Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v21]

[John Hendrikx]

On Tue, 9 Jul 2024 09:06:58 GMT, Johan Vos  wrote:

>> A listener was added but never removed.
>> This patch removes the listener when the menu it links to is cleared. Fix 
>> for https://bugs.openjdk.org/browse/JDK-8319779
>
> Johan Vos has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Allow scheduled runnables to be executed on the FX App Thread before 
> checking for exceptions.

LGTM, note that I couldn't test this as I don't have access to a Mac, but I 
didn't see a problem on Windows with these changes.

-

Marked as reviewed by jhendrikx (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1283#pullrequestreview-2166059441

Re: RFR: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty [v6]

[duke]

On Thu, 4 Jul 2024 08:06:39 GMT, eduardsdv  wrote:

>> This is an alternative solution to the PR: 
>> https://github.com/openjdk/jfx/pull/1310.
>> 
>> This solution is based on the invariant that if a node is marked as dirty, 
>> all ancestors must also be marked as dirty and that if an ancestor is marked 
>> as clean, all descendants must also be marked as clean. 
>> Therefore I removed the ``clearDirtyTree()`` method and put its content to 
>> the ``clearDirty()`` method.
>> 
>> Furthermore, since dirty flag is only used when rendering by 
>> ``ViewPainter``, it should also be deleted by ``ViewPainter`` only. 
>> This guarantees:
>> 1. that all dirty flags are removed after rendering, and 
>> 2. that no dirty flags are removed when a node is rendered, e.g. by creating 
>> a snapshot or printing.
>> Therefore I removed all calls of the methods ``clearDirty()`` and 
>> ``clearDirtyTree()`` from all other classes except the ``ViewerPainter``.
>> 
>> The new version of the ``clearDirty()`` method together with calling it from 
>> the ``ViewerPainter`` needs to visit far fewer nodes compared to the version 
>> prior this PR.
>> 
>> The supplied test checks that the nodes are updated even if they are 
>> partially covered, which led to the error in the version before the PR. The 
>> test can be started with:
>> ``gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests 
>> NGNodeDirtyFlagTest``
>
> eduardsdv has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8322619: Remove waiting for the QuantumRenderer

@eduardsdv 
Your change (at version a7646e8ed072e51e1fd912ffa1f7087024f3557a) is now ready 
to be sponsored by a Committer.

-

PR Comment: https://git.openjdk.org/jfx/pull/1451#issuecomment-2217405349

Re: RFR: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty [v6]

[Lukasz Kostyra]

On Thu, 4 Jul 2024 08:06:39 GMT, eduardsdv  wrote:

>> This is an alternative solution to the PR: 
>> https://github.com/openjdk/jfx/pull/1310.
>> 
>> This solution is based on the invariant that if a node is marked as dirty, 
>> all ancestors must also be marked as dirty and that if an ancestor is marked 
>> as clean, all descendants must also be marked as clean. 
>> Therefore I removed the ``clearDirtyTree()`` method and put its content to 
>> the ``clearDirty()`` method.
>> 
>> Furthermore, since dirty flag is only used when rendering by 
>> ``ViewPainter``, it should also be deleted by ``ViewPainter`` only. 
>> This guarantees:
>> 1. that all dirty flags are removed after rendering, and 
>> 2. that no dirty flags are removed when a node is rendered, e.g. by creating 
>> a snapshot or printing.
>> Therefore I removed all calls of the methods ``clearDirty()`` and 
>> ``clearDirtyTree()`` from all other classes except the ``ViewerPainter``.
>> 
>> The new version of the ``clearDirty()`` method together with calling it from 
>> the ``ViewerPainter`` needs to visit far fewer nodes compared to the version 
>> prior this PR.
>> 
>> The supplied test checks that the nodes are updated even if they are 
>> partially covered, which led to the error in the version before the PR. The 
>> test can be started with:
>> ``gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests 
>> NGNodeDirtyFlagTest``
>
> eduardsdv has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8322619: Remove waiting for the QuantumRenderer

Changes look good. Run all tests on all platforms I have available for me (Win 
11, macOS 14.5, Ubuntu 22.04) and noticed no regressions or issues. Ensemble 
also works fine.

-

Marked as reviewed by lkostyra (Committer).

PR Review: https://git.openjdk.org/jfx/pull/1451#pullrequestreview-2166014426

consistent naming for tests

[Johan Vos]

Hi,

An interesting question from John Hendrikx (
https://github.com/openjdk/jfx/pull/1283/#discussion_r1637684395) probably
needs some general discussion on this list.
Afaik, we don't have guidelines for how to name tests (in
https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md#coding-style-and-testing-guidelines
)

In the different test files we have, I see different patterns:
* in some cases, tests are always prefixed with `test` (e.g. `testFoo()`)
* in some cases, tests have a concise but somehow meaningful name (e.g.
`testScrollBarStaysVisible`)
* in some cases, tests refer to JBS issues (e.g. testJDK8309935)
* in some cases, the test is explained in comments.

I think it would be good to have some consistency going forward. (I'm not
advocating we need to rename existing tests!)
I don't have a strong preference myself, but I think the link to the JBS
issue that triggered the creation of a specific test is always good to
have. I am also very ok with comments, but I learned not everyone likes
that.

Thoughts?

- Johan

Re: RFR: 8335934: Change JavaFX release version to 24

[Johan Vos]

On Mon, 8 Jul 2024 22:51:47 GMT, Kevin Rushforth  wrote:

> Bump the version number of JavaFX to 24. I will integrate this to `master` as 
> part of forking the `jfx23` stabilization branch, which is scheduled for 
> Thursday, July 11, 2024 at 16:00 UTC.

Marked as reviewed by jvos (Reviewer).

-

PR Review: https://git.openjdk.org/jfx/pull/1499#pullrequestreview-2165650586

Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v18]

[Johan Vos]

On Wed, 19 Jun 2024 14:25:52 GMT, Ambarish Rapte  wrote:

>> Johan Vos has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - process more reviewer comments
>>  - Process reviewer comments
>
> tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
>  line 321:
> 
>> 319: Stage stage = new Stage();
>> 320: stage.setScene(new Scene(root));
>> 321: stage.show();
> 
> I think, a CountDownLatch should be added to make sure that stage is shown 
> before proceeding.

I don't completely understand this. The stage.show() is a blocking call, and 
the runnable that it might schedule will be executed before other runnables 
(part of the processing in the rest of the method) are scheduled.
I added a CountDownLatch at the end of the processing, so that we allow for the 
processing to run into exceptions before we finalize the test.
Does that make sense?

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1670106371

Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v18]

[Johan Vos]

On Wed, 19 Jun 2024 12:39:11 GMT, Ambarish Rapte  wrote:

>> Johan Vos has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - process more reviewer comments
>>  - Process reviewer comments
>
> tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
>  line 1:
> 
>> 1: /*
> 
> This test file is moved from a different location, could do `git mv` instead 
> removing and adding.

I manually reverted the add/remove part, and replaced it with `git mv`. I 
assume/hope that by squashing the commits, the add/remove will not be part of 
the change.

> tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
>  line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
> 
> This file is moved, so the copyright should reflect same. Should be : `2023, 
> 2024`

right, done.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1670094429
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1670091229

Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v16]

[Johan Vos]

On Thu, 13 Jun 2024 07:17:01 GMT, John Hendrikx  wrote:

>> Johan Vos has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Add more type info
>
> tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
>  line 287:
> 
>> 285: 
>> 286: @Test
>> 287: public void testJDK8309935() {
> 
> minor: Not sure what our policies are, but I find test names like this pretty 
> useless, perhaps a short indication what this is doing?

It is a good question. We don't have a consistent approach yet. I personally 
like the link to the JBS issue as that has (or should have) all info. I added a 
short indication, but the real info should be on the issue, imho.
(this is something we should discuss on the mailing list)

> tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
>  line 346:
> 
>> 344: });
>> 345: Util.runAndWait(() -> {
>> 346: });
> 
> I checked the code that `runAndWait` would do when you pass in 0 runnables, 
> and it basically does no waiting at all (less than a microsecond for sure).  
> If this is really essential (I removed it and the test still passed) then I 
> think a `sleep` might be better.

The Util.runAndWait was indeed not a good idea. The problem is not that the 
test might not pass, the problem is that the test might not fail if we don't 
wait for things that need to run to complete. But this is better done with a 
Platform.runLater() which will add a runnable to the queue, so when that is 
executed, we know that all previously scheduled runnables completed.
I changed the approach ehre.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1670088321
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1670090848

Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v21]

[Johan Vos]

> A listener was added but never removed.
> This patch removes the listener when the menu it links to is cleared. Fix for 
> https://bugs.openjdk.org/browse/JDK-8319779

Johan Vos has updated the pull request incrementally with one additional commit 
since the last revision:

  Allow scheduled runnables to be executed on the FX App Thread before checking 
for exceptions.

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1283/files
  - new: https://git.openjdk.org/jfx/pull/1283/files/8e30b998..39bf43da

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx&pr=1283&range=20
 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1283&range=19-20

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

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

Re: RFR: 8335934: Change JavaFX release version to 24

[Ambarish Rapte]

On Mon, 8 Jul 2024 22:51:47 GMT, Kevin Rushforth  wrote:

> Bump the version number of JavaFX to 24. I will integrate this to `master` as 
> part of forking the `jfx23` stabilization branch, which is scheduled for 
> Thursday, July 11, 2024 at 16:00 UTC.

Marked as reviewed by arapte (Reviewer).

-

PR Review: https://git.openjdk.org/jfx/pull/1499#pullrequestreview-2165358897

Re: RFR: 8331603: Cleanup native AbstractSurface methods getRGBImpl, setRGBImpl [v2]

[Ambarish Rapte]

> The parameter "offset" is not validated in the 2 native methods getRGBImpl() 
> and setRGBImpl() of com.sun.pisces.AbstractSurface (in JAbstractSurface.c).
> The PR adds the "offset < 0" check to both the methods.

Ambarish Rapte has updated the pull request incrementally with one additional 
commit since the last revision:

  const vars

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1497/files
  - new: https://git.openjdk.org/jfx/pull/1497/files/7424f752..3e4a8987

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx&pr=1497&range=01
 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1497&range=00-01

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

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

Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v20]

[Johan Vos]

> A listener was added but never removed.
> This patch removes the listener when the menu it links to is cleared. Fix for 
> https://bugs.openjdk.org/browse/JDK-8319779

Johan Vos has updated the pull request incrementally with three additional 
commits since the last revision:

 - Add changes back to the test in the new location
 - Use git mv to move file from old to new location
 - Revert remove/add of test file

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1283/files
  - new: https://git.openjdk.org/jfx/pull/1283/files/55dc803f..8e30b998

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx&pr=1283&range=19
 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1283&range=18-19

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

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