Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0

[Karthik P K]

On Tue, 31 Jan 2023 14:40:00 GMT, Karthik P K  wrote:

> In `selectIndices` method, zero length array is not considered while ignoring 
> row number given as parameter.
> 
> Updated the code to consider both null and zero length array in the condition 
> before ignoring the row value given as parameter.
> 
> Added unit test to validate the fix

Yes similar change can be applied to `TreeTableView`.
Updated the code.

-

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

Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0 [v2]

[Karthik P K]

> In `selectIndices` method, zero length array is not considered while ignoring 
> row number given as parameter.
> 
> Updated the code to consider both null and zero length array in the condition 
> before ignoring the row value given as parameter.
> 
> Added unit test to validate the fix

Karthik P K has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix first index selection issue in TreeTableView

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1018/files
  - new: https://git.openjdk.org/jfx/pull/1018/files/06c1950b..4f623477

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

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

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

Re: RFR: JDK-8299977 : Update WebKit to 615.1 [v3]

[Hima Bindu Meda]

> Update JavaFX WebKit to GTK WebKit 2.38 (615.1).
> 
> Verified the updated version build, sanity tests and stability.
> This does not cause any issues except one unit test failure.
> Also, there are some artifacts observed while playing youtube
> 
> The above issues are recorded and ignored using below bugs:
> [JDK-8300954](https://bugs.openjdk.org/browse/JDK-8300954)
> [JDK-8301022](https://bugs.openjdk.org/browse/JDK-8301022)

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

  Remove the log added for testing purpose

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1011/files
  - new: https://git.openjdk.org/jfx/pull/1011/files/66237d46..a804a6ed

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

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

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

Re: Style themes API

[Michael Strauß]

On Tue, Jan 31, 2023 at 6:14 PM Pedro Duque Vieira
 wrote:
> Thinking of existing themes migrating to this StyleTheme API (90% are 
> implemented as author stylesheets), some visual glitches may start appearing 
> for users of those themes once they start using the migrated versions (since 
> they will go from being author stylesheets to user agent stylesheets).
> So, 2 things to think about:
> 2.1 - How important is it that we think about this use case: existing themes 
> migrating to this new API. I'd say we should probably think about it when 
> designing this new API.
> 2.2 - The second thing then is, how problematic will it be for programmers 
> using these themes to have their apps start having visual glitches because of 
> these themes changing from author stylesheets to user agent style sheets?
> Perhaps we simply don't bother with this last point and simply have it be the 
> price of innovation. And so programmers will have to adapt their applications 
> and remove any code that is overriding the styles defined in the StyleThemes 
> (which are likely causing these visual glitches when migrating to Themes 
> using this new StyleTheme API). The same for any styles already defined in 
> custom author stylesheets that are being used by the application and that 
> might be NOW overriding the styles of the theme.

1) Ease of migration should be an important consideration, but there
are also other considerations. I'd argue that getting the feature
stacking and mental model right is more important, and giving themes a
way to individually select if the stylesheets are user-agent or user
stylesheets makes it hard to understand why my locally-set property
value is being ignored by JavaFX in one theme, but not in the other.

2) The scenario you describe occurs when an application sets the value
of styleable properties from code, but the values are overridden by an
author stylesheet. Yes, that might happen, and I think it's up to the
application developers to fix that. The reason why I think we should
only support `Application.userAgentStyleTheme` in the first version of
this new feature is quite simple: I'd rather have the feature actually
make it into JavaFX than be stuck in review because it's too large and
too complex of a change. User-agent style themes are pretty easy to
implement (most of the code is already in place), while author style
themes are not. We can add author style themes as a follow-up
enhancement.



> I don't think we should put having StyleTheme be just a collection of 
> stylesheets and nothing else as a requirement. I'd rather think of StyleTheme 
> as being the concept of a Theme and all the possible properties and 
> definitions of what a Theme is. This is one of the strengths of this new 
> feature you've proposed, i.e.
> having the concept of a Theme exist in JavaFX whereas in the past there was 
> no such concept and a "Theme" was just a collection of stylesheets. Added 
> through the getStylesheets() method or through the setUserAgentStylesheet 
> method.
>
> I'll just note that we don't just want the applications to be light or dark 
> depending on what is set in the Operating System. You'll also want the 
> ability to set whether the app is in dark or light mode on a per application 
> level irrespective of what is defined in the OS (this already happens on many 
> existing applications out there). So I think that DarkModeAware interface 
> would have to have a method like:
>   ThemeMode getMode();
> Where ThemeMode can either be LIGHT, DARK or OS_DEFINED (I've named it 
> ThemeMode just as an example).
>
> So I think we could do it in 2 ways: one way would be to do it as you say 
> (though DarkModeAware interface would need the getMode() method that I 
> suggested, I think) or add a method in the StyleTheme interface itself in a 
> future release:
>   ThemeMode getMode();
> That method would probably need to have a default implementation that simply 
> returns LIGHT because of retro compatibility.
>
> So, ThemeMode could be one of 3 possible values: LIGHT, DARK or OS_DEFINED. 
> Where OS_DEFINED means that the app will honor whatever is defined at the OS 
> level.
> JavaFX Window decorations would need to respect whatever is returned in this 
> getMode() method and change their decorations to either LIGHT or DARK 
> depending on its value. This would also remove the need for boilerplate code 
> that for every Window that is created sets it to be LIGHT or DARK depending 
> on whether the Theme is LIGHT or DARK.
>
> Of course, we can also simply support this from the get go and have this 
> method exist in StyleTheme from the start.

I'll point you to this document, which describes the latest version of
this feature: https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548
What do you think about the interaction between the various parts of
the new API, especially in regards to the appearance of native
platform decorations?
I'm not convinced that it is a good idea to make 

Re: Style themes API

[Pedro Duque Vieira]

My previous message seems to have perhaps gotten lost in the
various messages exchanged in this mailing list.. (just doing this small
comment so that my previous message doesn't get lost..)

It's getting very late here where I live, I will be back later tomorrow (a
few hours from now).

Thanks

On Tue, Jan 31, 2023 at 5:14 PM Pedro Duque Vieira <
pedro.duquevie...@gmail.com> wrote:

> Hi Michael,
>
> Truly sorry for the late reply but haven't had time to reply sooner,
> unfortunately..
>
> 2 -
>
>> > I've been building javafx themes for years now. Started creating JMetro
>> in JavaFX version 2. During this time I've been following how other themes
>> are being built and I can say that about 90% of the themes (rough estimate)
>> that are out there, are composed of author stylesheets, i.e they override
>> values set from code (including JMetro).
>> > I agree that themes that are to be provided to other developers would
>> probably be better off if they are user agent stylesheets. My point is that
>> migrating those 90% of themes to use this API would be easier if this flag
>> exists otherwise migration would likely introduce UI regressions for
>> developers using those themes.
>> >
>> > To be clear, what I'm proposing is a flag, or enum, or whatever, in
>> StyleTheme that defines whether the stylesheets in the StyleTheme should be
>> a user agent stylesheet or author stylesheet.
>> >
>> > Another point is in making themes that are only to be used locally in a
>> specific app that the developer is creating. It might be of interest that
>> he can toggle this flag and make all settings defined in the theme override
>> any styling set from code (or in FXML) so that he can have a centralized
>> point where all styling is done.
>
>
> I think that the likelihood of this feature making it into the 21
>> release dramatically increases if we keep changes to the CSS subsystem
>> to an absolute minimum. I understand your point, but I want to deliver
>> this feature in the smallest useful package and add more features
>> later with follow-on PRs.
>
>
> That's why I've renamed `Application.styleTheme` to
>> `Application.userAgentStyleTheme`. This makes it unmistakably clear
>> that we're only talking about user-agent stylesheets.
>> Also, it leaves open the possibility to add the
>> `Application.styleTheme` property later, which would cause the theme
>> to be interpreted as comprised of author stylesheets.
>> Classifying author/UA themes at the property level is also more
>> consistent with the way author/UA stylesheets are currently classified
>> in JavaFX.
>> What do you think about the styleTheme/userAgentStyleTheme API,
>> instead of having a flag in the implementation of StyleTheme itself?
>
>
> Since one of the objectives of the PR is to promote Caspian and Modena
>> to first-class theme implementations, it makes sense to focus on UA
>> themes first (since built-in themes are comprised of UA stylesheets).
>
>
> OK.
> Thinking of existing themes migrating to this StyleTheme API (90% are
> implemented as author stylesheets), some visual glitches may start
> appearing for users of those themes once they start using the migrated
> versions (since they will go from being author stylesheets to user agent
> stylesheets).
> So, 2 things to think about:
> 2.1 - How important is it that we think about this use case: existing
> themes migrating to this new API. I'd say we should probably think about it
> when designing this new API.
> 2.2 - The second thing then is, how problematic will it be for programmers
> using these themes to have their apps start having visual glitches because
> of these themes changing from author stylesheets to user agent style
> sheets?
> Perhaps we simply don't bother with this last point and simply have it be
> the price of innovation. And so programmers will have to adapt their
> applications and remove any code that is overriding the styles defined in
> the StyleThemes (which are likely causing these visual glitches when
> migrating to Themes using this new StyleTheme API). The same for any styles
> already defined in custom author stylesheets that are being used by the
> application and that might be NOW overriding the styles of the theme.
>
>
> 3 -
> > Style themes are generally either defined as dark or light. i.e. you'll
> very rarely have a theme that has some Windows with dark decorations and
> others with light decorations. So I think it makes sense to have this as a
> theme property.
> > The ability to toggle decorations (light or dark) on an individual
> Window is a good point you're making. Perhaps we should have a global
> definition in the StyleTheme (whether the theme is light or dark) and a
> definition that can be set per Window.
> > If you set it on the StyleTheme then all Windows will respect it, but
> you can override that value on each Window. If you override it on a
> specific Window, that definition would take precedence over the global
> StyleTheme one.
>
> In general, Style

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

[Michael Strauß]

On Tue, 31 Jan 2023 23:07:12 GMT, Andy Goryachev  wrote:

> I think that, by default, the FX frame decorations should pick up the 
> platform theme (dark, light, accent color, etc.). It should be possible to 
> override this behavior somehow - possibly by setting the base/accent color - 
> but using an enum for this purpose feels unnecessary.

I think it shouldn't do that automatically, because the default themes do not 
support a dark mode out of the box. _No_ theme will support multiple 
appearances without being specifically designed to do so.

If a theme supports multiple appearances, the "just use the platform 
appearance" behavior only requires a single line of code, which also takes care 
of macOS's "auto" appearance:


var stage = new Stage();

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


By the way, `Stage.appearance` doesn't really have anything to do with 
`StyleTheme`. A style theme is the proposed collection of stylesheets that 
define the look of a JavaFX application, while the stage appearance _only_ 
determines the appearance of native window decorations.

On Windows, the dark appearance adds the `DWMA_USE_IMMERSIVE_DARK_MODE` flag to 
the native window (see [Support Dark and Light themes in Win32 
apps](https://learn.microsoft.com/en-us/windows/apps/desktop/modernize/apply-windows-themes)),
 on macOS it chooses the "DarkAqua" appearance instead of the "Aqua" appearance 
(see [Choosing a Specific Appearance for Your macOS 
App](https://developer.apple.com/documentation/appkit/nsappearancecustomization/choosing_a_specific_appearance_for_your_macos_app)).
 There is no middle ground, it's either dark or it's not. Therefore I think the 
enumeration is a useful representation of this distinction.

-

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

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

[Scott Palmer]

On Tue, Jan 31, 2023 at 6:15 PM Andy Goryachev  wrote:

> On Tue, 31 Jan 2023 23:04:50 GMT, Scott Palmer 
> wrote:
>
> > Is it necessary for any application to know about "auto"?
>
> I actually don't know how the "auto" behaves.  From the description it
> seems the colors might actually change gradually throughout the day, thus
> making an enum useless and necessitating the use of the actual colors.
>

>From https://support.apple.com/en-ca/guide/mac-help/mchl52e1c2d2/mac

Auto just changes from light to dark based on the "Night Shift" setting,
which uses either a fixed schedule, or sunset.  It doesn't have any "in
between" modes. Though it does animate the transition of the colors over
1/2 second or so.

The point being that as far as making the application aware of the current
state, auto isn't needed.  As a preference for within the application
(force light or dark, or 'auto' = track the OS setting) it might be
useful.  I would only include it if the Theme proposal is also going to
dynamically track the OS preference and switch Themes for you.  It looks
like you have that covered by having the application bind to the Platform
Preferences value, in which case "auto" would be an application setting
rather than a JavaFX platform setting.

Scott

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

[Andy Goryachev]

On Sun, 29 Jan 2023 17:10:26 GMT, Michael Strauß  wrote:

>> Platform preferences are the preferred UI settings of the operating system. 
>> For example, on Windows this includes the color values identified by the 
>> `Windows.UI.ViewManagement.UIColorType` enumeration; on macOS this includes 
>> the system color values of the `NSColor` class.
>> 
>> Exposing these dynamic values to JavaFX applications allows developers to 
>> create themes that can integrate seamlessly with the color scheme of the 
>> operating system.
>> 
>> Platform preferences are exposed as an `ObservableMap` of platform-specific 
>> key-value pairs, which means that the preferences available on Windows are 
>> different from macOS or Linux. JavaFX provides a small, curated list of 
>> preferences that are available on most platforms, and are therefore exposed 
>> with a platform-independent API:
>> 
>> 
>> public interface Preferences extends ObservableMap {
>> // Platform-independent API
>> ReadOnlyObjectProperty appearanceProperty();
>> ReadOnlyObjectProperty backgroundColorProperty();
>> ReadOnlyObjectProperty foregroundColorProperty();
>> ReadOnlyObjectProperty accentColorProperty();
>> 
>> // Convenience methods to retrieve platform-specific values from the map
>> Optional getInteger(String key);
>> Optional getDouble(String key);
>> Optional getString(String key);
>> Optional getBoolean(String key);
>> Optional getColor(String key);
>> }
>> 
>> 
>> The platform appearance is defined by the new `javafx.stage.Appearance` 
>> enumeration:
>> 
>> 
>> public enum Appearance {
>> LIGHT,
>> DARK
>> }
>> 
>> 
>> An instance of the `Preferences` interface can be retrieved by calling 
>> `Platform.getPreferences()`.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Use Optional for convenience methods in Preferences

mailing list ate a screenshot from the last message.  here it is:

https://user-images.githubusercontent.com/107069028/215905602-1e656e6c-620f-4437-b041-11b6ebc46834.png";>

-

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

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

[Andy Goryachev]

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

> Is it necessary for any application to know about "auto"?

I actually don't know how the "auto" behaves.  From the description it seems 
the colors might actually change gradually throughout the day, thus making an 
enum useless and necessitating the use of the actual colors.

***@***.***

-andy




From: Scott Palmer ***@***.***>
Date: Tuesday, January 31, 2023 at 15:05
To: openjdk/jfx ***@***.***>
Cc: Andy Goryachev ***@***.***>, Comment ***@***.***>
Subject: [External] : Re: [openjdk/jfx] 8301302: Platform preferences API (PR 
#1014)
On Tue, Jan 31, 2023 at 3:00 PM Andy Goryachev ***@***.***>
wrote:

> In the context of adding theme support in javafx, I think this PR is a
> step in the right direction. I also like a small set of
> platform-independent properties like fg and bg colors. I wonder if the same
> approach can be extended for other aspects of platform L&F, like fonts,
> spacing, and may be other aspects (like, hiding scrollbars setting on Mac?)
>
> I agree with @kevinrushforth 
> 
>  - we'd
> need more discussion on the actual implementation. There are a few items
> that I feel are unnecessary, or might be done differently, and I'd like to
> learn what other people think.
>
> Specifically:
>
> 1. Appearance enum seems unnecessary - there might be more choices in
> a specific platform (Mac Ventura has three: dark/light/auto).
>
>
Is it necessary for any application to know about "auto"? Presumably when
the mode automatically changes from one to the other there would be an
event that should be handled, just as if the user changed the setting while
the application was running.

Scott

> Message ID: ***@***.***>
>


—
Reply to this email directly, view it on 
GitHub,
 or 
unsubscribe.
You are receiving this because you commented.Message ID: ***@***.***>

-

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

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

[Scott Palmer]

On Sun, 29 Jan 2023 17:10:26 GMT, Michael Strauß  wrote:

>> Platform preferences are the preferred UI settings of the operating system. 
>> For example, on Windows this includes the color values identified by the 
>> `Windows.UI.ViewManagement.UIColorType` enumeration; on macOS this includes 
>> the system color values of the `NSColor` class.
>> 
>> Exposing these dynamic values to JavaFX applications allows developers to 
>> create themes that can integrate seamlessly with the color scheme of the 
>> operating system.
>> 
>> Platform preferences are exposed as an `ObservableMap` of platform-specific 
>> key-value pairs, which means that the preferences available on Windows are 
>> different from macOS or Linux. JavaFX provides a small, curated list of 
>> preferences that are available on most platforms, and are therefore exposed 
>> with a platform-independent API:
>> 
>> 
>> public interface Preferences extends ObservableMap {
>> // Platform-independent API
>> ReadOnlyObjectProperty appearanceProperty();
>> ReadOnlyObjectProperty backgroundColorProperty();
>> ReadOnlyObjectProperty foregroundColorProperty();
>> ReadOnlyObjectProperty accentColorProperty();
>> 
>> // Convenience methods to retrieve platform-specific values from the map
>> Optional getInteger(String key);
>> Optional getDouble(String key);
>> Optional getString(String key);
>> Optional getBoolean(String key);
>> Optional getColor(String key);
>> }
>> 
>> 
>> The platform appearance is defined by the new `javafx.stage.Appearance` 
>> enumeration:
>> 
>> 
>> public enum Appearance {
>> LIGHT,
>> DARK
>> }
>> 
>> 
>> An instance of the `Preferences` interface can be retrieved by calling 
>> `Platform.getPreferences()`.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Use Optional for convenience methods in Preferences

On Tue, Jan 31, 2023 at 3:00 PM Andy Goryachev ***@***.***>
wrote:

> In the context of adding theme support in javafx, I think this PR is a
> step in the right direction. I also like a small set of
> platform-independent properties like fg and bg colors. I wonder if the same
> approach can be extended for other aspects of platform L&F, like fonts,
> spacing, and may be other aspects (like, hiding scrollbars setting on Mac?)
>
> I agree with @kevinrushforth  - we'd
> need more discussion on the actual implementation. There are a few items
> that I feel are unnecessary, or might be done differently, and I'd like to
> learn what other people think.
>
> Specifically:
>
>1. Appearance enum seems unnecessary - there might be more choices in
>a specific platform (Mac Ventura has three: dark/light/auto).
>
>
Is it necessary for any application to know about "auto"?  Presumably when
the mode automatically changes from one to the other there would be an
event that should be handled, just as if the user changed the setting while
the application was running.

Scott

> Message ID: ***@***.***>
>

-

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

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

[Andy Goryachev]

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

> the reason for the Appearance enumeration and the Preferences.appearance 
> property is to support dark window frames.

I think that, by default, the FX frame decorations should pick up the platform 
theme (dark, light, accent color, etc.).  It should be possible to override 
this behavior somehow - possibly by setting the base/accent color - but using 
an enum for this purpose feels unnecessary.


> Saving the allocation of just a single map with 20-or-so key-value pairs is 
> not a convincing reason to complicate the implementation

It's not just allocation, but also listening to changes at run time.  For 
example, the user changing the theme from light to dark should, in my opinion, 
update any running FX application, unless otherwise is prescribed by the 
application requirements.



-andy






From: mstr2 ***@***.***>
Date: Tuesday, January 31, 2023 at 12:18
To: openjdk/jfx ***@***.***>
Cc: Andy Goryachev ***@***.***>, Comment ***@***.***>
Subject: [External] : Re: [openjdk/jfx] 8301302: Platform preferences API (PR 
#1014)

In the context of adding theme support in javafx, I think this PR is a step in 
the right direction. I also like a small set of platform-independent properties 
like fg and bg colors. I wonder if the same approach can be extended for other 
aspects of platform L&F, like fonts, spacing, and may be other aspects (like, 
hiding scrollbars setting on Mac?)

That could indeed be a useful enhancement.

  1.  Appearance enum seems unnecessary - there might be more choices in a 
specific platform (Mac Ventura has three: dark/light/auto). Perhaps using fg/bg 
color intensity is sufficient to find out whether the overall theme is "dark" 
or "light".

While dark/light mode can indeed be detected just by comparing foreground and 
background color, the reason for the Appearance enumeration and the 
Preferences.appearance property is to support dark window frames. In this 
gist
 (see "Stage appearance"), I've described how the stage appearance and platform 
appearance APIs interact.

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

The use of ObservableMap is debatable.
I think that always initializing platform properties makes it easier to reason 
about the code. Saving the allocation of just a single map with 20-or-so 
key-value pairs is not a convincing reason to complicate the implementation, 
especially since most of the platform preferences implementation lives in the 
Glass toolkit, and not in the user-facing framework.

—
Reply to this email directly, view it on 
GitHub,
 or 
unsubscribe.
You are receiving this because you commented.Message ID: ***@***.***>

-

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

Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0

[Andy Goryachev]

On Tue, 31 Jan 2023 20:03:19 GMT, Nir Lisker  wrote:

> TreeTableView has the same problem.

good point!  could we apply a similar change to TreeTableView:3012?

-

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

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

[Michael Strauß]

On Tue, 31 Jan 2023 20:00:00 GMT, Andy Goryachev  wrote:

> In the context of adding theme support in javafx, I think this PR is a step 
> in the right direction. I also like a small set of platform-independent 
> properties like fg and bg colors. I wonder if the same approach can be 
> extended for other aspects of platform L&F, like fonts, spacing, and may be 
> other aspects (like, hiding scrollbars setting on Mac?)

That could indeed be a useful enhancement.

> 1. Appearance enum seems unnecessary - there might be more choices in a 
> specific platform (Mac Ventura has three: dark/light/auto).  Perhaps using 
> fg/bg color intensity is sufficient to find out whether the overall theme is 
> "dark" or "light".

While dark/light mode can indeed be detected just by comparing foreground and 
background color, the reason for the `Appearance` enumeration and the 
`Preferences.appearance` property is to support dark window frames. In [this 
gist](https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548) (see 
"Stage appearance"), I've described how the stage appearance and platform 
appearance APIs interact.

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

The use of `ObservableMap` is debatable.
I think that always initializing platform properties makes it easier to reason 
about the code. Saving the allocation of just a single map with 20-or-so 
key-value pairs is not a convincing reason to complicate the implementation, 
especially since most of the platform preferences implementation lives in the 
Glass toolkit, and not in the user-facing framework.

-

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

Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0

[Nir Lisker]

On Tue, 31 Jan 2023 14:40:00 GMT, Karthik P K  wrote:

> In `selectIndices` method, zero length array is not considered while ignoring 
> row number given as parameter.
> 
> Updated the code to consider both null and zero length array in the condition 
> before ignoring the row value given as parameter.
> 
> Added unit test to validate the fix

`TreeTableView` has the same problem.

-

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

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

[Andy Goryachev]

On Sun, 29 Jan 2023 17:10:26 GMT, Michael Strauß  wrote:

>> Platform preferences are the preferred UI settings of the operating system. 
>> For example, on Windows this includes the color values identified by the 
>> `Windows.UI.ViewManagement.UIColorType` enumeration; on macOS this includes 
>> the system color values of the `NSColor` class.
>> 
>> Exposing these dynamic values to JavaFX applications allows developers to 
>> create themes that can integrate seamlessly with the color scheme of the 
>> operating system.
>> 
>> Platform preferences are exposed as an `ObservableMap` of platform-specific 
>> key-value pairs, which means that the preferences available on Windows are 
>> different from macOS or Linux. JavaFX provides a small, curated list of 
>> preferences that are available on most platforms, and are therefore exposed 
>> with a platform-independent API:
>> 
>> 
>> public interface Preferences extends ObservableMap {
>> // Platform-independent API
>> ReadOnlyObjectProperty appearanceProperty();
>> ReadOnlyObjectProperty backgroundColorProperty();
>> ReadOnlyObjectProperty foregroundColorProperty();
>> ReadOnlyObjectProperty accentColorProperty();
>> 
>> // Convenience methods to retrieve platform-specific values from the map
>> Optional getInteger(String key);
>> Optional getDouble(String key);
>> Optional getString(String key);
>> Optional getBoolean(String key);
>> Optional getColor(String key);
>> }
>> 
>> 
>> The platform appearance is defined by the new `javafx.stage.Appearance` 
>> enumeration:
>> 
>> 
>> public enum Appearance {
>> LIGHT,
>> DARK
>> }
>> 
>> 
>> An instance of the `Preferences` interface can be retrieved by calling 
>> `Platform.getPreferences()`.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Use Optional for convenience methods in Preferences

In the context of adding theme support in javafx, I think this PR is a step in 
the right direction.  I also like a small set of platform-independent 
properties like fg and bg colors.  I wonder if the same approach can be 
extended for other aspects of platform L&F, like fonts, spacing, and may be 
other aspects (like, hiding scrollbars setting on Mac?)

I agree with @kevinrushforth - we'd need more discussion on the actual 
implementation.  There are a few items that I feel are unnecessary, or might be 
done differently, and I'd like to learn what other people think.

Specifically:

1. Appearance enum seems unnecessary - there might be more choices in a 
specific platform (Mac Ventura has three: dark/light/auto).  Perhaps using 
fg/bg color intensity is sufficient to find out whether the overall theme is 
"dark" or "light".
2. ObservableMap.  Similarly to Node.getProperties(), I wonder if there might 
be a better way to observe the changes.  May be a different metaphor 
(subscription?), like adding a value change listener to a specific key.  We do 
need a set of keys (perhaps that can be an ObservableSet).  Having said that, 
ObservableMap is good enough solution, and forgive me for stating the obvious, 
it should not initialize anything if the platform properties have not been 
requested by the application code.

What do you think?

-

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

Re: Style themes update

[Michael Strauß]

I've created a Gist with the updated proposal, this seems easier than
sending around large amounts of text for every change:

https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548

Re: RFR: 8298382: JavaFX ChartArea Accessibility Reader

[Alexander Zuev]

On Tue, 31 Jan 2023 18:48:41 GMT, Kevin Rushforth  wrote:

> Can you provide an evaluation of the bug and a description of the fix?

Done.

-

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

Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0

[John Hendrikx]

On Tue, 31 Jan 2023 19:40:06 GMT, John Hendrikx  wrote:

>> I had the same thought here.
>> The signature is indeed weird, but since it is perfectly legal to invoke it 
>> with selectIndices(0, null), I think this change is absolutely correct.
>
> Well it's legal Java code, but that doesn't mean that leaving something 
> `null` is allowed.  At the very least it is undocumented behavior:
> 
> /**
>  * This method allows for one or more selections to be set at the same 
> time.
>  * It will ignore any value that is not within the valid range (i.e. 
> greater
>  * than or equal to zero, and less than the total number of items in the
>  * underlying data model). Any duplication of indices will be ignored.
>  *
>  * If there is already one or more indices selected in this model, 
> calling
>  * this method will not clear these selections - to do so it is
>  * necessary to first call clearSelection.
>  *
>  * The last valid value given will become the selected index / selected
>  * item.
>  * @param index the first index to select
>  * @param indices zero or more additional indices to select
>  */
> public abstract void selectIndices(int index, int... indices);

I think it is also pretty clear the original author intended to check 
`rows.length == 0` and made the mistake that it would be called with `rows == 
null` when there are no further indices specified, which is incorrect.

-

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

Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0

[John Hendrikx]

On Tue, 31 Jan 2023 19:36:12 GMT, Andy Goryachev  wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java 
>> line 2645:
>> 
>>> 2643: 
>>> 2644: @Override public void selectIndices(int row, int... rows) {
>>> 2645: if (rows == null || rows.length == 0) {
>> 
>> To get `rows` to be `null` you'd have to call this method as: 
>> `selectIndices(2, null)` -- IMHO that should result in an exception, and not 
>> be accepted to be the same as `selectIndices(2)` or `selectIndices(2, new 
>> int[0])`.
>> 
>> I checked the documentation, and it does not mention that `null` is allowed 
>> here, which means it isn't.
>> 
>> Also: weird method signature, but I guess if you want to enforce at least 
>> one parameter it could be done this way.
>
> I had the same thought here.
> The signature is indeed weird, but since it is perfectly legal to invoke it 
> with selectIndices(0, null), I think this change is absolutely correct.

Well it's legal Java code, but that doesn't mean that leaving something `null` 
is allowed.  At the very least it is undocumented behavior:

/**
 * This method allows for one or more selections to be set at the same 
time.
 * It will ignore any value that is not within the valid range (i.e. greater
 * than or equal to zero, and less than the total number of items in the
 * underlying data model). Any duplication of indices will be ignored.
 *
 * If there is already one or more indices selected in this model, 
calling
 * this method will not clear these selections - to do so it is
 * necessary to first call clearSelection.
 *
 * The last valid value given will become the selected index / selected
 * item.
 * @param index the first index to select
 * @param indices zero or more additional indices to select
 */
public abstract void selectIndices(int index, int... indices);

-

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

Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0

[Andy Goryachev]

On Tue, 31 Jan 2023 19:31:46 GMT, John Hendrikx  wrote:

>> In `selectIndices` method, zero length array is not considered while 
>> ignoring row number given as parameter.
>> 
>> Updated the code to consider both null and zero length array in the 
>> condition before ignoring the row value given as parameter.
>> 
>> Added unit test to validate the fix
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java 
> line 2645:
> 
>> 2643: 
>> 2644: @Override public void selectIndices(int row, int... rows) {
>> 2645: if (rows == null || rows.length == 0) {
> 
> To get `rows` to be `null` you'd have to call this method as: 
> `selectIndices(2, null)` -- IMHO that should result in an exception, and not 
> be accepted to be the same as `selectIndices(2)` or `selectIndices(2, new 
> int[0])`.
> 
> I checked the documentation, and it does not mention that `null` is allowed 
> here, which means it isn't.
> 
> Also: weird method signature, but I guess if you want to enforce at least one 
> parameter it could be done this way.

I had the same thought here.
The signature is indeed weird, but since it is perfectly legal to invoke it 
with selectIndices(0, null), I think this change is absolutely correct.

-

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

Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0

[John Hendrikx]

On Tue, 31 Jan 2023 14:40:00 GMT, Karthik P K  wrote:

> In `selectIndices` method, zero length array is not considered while ignoring 
> row number given as parameter.
> 
> Updated the code to consider both null and zero length array in the condition 
> before ignoring the row value given as parameter.
> 
> Added unit test to validate the fix

Change looks good, I just feel we shouldn't be catering to `null` here at all.

modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 
2645:

> 2643: 
> 2644: @Override public void selectIndices(int row, int... rows) {
> 2645: if (rows == null || rows.length == 0) {

To get `rows` to be `null` you'd have to call this method as: `selectIndices(2, 
null)` -- IMHO that should result in an exception, and not be accepted to be 
the same as `selectIndices(2)` or `selectIndices(2, new int[0])`.

I checked the documentation, and it does not mention that `null` is allowed 
here, which means it isn't.

Also: weird method signature, but I guess if you want to enforce at least one 
parameter it could be done this way.

-

Marked as reviewed by jhendrikx (Committer).

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

Re: RFR: 8251862: Wrong position of Popup windows at the intersection of 2 screens [v4]

[Kevin Rushforth]

> On Windows platforms with more than one screen, a PopupWindow created for a 
> Stage that straddles two windows will be drawn with an incorrect position and 
> screen scale if the majority of the Stage is on one screen, and the popup is 
> positioned on the other screen. In this case, the Stage is drawn using the 
> screen scale of the screen that most of the window is on, while the popup is 
> drawn using the scale of the screen that it is (typically entirely) on.
> 
> The most common way this can happen is when you have two screens of a 
> different scale with the secondary screen on the left or above the primary 
> screen. If you position the Stage such that most of it is still on the 
> primary screen (thus the Stage is drawn using the scale of the primary 
> screen), with a menu, a control with a context menu, or a control with a 
> Tooltip now on the secondary screen, the popup window for the menu or Tooltip 
> will be drawn using the screen scale of the secondary window and thus won't 
> be positioned or sized correctly relative to the menu bar, or control in the 
> main window.
> 
> The fix implemented by this PR is to always use the screen of the owner 
> window, including the screen scales, when rendering a popup window. This 
> matches the behavior of native Windows apps, such as Notepad.

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

  Update copyright year to 2023

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/971/files
  - new: https://git.openjdk.org/jfx/pull/971/files/4d08c853..d256762b

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

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

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

Re: RFR: JDK-8299977 : Update WebKit to 615.1 [v2]

[Joeri Sykora]

On Mon, 30 Jan 2023 15:11:20 GMT, Hima Bindu Meda  wrote:

>> Update JavaFX WebKit to GTK WebKit 2.38 (615.1).
>> 
>> Verified the updated version build, sanity tests and stability.
>> This does not cause any issues except one unit test failure.
>> Also, there are some artifacts observed while playing youtube
>> 
>> The above issues are recorded and ignored using below bugs:
>> [JDK-8300954](https://bugs.openjdk.org/browse/JDK-8300954)
>> [JDK-8301022](https://bugs.openjdk.org/browse/JDK-8301022)
>
> Hima Bindu Meda has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Add blank lines

Building and testing succeeded on linux, mac and windows x86_64.

-

Marked as reviewed by sykora (Author).

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

Re: RFR: 8298382: JavaFX ChartArea Accessibility Reader

[Kevin Rushforth]

On Mon, 30 Jan 2023 21:56:45 GMT, Alexander Zuev  wrote:

> Change the underlying class XYChart to take into account labels for axes. 
> Make label patterns and default axes labels localized.

Can you provide an evaluation of the bug and a description of the fix?

-

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

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

[Kevin Rushforth]

On Sun, 29 Jan 2023 17:10:26 GMT, Michael Strauß  wrote:

>> Platform preferences are the preferred UI settings of the operating system. 
>> For example, on Windows this includes the color values identified by the 
>> `Windows.UI.ViewManagement.UIColorType` enumeration; on macOS this includes 
>> the system color values of the `NSColor` class.
>> 
>> Exposing these dynamic values to JavaFX applications allows developers to 
>> create themes that can integrate seamlessly with the color scheme of the 
>> operating system.
>> 
>> Platform preferences are exposed as an `ObservableMap` of platform-specific 
>> key-value pairs, which means that the preferences available on Windows are 
>> different from macOS or Linux. JavaFX provides a small, curated list of 
>> preferences that are available on most platforms, and are therefore exposed 
>> with a platform-independent API:
>> 
>> 
>> public interface Preferences extends ObservableMap {
>> // Platform-independent API
>> ReadOnlyObjectProperty appearanceProperty();
>> ReadOnlyObjectProperty backgroundColorProperty();
>> ReadOnlyObjectProperty foregroundColorProperty();
>> ReadOnlyObjectProperty accentColorProperty();
>> 
>> // Convenience methods to retrieve platform-specific values from the map
>> Optional getInteger(String key);
>> Optional getDouble(String key);
>> Optional getString(String key);
>> Optional getBoolean(String key);
>> Optional getColor(String key);
>> }
>> 
>> 
>> The platform appearance is defined by the new `javafx.stage.Appearance` 
>> enumeration:
>> 
>> 
>> public enum Appearance {
>> LIGHT,
>> DARK
>> }
>> 
>> 
>> An instance of the `Preferences` interface can be retrieved by calling 
>> `Platform.getPreferences()`.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Use Optional for convenience methods in Preferences

It is premature to review this, since we have not had a discussion of whether 
we want such an API in the core of JavaFX. I am moving it back to Draft.

When/if it is ready to be reviewed it will need a CSR.

-

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

Re: RFR: JDK-8223373: Remove IntelliJ IDEA specific files from the source code repository

[Kevin Rushforth]

On Mon, 23 Jan 2023 23:55:51 GMT, Thiago Milczarek Sayao  
wrote:

> This PR does:
> 
> - Remove specific Idea files and let it be imported from gradle;
> - Adds checkstyle (to use with checkstyle plugin - it will let you know style 
> mistakes);
> - Configures auto-format to sun style (with the changes mentioned in [Code 
> Style Rules](https://wiki.openjdk.org/display/OpenJFX/Code+Style+Rules));
> - Automatically sets Copyright notice (updates year too);
> - Run configurations for samples/toys and builds.

This touches `build.gradle` and `settings.gradle` in addition to IDE-specific 
files. Those changes will need additional review, so I'm requesting two 
reviewers.

-

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

Re: RFR: 8088998: LineChart: duplicate child added exception when remove & add a series

[Kevin Rushforth]

On Mon, 30 Jan 2023 12:21:38 GMT, Karthik P K  wrote:

> While checking for duplicate series addition to the line chart, `setToRemove` 
> value was not considered before throwing exception. Hence code to handling 
> the case of adding the removed series was never run.
> 
> Added condition to check `setToRemove` boolean value before throwing 
> exception. Made changes to call `setChart` method after calling 
> `seriesBeingRemovedIsAdded`. Otherwise chart will not be drawn for the 
> series, only points will be plotted.
> 
> This issue is reproducible only when animation is enabled because timeline 
> will be still running when removed series is added back to the same chart. 
> Since timeline does not run in unit tests, added system test to validate the 
> fix.

Reviewers: @aghaisas @andy-goryachev-oracle

-

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

Re: RFR: 8281327: JavaFX does not support fonts installed per-user on Windows 10/11

[Jose Pereda]

On Tue, 31 Jan 2023 16:47:01 GMT, Michael Strauß  wrote:

>> This PR simply applies the patch from 
>> [JDK-8218914](https://bugs.openjdk.org/browse/JDK-8218914) that solved the 
>> same issue for the JDK.
>> 
>> I've tested it on Windows 11 (Version 22H2 Build 22621.1105). 
>> 
>> I have the Roboto font installed under 
>> C:\Users%user\AppData\Local\Microsoft\Windows\Fonts
>> and with this PR, -Dprism.debugfonts shows:
>> 
>> 
>> ...
>> font=segoe ui historic file=seguihis.ttf
>> font=roboto 
>> file=C:\Users%user\AppData\Local\Microsoft\Windows\Fonts\Roboto-Regular.ttf
>> font=yu mincho file=yumin.ttf
>> ...
>> 
>> 
>> Also, I can see that the font is picked in a simple JavaFX application.
>> Without this PR, the font is not used, and defaults to a system one. 
>> 
>> I don't think I can add this as a system/manual test to the PR, though.
>
> modules/javafx.graphics/src/main/native-font/fontpath.c line 572:
> 
>> 570: if (ret != ERROR_SUCCESS ||
>> 571: dwMaxValueNameLen >= MAX_BUFFER ||
>> 572: dwMaxValueDataLen >= MAX_BUFFER) {
> 
> This implementation instantly fails if _any_ value or data exceeds 
> `MAX_BUFFER`. Wouldn't it be better to only skip the values that are too 
> large, instead of ignoring all values entirely?
> 
> Instead of using `RegQueryInfoKeyW`, the 
> [documentation](https://learn.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regenumvaluew)
>  suggests the following approach:
> 
>> To enumerate values, an application should initially call the RegEnumValue 
>> function with the dwIndex parameter set to zero. The application should then 
>> increment dwIndex and call the RegEnumValue function until there are no more 
>> values (until the function returns ERROR_NO_MORE_ITEMS).

Please note that this is just a refactoring of the existing code, extracting a 
method so that it can be called twice, and it is also what the JDK is doing. 
If the affected code needs changes, maybe that is for a follow up.

-

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

Re: [jfx20] RFR: 8300013: Node.focusWithin doesn't account for nested focused nodes [v2]

[Michael Strauß]

On Tue, 31 Jan 2023 17:08:40 GMT, Michael Strauß  wrote:

>> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 8325:
>> 
>>> 8323: } else if (count == 0) {
>>> 8324: set(false);
>>> 8325: }
>> 
>> Is it worth adding a check for `count < 0` and logging a warning (possibly 
>> treating it as if it were 0)? In theory, it shouldn't happen, but if it ever 
>> did, `focusWithin` would be wrong after that. This could be done as a P4 
>> follow-up for 21, unless you are certain that it cannot ever happen.
>
> On the one hand, this might make the code a little bit more robust, but on 
> the other hand, it might hide a bug elsewhere. Surely `count` should never be 
> negative. I lean slightly towards not protecting code against bugs in this 
> way, mostly because it might expose potential bugs sooner.
> 
> If we want to validate that this method is not called with an incorrect 
> argument, maybe just throwing an exception would be better.

I've created a ticket to investigate this: 
https://bugs.openjdk.org/browse/JDK-8301556

-

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

Re: Style themes API

[Pedro Duque Vieira]

Hi Michael,

Truly sorry for the late reply but haven't had time to reply sooner,
unfortunately..

2 -

> > I've been building javafx themes for years now. Started creating JMetro
> in JavaFX version 2. During this time I've been following how other themes
> are being built and I can say that about 90% of the themes (rough estimate)
> that are out there, are composed of author stylesheets, i.e they override
> values set from code (including JMetro).
> > I agree that themes that are to be provided to other developers would
> probably be better off if they are user agent stylesheets. My point is that
> migrating those 90% of themes to use this API would be easier if this flag
> exists otherwise migration would likely introduce UI regressions for
> developers using those themes.
> >
> > To be clear, what I'm proposing is a flag, or enum, or whatever, in
> StyleTheme that defines whether the stylesheets in the StyleTheme should be
> a user agent stylesheet or author stylesheet.
> >
> > Another point is in making themes that are only to be used locally in a
> specific app that the developer is creating. It might be of interest that
> he can toggle this flag and make all settings defined in the theme override
> any styling set from code (or in FXML) so that he can have a centralized
> point where all styling is done.


I think that the likelihood of this feature making it into the 21
> release dramatically increases if we keep changes to the CSS subsystem
> to an absolute minimum. I understand your point, but I want to deliver
> this feature in the smallest useful package and add more features
> later with follow-on PRs.


That's why I've renamed `Application.styleTheme` to
> `Application.userAgentStyleTheme`. This makes it unmistakably clear
> that we're only talking about user-agent stylesheets.
> Also, it leaves open the possibility to add the
> `Application.styleTheme` property later, which would cause the theme
> to be interpreted as comprised of author stylesheets.
> Classifying author/UA themes at the property level is also more
> consistent with the way author/UA stylesheets are currently classified
> in JavaFX.
> What do you think about the styleTheme/userAgentStyleTheme API,
> instead of having a flag in the implementation of StyleTheme itself?


Since one of the objectives of the PR is to promote Caspian and Modena
> to first-class theme implementations, it makes sense to focus on UA
> themes first (since built-in themes are comprised of UA stylesheets).


OK.
Thinking of existing themes migrating to this StyleTheme API (90% are
implemented as author stylesheets), some visual glitches may start
appearing for users of those themes once they start using the migrated
versions (since they will go from being author stylesheets to user agent
stylesheets).
So, 2 things to think about:
2.1 - How important is it that we think about this use case: existing
themes migrating to this new API. I'd say we should probably think about it
when designing this new API.
2.2 - The second thing then is, how problematic will it be for programmers
using these themes to have their apps start having visual glitches because
of these themes changing from author stylesheets to user agent style
sheets?
Perhaps we simply don't bother with this last point and simply have it be
the price of innovation. And so programmers will have to adapt their
applications and remove any code that is overriding the styles defined in
the StyleThemes (which are likely causing these visual glitches when
migrating to Themes using this new StyleTheme API). The same for any styles
already defined in custom author stylesheets that are being used by the
application and that might be NOW overriding the styles of the theme.


3 -
> Style themes are generally either defined as dark or light. i.e. you'll
very rarely have a theme that has some Windows with dark decorations and
others with light decorations. So I think it makes sense to have this as a
theme property.
> The ability to toggle decorations (light or dark) on an individual Window
is a good point you're making. Perhaps we should have a global definition
in the StyleTheme (whether the theme is light or dark) and a definition
that can be set per Window.
> If you set it on the StyleTheme then all Windows will respect it, but you
can override that value on each Window. If you override it on a specific
Window, that definition would take precedence over the global StyleTheme
one.

In general, StyleTheme is a very simple concept: it's a dynamic
> collection of stylesheets.
> I don't think the interface should force implementers to deal with
> anything other than stylesheets. If we did that, the following code
> wouldn't work any more:


Application.setUserAgentStyleTheme(
> () -> List.of("stylesheet1.css", "stylesheet2.css");


Maybe we can add a new interface, for example DarkModeAware, and if an
> application theme is marked with this interface, JavaFX will try to
> respect dark/light mode for p

Re: [jfx20] RFR: 8300013: Node.focusWithin doesn't account for nested focused nodes [v2]

[Michael Strauß]

On Tue, 31 Jan 2023 16:42:32 GMT, Kevin Rushforth  wrote:

>> Michael Strauß has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   refactoring
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 8325:
> 
>> 8323: } else if (count == 0) {
>> 8324: set(false);
>> 8325: }
> 
> Is it worth adding a check for `count < 0` and logging a warning (possibly 
> treating it as if it were 0)? In theory, it shouldn't happen, but if it ever 
> did, `focusWithin` would be wrong after that. This could be done as a P4 
> follow-up for 21, unless you are certain that it cannot ever happen.

On the one hand, this might make the code a little bit more robust, but on the 
other hand, it might hide a bug elsewhere. Surely `count` should never be 
negative. I lean slightly towards not protecting code against bugs in this way, 
mostly because it might expose potential bugs sooner.

If we want to validate that this method is not called with an incorrect 
argument, maybe just throwing an exception would be better.

-

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

[jfx20] Integrated: 8300013: Node.focusWithin doesn't account for nested focused nodes

[Michael Strauß]

On Thu, 12 Jan 2023 03:08:30 GMT, Michael Strauß  wrote:

> When a scene graph contains multiple nested focused nodes (this can happen 
> with `TableView` and other controls), the `focusWithin` bits that are cleared 
> when a focused node is de-focused must only be cleared when there is no other 
> nested node in the scene graph that would also cause `focusWithin` to be set.
> 
> For example, consider a scene graph of nested nodes:
> A -> B -> C -> D
> 
> When B and D are both focused, the scene graph looks like this:
> A(`focusWithin`)
> -> B(`focused`, `focusWithin`)
> -> C(`focusWithin`)
> -> D(`focused`, `focusWithin`)
> 
> When B is de-focused, the `focusWithin` flags must still be preserved because 
> D remains focused.
> 
> This PR fixes the issue by counting the number of times `focusWithin` has 
> been "set", and only clears it when it has been "un-set" an equal number of 
> times.

This pull request has now been integrated.

Changeset: a4bc9d1a
Author:Michael Strauß 
URL:   
https://git.openjdk.org/jfx/commit/a4bc9d1a69e56cab92d3dc34cfff49c5cb524443
Stats: 89 lines in 2 files changed: 73 ins; 0 del; 16 mod

8300013: Node.focusWithin doesn't account for nested focused nodes

Reviewed-by: aghaisas, kcr

-

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

Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0

[Nir Lisker]

On Tue, 31 Jan 2023 14:40:00 GMT, Karthik P K  wrote:

> In `selectIndices` method, zero length array is not considered while ignoring 
> row number given as parameter.
> 
> Updated the code to consider both null and zero length array in the condition 
> before ignoring the row value given as parameter.
> 
> Added unit test to validate the fix

I recently looked at https://bugs.openjdk.org/browse/JDK-8120635. Is it 
related? I managed to reproduce it, so I think it should be reopened.

-

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

Re: [jfx20] RFR: 8300013: Node.focusWithin doesn't account for nested focused nodes [v2]

[Kevin Rushforth]

On Fri, 13 Jan 2023 04:04:54 GMT, Michael Strauß  wrote:

>> When a scene graph contains multiple nested focused nodes (this can happen 
>> with `TableView` and other controls), the `focusWithin` bits that are 
>> cleared when a focused node is de-focused must only be cleared when there is 
>> no other nested node in the scene graph that would also cause `focusWithin` 
>> to be set.
>> 
>> For example, consider a scene graph of nested nodes:
>> A -> B -> C -> D
>> 
>> When B and D are both focused, the scene graph looks like this:
>> A(`focusWithin`)
>> -> B(`focused`, `focusWithin`)
>> -> C(`focusWithin`)
>> -> D(`focused`, `focusWithin`)
>> 
>> When B is de-focused, the `focusWithin` flags must still be preserved 
>> because D remains focused.
>> 
>> This PR fixes the issue by counting the number of times `focusWithin` has 
>> been "set", and only clears it when it has been "un-set" an equal number of 
>> times.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   refactoring

Looks good. Approved for `jfx20`.

I left a couple questions, one of which might warrant a follow-up issue 
depending on your answer.

modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 8321:

> 8319: count += change;
> 8320: 
> 8321: if (count == 1) {

This presumes that you never call this with `change > 1`. You don't, so it 
seems fine.

modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 8325:

> 8323: } else if (count == 0) {
> 8324: set(false);
> 8325: }

Is it worth adding a check for `count < 0` and logging a warning (possibly 
treating it as if it were 0)? In theory, it shouldn't happen, but if it ever 
did, `focusWithin` would be wrong after that. This could be done as a P4 
follow-up for 21, unless you are certain that it cannot ever happen.

-

Marked as reviewed by kcr (Lead).

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

Re: RFR: 8281327: JavaFX does not support fonts installed per-user on Windows 10/11

[Michael Strauß]

On Tue, 31 Jan 2023 10:30:22 GMT, Jose Pereda  wrote:

> This PR simply applies the patch from 
> [JDK-8218914](https://bugs.openjdk.org/browse/JDK-8218914) that solved the 
> same issue for the JDK.
> 
> I've tested it on Windows 11 (Version 22H2 Build 22621.1105). 
> 
> I have the Roboto font installed under 
> C:\Users%user\AppData\Local\Microsoft\Windows\Fonts
> and with this PR, -Dprism.debugfonts shows:
> 
> 
> ...
> font=segoe ui historic file=seguihis.ttf
> font=roboto 
> file=C:\Users%user\AppData\Local\Microsoft\Windows\Fonts\Roboto-Regular.ttf
> font=yu mincho file=yumin.ttf
> ...
> 
> 
> Also, I can see that the font is picked in a simple JavaFX application.
> Without this PR, the font is not used, and defaults to a system one. 
> 
> I don't think I can add this as a system/manual test to the PR, though.

modules/javafx.graphics/src/main/native-font/fontpath.c line 572:

> 570: if (ret != ERROR_SUCCESS ||
> 571: dwMaxValueNameLen >= MAX_BUFFER ||
> 572: dwMaxValueDataLen >= MAX_BUFFER) {

This implementation instantly fails if _any_ value or data exceeds 
`MAX_BUFFER`. Wouldn't it be better to only skip the values that are too large, 
instead of ignoring all values entirely?

Instead of using `RegQueryInfoKeyW`, the 
[documentation](https://learn.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regenumvaluew)
 suggests the following approach:

> To enumerate values, an application should initially call the RegEnumValue 
> function with the dwIndex parameter set to zero. The application should then 
> increment dwIndex and call the RegEnumValue function until there are no more 
> values (until the function returns ERROR_NO_MORE_ITEMS).

-

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

Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0

[Andy Goryachev]

On Tue, 31 Jan 2023 14:40:00 GMT, Karthik P K  wrote:

> In `selectIndices` method, zero length array is not considered while ignoring 
> row number given as parameter.
> 
> Updated the code to consider both null and zero length array in the condition 
> before ignoring the row value given as parameter.
> 
> Added unit test to validate the fix

I wonder if there are other places like this elsewhere in the code.
This change looks good.

-

Marked as reviewed by angorya (Committer).

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

Re: Toolkit decorations experiment

[Dirk Lemmermann]

Since you are venturing into this space …. would love to be able to create 
blurred backgrounds in windows. Apple has some apps where the sidebar is 
semi-transparent with a strong blur effect.

> On 31 Jan 2023, at 15:15, Thiago Milczarek Sayão  
> wrote:
> 
> Hi,
> 
> I'm doing some experiments with toolkit decorations (instead of platform 
> decorations).
> 
> The goal is to allow for modern-looking apps, with "hamburger buttons" or 
> tabs (like firefox or chrome) on the decoration space.
> 
> It is coming into shape (nowhere near finished). It's CSS styleable.
> 
> Source:
> https://github.com/tsayao/jfx/tree/toolkit_decoration
> To run:
> java @build/run.args tests/manual/controls/SceneDecorationTest.java
> 
> It would depend on a window move + resize API sketched here:
> https://github.com/openjdk/jfx/pull/1013
> 
> -- Thiago

[jfx11u] Integrated: 8301010: Change JavaFX release version to 11.0.19 in jfx11u

[Johan Vos]

On Thu, 26 Jan 2023 15:31:52 GMT, Johan Vos  wrote:

> Bump release version to 11.0.19 in build.properties and .jcheck/conf
> Fix for JDK-8301010

This pull request has now been integrated.

Changeset: 6a31ceba
Author:Johan Vos 
URL:   
https://git.openjdk.org/jfx11u/commit/6a31ceba4a95bcc7766e396a6509a43e35c59392
Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod

8301010: Change JavaFX release version to 11.0.19 in jfx11u

Reviewed-by: kcr

-

PR: https://git.openjdk.org/jfx11u/pull/125

[jfx17u] Integrated: 8301011: Change JavaFX release version to 17.0.7 in jfx17u

[Johan Vos]

On Thu, 26 Jan 2023 15:25:32 GMT, Johan Vos  wrote:

> increase release version in build.properties and jcheck configuration
> Fix for JDK-8301011

This pull request has now been integrated.

Changeset: 27fac395
Author:Johan Vos 
URL:   
https://git.openjdk.org/jfx17u/commit/27fac39560bd146cce90fbe3f83b50066698bc7e
Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod

8301011: Change JavaFX release version to 17.0.7 in jfx17u

Reviewed-by: kcr

-

PR: https://git.openjdk.org/jfx17u/pull/104

RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0

[Karthik P K]

In `selectIndices` method, zero length array is not considered while ignoring 
row number given as parameter.

Updated the code to consider both null and zero length array in the condition 
before ignoring the row value given as parameter.

Added unit test to validate the fix

-

Commit messages:
 - TableView row selection issue fix

Changes: https://git.openjdk.org/jfx/pull/1018/files
 Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1018&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8138842
  Stats: 15 lines in 2 files changed: 14 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jfx/pull/1018.diff
  Fetch: git fetch https://git.openjdk.org/jfx pull/1018/head:pull/1018

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

Toolkit decorations experiment

[Thiago Milczarek Sayão]

Hi,

I'm doing some experiments with toolkit decorations (instead of platform
decorations).

The goal is to allow for modern-looking apps, with "hamburger buttons" or
tabs (like firefox or chrome) on the decoration space.

It is coming into shape (nowhere near finished). It's CSS styleable.

Source:
https://github.com/tsayao/jfx/tree/toolkit_decoration
To run:
java @build/run.args tests/manual/controls/SceneDecorationTest.java

It would depend on a window move + resize API sketched here:
https://github.com/openjdk/jfx/pull/1013

-- Thiago

Re: [jfx17u] RFR: 8293375: add_definitions USE_SYSTEM_MALLOC when USE_SYSTEM_MALLOC is ON

[Kevin Rushforth]

On Mon, 9 Jan 2023 13:29:27 GMT, Ao Qi  wrote:

> Clean backport. Verified on (after #102):
> - Linux/LoongArch64 (`USE_SYSTEM_MALLOC` is `on`, the platform triggering 
> this problem)
> - Linux/x64 (`USE_SYSTEM_MALLOC` is `off`)
> - Linux/aarch64 (`USE_SYSTEM_MALLOC` is `off`)

This needs to wait until PR #104 is integrated (else the fix version will be 
wrong).

@johanvos can then comment on this. I will note that this backport is on my 
list of things that should be done to keep the native WebKit in sync with 
mainline.

-

PR: https://git.openjdk.org/jfx17u/pull/103

[jfx17u] RFR: 8293375: add_definitions USE_SYSTEM_MALLOC when USE_SYSTEM_MALLOC is ON

[Ao Qi]

Clean backport. Verified on (after #102):
- Linux/LoongArch64 (`USE_SYSTEM_MALLOC` is `on`, the platform triggering this 
problem)
- Linux/x64 (`USE_SYSTEM_MALLOC` is `off`)
- Linux/aarch64 (`USE_SYSTEM_MALLOC` is `off`)

-

Commit messages:
 - 8293375: add_definitions USE_SYSTEM_MALLOC when USE_SYSTEM_MALLOC is ON

Changes: https://git.openjdk.org/jfx17u/pull/103/files
 Webrev: https://webrevs.openjdk.org/?repo=jfx17u&pr=103&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8293375
  Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jfx17u/pull/103.diff
  Fetch: git fetch https://git.openjdk.org/jfx17u pull/103/head:pull/103

PR: https://git.openjdk.org/jfx17u/pull/103

Re: [jfx17u] RFR: 8293375: add_definitions USE_SYSTEM_MALLOC when USE_SYSTEM_MALLOC is ON

[Ao Qi]

On Mon, 9 Jan 2023 13:29:27 GMT, Ao Qi  wrote:

> Clean backport. Verified on (after #102):
> - Linux/LoongArch64 (`USE_SYSTEM_MALLOC` is `on`, the platform triggering 
> this problem)
> - Linux/x64 (`USE_SYSTEM_MALLOC` is `off`)
> - Linux/aarch64 (`USE_SYSTEM_MALLOC` is `off`)

RFC and RFR. Thanks.

-

PR: https://git.openjdk.org/jfx17u/pull/103

Re: [jfx11u] RFR: 8301010: Change JavaFX release version to 11.0.19 in jfx11u

[Kevin Rushforth]

On Thu, 26 Jan 2023 15:31:52 GMT, Johan Vos  wrote:

> Bump release version to 11.0.19 in build.properties and .jcheck/conf
> Fix for JDK-8301010

Marked as reviewed by kcr (Lead).

-

PR: https://git.openjdk.org/jfx11u/pull/125

Re: [jfx17u] RFR: 8301011: Change JavaFX release version to 17.0.7 in jfx17u

[Kevin Rushforth]

On Thu, 26 Jan 2023 15:25:32 GMT, Johan Vos  wrote:

> increase release version in build.properties and jcheck configuration
> Fix for JDK-8301011

Marked as reviewed by kcr (Lead).

-

PR: https://git.openjdk.org/jfx17u/pull/104

Re: RFR: 8281327: JavaFX does not support fonts installed per-user on Windows 10/11

[Glavo]

On Tue, 31 Jan 2023 10:30:22 GMT, Jose Pereda  wrote:

> This PR simply applies the patch from 
> [JDK-8218914](https://bugs.openjdk.org/browse/JDK-8218914) that solved the 
> same issue for the JDK.
> 
> I've tested it on Windows 11 (Version 22H2 Build 22621.1105). 
> 
> I have the Roboto font installed under 
> C:\Users%user\AppData\Local\Microsoft\Windows\Fonts
> and with this PR, -Dprism.debugfonts shows:
> 
> 
> ...
> font=segoe ui historic file=seguihis.ttf
> font=roboto 
> file=C:\Users%user\AppData\Local\Microsoft\Windows\Fonts\Roboto-Regular.ttf
> font=yu mincho file=yumin.ttf
> ...
> 
> 
> Also, I can see that the font is picked in a simple JavaFX application.
> Without this PR, the font is not used, and defaults to a system one. 
> 
> I don't think I can add this as a system/manual test to the PR, though.

That is great! We have been looking forward to this patch for two years.

-

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

RFR: 8281327: JavaFX does not support fonts installed per-user on Windows 10/11

[Jose Pereda]

This PR simply applies the patch from 
[JDK-8218914](https://bugs.openjdk.org/browse/JDK-8218914) that solved the same 
issue for the JDK.

I've tested it on Windows 11 (Version 22H2 Build 22621.1105). 

I have the Roboto font installed under 
C:\Users%user\AppData\Local\Microsoft\Windows\Fonts
and with this PR, -Dprism.debugfonts shows:


...
font=segoe ui historic file=seguihis.ttf
font=roboto 
file=C:\Users%user\AppData\Local\Microsoft\Windows\Fonts\Roboto-Regular.ttf
font=yu mincho file=yumin.ttf
...


Also, I can see that the font is picked in a simple JavaFX application.
Without this PR, the font is not used, and defaults to a system one. 

I don't think I can add this as a system/manual test to the PR, though.

-

Commit messages:
 - Apply patch from JDK-8218914

Changes: https://git.openjdk.org/jfx/pull/1017/files
 Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1017&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8281327
  Stats: 113 lines in 1 file changed: 63 ins; 43 del; 7 mod
  Patch: https://git.openjdk.org/jfx/pull/1017.diff
  Fetch: git fetch https://git.openjdk.org/jfx pull/1017/head:pull/1017

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