Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v7]
On Wed, 7 Aug 2024 02:13:12 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 incrementally with one additional
> commit since the last revision:
>
> Fix bug
modules/javafx.graphics/src/main/java/javafx/css/Match.java line 90:
> 88: private final int specificity;
> 89:
> 90: @SuppressWarnings("removal")
What is this removal warning suppression? I know it was there before these
changes.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1715163301
Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v7]
On Wed, 7 Aug 2024 02:13:12 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 incrementally with one additional > commit since the last revision: > > Fix bug I see a lot of aversion from for-each loops in favor of indexed loops in the classes this PR touches. While I don't suggest to change it here, any idea why? - PR Comment: https://git.openjdk.org/jfx/pull/1333#issuecomment-2286067091
Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v7]
On Tue, 13 Aug 2024 11:54:16 GMT, Nir Lisker wrote: > I see a lot of aversion from for-each loops in favor of indexed loops in the > classes this PR touches. While I don't suggest to change it here, any idea > why? It's just the style the original code was written with. There is no reason to write the binary load/save code like this (as IO will be a far greater bottleneck). There are/were other parts (in `Match` and the selector subtypes) however that are performance sensitive, so perhaps the entire classes were just written in a similar style "just in case". - PR Comment: https://git.openjdk.org/jfx/pull/1333#issuecomment-2286078330
Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v7]
On Tue, 13 Aug 2024 11:51:39 GMT, Nir Lisker wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix bug
>
> modules/javafx.graphics/src/main/java/javafx/css/Match.java line 90:
>
>> 88: private final int specificity;
>> 89:
>> 90: @SuppressWarnings("removal")
>
> What is this removal warning suppression? I know it was there before these
> changes.
`SimpleSelector` was deprecated for removal (from the public javafx.css
package). Now that it is moved to the internal package, the deprecation on that
was also removed. The `SuppressWarnings` is therefore no longer needed here as
the deprecation is no longer present on the now internal `SimpleSelector` type.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1715180348
Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v7]
On Tue, 13 Aug 2024 12:05:18 GMT, John Hendrikx wrote:
>> modules/javafx.graphics/src/main/java/javafx/css/Match.java line 90:
>>
>>> 88: private final int specificity;
>>> 89:
>>> 90: @SuppressWarnings("removal")
>>
>> What is this removal warning suppression? I know it was there before these
>> changes.
>
> `SimpleSelector` was deprecated for removal (from the public javafx.css
> package). Now that it is moved to the internal package, the deprecation on
> that was also removed. The `SuppressWarnings` is therefore no longer needed
> here as the deprecation is no longer present on the now internal
> `SimpleSelector` type.
The diff seems to show I did not remove that line, but it should no longer be
needed. I'll check.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1715180851
Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v8]
> 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 two additional commits since the last revision: - Fix javadoc - Remove unnecessary suppress warnings - Changes: - all: https://git.openjdk.org/jfx/pull/1333/files - new: https://git.openjdk.org/jfx/pull/1333/files/39c94408..875c1a88 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1333&range=07 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1333&range=06-07 Stats: 2 lines in 2 files changed: 0 ins; 1 del; 1 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: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v7]
On Wed, 7 Aug 2024 22:31:57 GMT, Andy Goryachev wrote: >> John Hendrikx has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix bug > > modules/javafx.graphics/src/main/java/com/sun/javafx/css/CompoundSelector.java > line 75: > >> 73: /** >> 74: * The relationships between the selectors >> 75: * @return Immutable List> > minor: would {@code List} be a better choice here, if it works? Fixed - PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1715189665
Re: RFR: 8336938: Update libFFI to 3.4.6
On Tue, 13 Aug 2024 01:03:32 GMT, Alexander Matveev wrote: > Yes, it compiles fine. It was changed by libFFI itself in 3.4.6. I think to > be inline with rest of comments in this file. What I meant was that you missed making that change when you updated the file. The old line with the inline comment style is still in our copy of the file. So it no longer matches the upstream libffi 3.4.6. It seems best to apply that part of the patch so that we will match the upstream version of the file. - PR Comment: https://git.openjdk.org/jfx/pull/1531#issuecomment-2286144930
Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v7]
On Wed, 7 Aug 2024 22:47:28 GMT, Andy Goryachev wrote: > oops, yes, you did, my mistake. the master progressed since then, got me > confused. > > I got a bunch of errors again after switching: > > ``` > Description ResourceTypePathLocation > Cannot infer type arguments for ParsedValueImpl<> CssParser.java Java > Problem/graphics/src/main/java/javafx/css line 3024 > Cannot infer type arguments for ParsedValueImpl<> CssParser.java Java > Problem/graphics/src/main/java/javafx/css line 3214 > Cannot infer type arguments for ParsedValueImpl<> CssParser.java Java > Problem/graphics/src/main/java/javafx/css line 3267 > Cannot infer type arguments for ParsedValueImpl<> CssParser.java Java > Problem/graphics/src/main/java/javafx/css line 3282 > Cannot infer type arguments for ParsedValueImpl<> CssParser.java Java > Problem/graphics/src/main/java/javafx/css line 3607 > Cannot infer type arguments for ParsedValueImpl<> CssParser.java Java > Problem/graphics/src/main/java/javafx/css line 3651 > ``` I got these too, as well as many The method createShader(String, InputStream, Map, Map, int, boolean, boolean) in the type ShaderFactory is not applicable for the arguments (InputStream, HashMap, HashMap, int, boolean, boolean) I haven't worked on this repo for 3 weeks, what changed? - PR Comment: https://git.openjdk.org/jfx/pull/1333#issuecomment-2286360227
Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v7]
On Tue, 13 Aug 2024 14:10:52 GMT, Nir Lisker wrote: > > oops, yes, you did, my mistake. the master progressed since then, got me > > confused. > > I got a bunch of errors again after switching: > > ``` > > Description ResourceTypePathLocation > > Cannot infer type arguments for ParsedValueImpl<> CssParser.java Java > > Problem/graphics/src/main/java/javafx/css line 3024 > > Cannot infer type arguments for ParsedValueImpl<> CssParser.java Java > > Problem/graphics/src/main/java/javafx/css line 3214 > > Cannot infer type arguments for ParsedValueImpl<> CssParser.java Java > > Problem/graphics/src/main/java/javafx/css line 3267 > > Cannot infer type arguments for ParsedValueImpl<> CssParser.java Java > > Problem/graphics/src/main/java/javafx/css line 3282 > > Cannot infer type arguments for ParsedValueImpl<> CssParser.java Java > > Problem/graphics/src/main/java/javafx/css line 3607 > > Cannot infer type arguments for ParsedValueImpl<> CssParser.java Java > > Problem/graphics/src/main/java/javafx/css line 3651 > > ``` > > I got these too, as well as many > > ``` > The method createShader(String, InputStream, Map, > Map, int, boolean, boolean) in the type ShaderFactory is not > applicable for the arguments (InputStream, HashMap, > HashMap, int, boolean, boolean) > ``` > > I haven't worked on this repo for 3 weeks, what changed? The ones mentioned by Andy I also see, and are just a difference in opinion between Javac and ECJ, or perhaps a bug in the incremental compilation that Eclipse does (as they usually only appear when it does an incremental compile, not a full one). When those occur, I just select them and remove them from the errors list. The others I haven't seen before, and I'm not seeing them now. I did a search for `createShader` but didn't see anything unusual. I'm not aware of any special changes in the last 3 weeks. I did a fresh import recently, and nothing strange popped up. - PR Comment: https://git.openjdk.org/jfx/pull/1333#issuecomment-2286377890
Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v7]
On Tue, 13 Aug 2024 14:18:39 GMT, John Hendrikx wrote: > ``` > The method createShader(String, InputStream, Map, > Map, int, boolean, boolean) in the type ShaderFactory is not > applicable for the arguments (InputStream, HashMap, > HashMap, int, boolean, boolean) > ``` > > I haven't worked on this repo for 3 weeks, what changed? You need to do a clean build. This method was changed by [JDK-8336624](https://bugs.openjdk.org/browse/JDK-8336624) / PR #1530 - PR Comment: https://git.openjdk.org/jfx/pull/1333#issuecomment-2286427951
Re: RFR: 8289174: JavaFX build fails on Windows when VS150COMNTOOLS is not set [v3]
On Mon, 12 Aug 2024 21:31:49 GMT, Kevin Rushforth wrote:
>> 1. Added logic to look in the standard locations for Visual Studio 2022,
>> with a fallback to 2019 and then 2017 if 2022 is not found. It will look in
>> the following locations:
>>
>> C:("Program Files"|"Program Files (x86)")\Microsoft Visual
>> Studio(2022|2019|2017)\
>> (Enterprise|Professional|Community)\VC\Auxiliary\Build
>>
>> 2. Use `VSCOMNTOOLS` as the primary way for a developer to specify a custom
>> location for Visual Studio, with `VS150COMNTOOLS` as a legacy fallback (a
>> warning is printed if `VS150COMNTOOLS` is set). If a developer installs
>> Visual Studio in the default location, there will be no need to set this any
>> more.
>> 3. Removed support for older compilers (VS2010 and VS2013 in particular),
>> since they will no longer work anyway. We were still using the 32-bit
>> version of the script in many cases, which is unnecessary and no longer
>> makes sense, so as part of this cleanup, I now consistently use
>> `vcvars64.bat` to set up the environment.
>> 4. Modified the GitHub actions build to no longer specify `VS150COMNTOOLS`,
>> since it will now find it automatically.
>> 5. Removed the following unused variables:
>>
>> DEVENVCMD
>> DXSDK_DIR
>> VS_VER
>>
>> 6. Changed most of the defaults in `win.gradle` to the empty string. The
>> defaults were a misguided attempt to set values to something in case the
>> Visual Studio installation cannot be found. By not having defaults, it will
>> be more obvious if something goes wrong (and will fail fast).
>> 7. If the Visual Studio Installation cannot be found, it will print a
>> sensible error message and retry the next time the build is run (making it
>> less likely that a manual `rm -rf build` is needed).
>> 8. Fixed a bug where the Microsoft redist files were not being located and
>> copied into the build dir (build/sdk/bin).
>>
>>
>> I left some debug print statements in, and will remove them in a follow-on
>> commit.
>>
>> I have tested this with a local installation of Visual Studio 2022 Community
>> and, via GitHub Actions, an installation of Visual Studio 2022 Enterprise.
>> On my local system, I built with and without Media and WebKit.
>
> Kevin Rushforth has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Review comments
Continuing from
https://github.com/openjdk/jfx/pull/1333#issuecomment-2286427951, I updated my
VS and did a clean build on `master` and got
> java.util.concurrent.ExecutionException:
> org.gradle.process.internal.ExecException: A problem occurred starting
> process 'command 'C:/Program Files/Microsoft Visual
> Studio/2022/Community/VC/Tools/MSVC/14.33.31629/bin/Hostx64/x64/cl.exe''
After applying this patch, my clean build was successful (up to the webkit
part). I then removed the `VS150COMNTOOLS` env var and tried another clean
build and it worked as well.
Tested on Windows 10.
-
Marked as reviewed by nlisker (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1534#pullrequestreview-2235891455
Re: RFR: 8336938: Update libFFI to 3.4.6 [v2]
> - libFFI updated to 3.4.6. > - No additional changes are done. > - Tested on Windows, macOS and Linux with all supported formats. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8336938: Update libFFI to 3.4.6 [v2] - Changes: - all: https://git.openjdk.org/jfx/pull/1531/files - new: https://git.openjdk.org/jfx/pull/1531/files/e51e5c71..c4a9d5b2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1531&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1531&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1531.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1531/head:pull/1531 PR: https://git.openjdk.org/jfx/pull/1531
Re: RFR: 8336938: Update libFFI to 3.4.6 [v2]
On Tue, 13 Aug 2024 19:04:09 GMT, Alexander Matveev wrote: >> - libFFI updated to 3.4.6. >> - No additional changes are done. >> - Tested on Windows, macOS and Linux with all supported formats. > > Alexander Matveev has updated the pull request incrementally with one > additional commit since the last revision: > > 8336938: Update libFFI to 3.4.6 [v2] Code changes look good. Testing looks good. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1531#pullrequestreview-2236361557
Re: RFR: 8289174: JavaFX build fails on Windows when VS150COMNTOOLS is not set [v3]
On Mon, 12 Aug 2024 21:31:49 GMT, Kevin Rushforth wrote:
>> 1. Added logic to look in the standard locations for Visual Studio 2022,
>> with a fallback to 2019 and then 2017 if 2022 is not found. It will look in
>> the following locations:
>>
>> C:("Program Files"|"Program Files (x86)")\Microsoft Visual
>> Studio(2022|2019|2017)\
>> (Enterprise|Professional|Community)\VC\Auxiliary\Build
>>
>> 2. Use `VSCOMNTOOLS` as the primary way for a developer to specify a custom
>> location for Visual Studio, with `VS150COMNTOOLS` as a legacy fallback (a
>> warning is printed if `VS150COMNTOOLS` is set). If a developer installs
>> Visual Studio in the default location, there will be no need to set this any
>> more.
>> 3. Removed support for older compilers (VS2010 and VS2013 in particular),
>> since they will no longer work anyway. We were still using the 32-bit
>> version of the script in many cases, which is unnecessary and no longer
>> makes sense, so as part of this cleanup, I now consistently use
>> `vcvars64.bat` to set up the environment.
>> 4. Modified the GitHub actions build to no longer specify `VS150COMNTOOLS`,
>> since it will now find it automatically.
>> 5. Removed the following unused variables:
>>
>> DEVENVCMD
>> DXSDK_DIR
>> VS_VER
>>
>> 6. Changed most of the defaults in `win.gradle` to the empty string. The
>> defaults were a misguided attempt to set values to something in case the
>> Visual Studio installation cannot be found. By not having defaults, it will
>> be more obvious if something goes wrong (and will fail fast).
>> 7. If the Visual Studio Installation cannot be found, it will print a
>> sensible error message and retry the next time the build is run (making it
>> less likely that a manual `rm -rf build` is needed).
>> 8. Fixed a bug where the Microsoft redist files were not being located and
>> copied into the build dir (build/sdk/bin).
>>
>>
>> I left some debug print statements in, and will remove them in a follow-on
>> commit.
>>
>> I have tested this with a local installation of Visual Studio 2022 Community
>> and, via GitHub Actions, an installation of Visual Studio 2022 Enterprise.
>> On my local system, I built with and without Media and WebKit.
>
> Kevin Rushforth has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Review comments
Changes look good and also worked for me.
Removed the `VS150COMNTOOLS` variable and rebuild the project.
EDIT: Tested on Windows 11.
-
Marked as reviewed by mhanl (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1534#pullrequestreview-2236391549
Re: RFR: 8334900: IOOBE when adding data to a Series of a BarChart that already contains data [v4]
On Sat, 10 Aug 2024 16:21:11 GMT, Markus Mack wrote: >> This PR is a fix for another IOOBE that I discovered while working on #1476. >> >> The PR simplifies the code for adding a series that already contains data by >> adding the data points one-by-one. >> As far as I can see no attempt was previously made to optimize the bulk >> operation except for some trivial O(1) operations, so this should have no >> noticable performance impact. >> >> Accidentally this fixes another bug related to the missing "negative" style >> class when negative data values are added. >> >> Also, the PR aligns the handling of duplicate categories with the behavior >> clarified in #1476, when there are duplicates in the data that was already >> in the series before the series was added to the chart. >> >> Note a change was made to the createTestSeries() test method, letting it >> start at index 1, avoiding the duplicate data items resulting from >> multiplying by 0. >> Without this change `testSeriesRemoveAnimatedStyleClasses` would fail >> because it counts the number of plot children, where the duplicates are now >> removed. > > Markus Mack has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains six commits: > > - test "negative" style class in BarChartTest tests > - Merge remote-tracking branch 'refs/remotes/origin/master' into > fixes/bar-chart-add-nonempty-series > - fix "negative" style class when series is changed > - Merge remote-tracking branch 'refs/remotes/origin/master' into > fixes/bar-chart-add-nonempty-series > ># Conflicts: ># > modules/javafx.controls/src/test/java/test/javafx/scene/chart/BarChartTest.java > - BarChart: Fix adding non-empty series > - BarChart: Add styleClass "negative" for added data thank you for making the changes! looks good now. - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1488#pullrequestreview-2236574603
Re: RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v8]
On Tue, 13 Aug 2024 12:18:12 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 incrementally with two additional > commits since the last revision: > > - Fix javadoc > - Remove unnecessary suppress warnings Marked as reviewed by angorya (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1333#pullrequestreview-2236600499
Re: RFR: 8337246: SpinnerSkin does not consume ENTER KeyEvent when editor ActionEvent is consumed [v2]
> Enable backpropagation of `isConsumed` flag to the ancestor(s) of events > cloned via `Event.copyFor()`. > > This change has a minimal API impact and allows for a fine-grained control of > when to propagate and when not to propagate. > > The proposed change could make > [JDK-8303209](https://bugs.openjdk.org/browse/JDK-8303209) unnecessary. Andy Goryachev 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 10 additional commits since the last revision: - avoid csr - Merge remote-tracking branch 'origin/master' into ag.consume - only if consumed - only when consumed - propagate - cleanup - simpler - event helper - copy for test - propagate consume action - Changes: - all: https://git.openjdk.org/jfx/pull/1523/files - new: https://git.openjdk.org/jfx/pull/1523/files/551ed976..48022d6b Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1523&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1523&range=00-01 Stats: 1379 lines in 29 files changed: 1135 ins; 90 del; 154 mod Patch: https://git.openjdk.org/jfx/pull/1523.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1523/head:pull/1523 PR: https://git.openjdk.org/jfx/pull/1523
Re: RFR: 8337246: SpinnerSkin does not consume ENTER KeyEvent when editor ActionEvent is consumed
On Fri, 9 Aug 2024 20:16:29 GMT, Martin Fox wrote: > An EventDispatcher that creates a new event without linking it to the > original will break this PR. But that's a consequence you're creating. You > can't say the developer is making a choice about event linking when the very > notion is new with this PR. Actually, this statement is not correct: the consumed flag will be propagated, but only if `propagateConsume` was set by the helper, which is being done in a very specific case. - PR Comment: https://git.openjdk.org/jfx/pull/1523#issuecomment-2287186541
Re: RFR: 8337246: SpinnerSkin does not consume ENTER KeyEvent when editor ActionEvent is consumed [v2]
On Tue, 13 Aug 2024 21:35:11 GMT, Andy Goryachev wrote: >> Enable backpropagation of `isConsumed` flag to the ancestor(s) of events >> cloned via `Event.copyFor()`. >> >> This change has a minimal API impact and allows for a fine-grained control >> of when to propagate and when not to propagate. >> >> The proposed change could make >> [JDK-8303209](https://bugs.openjdk.org/browse/JDK-8303209) unnecessary. > > Andy Goryachev 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 10 additional > commits since the last revision: > > - avoid csr > - Merge remote-tracking branch 'origin/master' into ag.consume > - only if consumed > - only when consumed > - propagate > - cleanup > - simpler > - event helper > - copy for test > - propagate consume action Removed 'final' keyword from existing public APIs. The changes are limited to TextFieldBehavior and SpinnerSkin. - PR Comment: https://git.openjdk.org/jfx/pull/1523#issuecomment-2287183307
Re: RFR: 8305418: [Linux] Replace obsolete XIM as Input Method Editor [v22]
On Sun, 9 Jun 2024 12:56:32 GMT, Thiago Milczarek Sayao wrote: >> This replaces obsolete XIM and uses gtk api for IME. >> Gtk uses [ibus](https://github.com/ibus/ibus) >> >> Gtk3+ uses relative positioning (as Wayland does), so I've added a Relative >> positioning on `InputMethodRequest`. >> >> [Screencast from 17-09-2023 >> 21:59:04.webm](https://github.com/openjdk/jfx/assets/30704286/6c398e39-55a3-4420-86a2-beff07b549d3) > > Thiago Milczarek Sayao has updated the pull request with a new target base > due to a merge or a rebase. The pull request now contains 96 commits: > > - Merge branch 'refs/heads/master' into new_ime > - Merge branch 'refs/heads/master' into new_ime > - Merge branch 'master' into new_ime > - Add signals to avoid warnings > - Merge branch 'master' into new_ime > ># Conflicts: ># modules/javafx.graphics/src/main/native-glass/gtk/GlassApplication.cpp > - Add check for jview > - - Fix comment path > - - Fix double keyrelease > - - Use a work-around to relative positioning (until wayland is not > officially supported) >- Unref pango attr list > - Merge branch 'master' into new_ime > - ... and 86 more: https://git.openjdk.org/jfx/compare/c8b96e4e...d6230dec Could we get this reviewed please? Perhaps @lukostyra or @beldenfox ? Also, there was a comment from @andzsinszan > TextFieldTableCell -> a bit buggy has it been looked at? - PR Comment: https://git.openjdk.org/jfx/pull/1080#issuecomment-2287200158
Re: RFR: 8289174: JavaFX build fails on Windows when VS150COMNTOOLS is not set [v2]
On Tue, 13 Aug 2024 06:27:51 GMT, Lukasz Kostyra wrote: >> Thanks, that would be helpful. Let me know what you find. > > I checked on a Win 11 system where Build Tools 2022 are installed in default > directory (no other Visual Studio edition) and the build failed with "Cannot > locate Visual Studio compilers" message. Adding `BuildTools` as an option to > this loop fixed it and JFX built properly (checked without Media and Webkit). > Alternatively, setting `VSCOMMONTOOLS` also helps, but I think we can easily > add `BuildTools` as an option here. > > As a side-note - building without this PR on the same system also failed so > it doesn't necessarily introduce a regression, but I think it would be a > worthy addition regardless. I decided to go ahead and add this. Can you test and review it? - PR Review Comment: https://git.openjdk.org/jfx/pull/1534#discussion_r1716227825
Re: RFR: 8289174: JavaFX build fails on Windows when VS150COMNTOOLS is not set [v4]
> 1. Added logic to look in the standard locations for Visual Studio 2022, with
> a fallback to 2019 and then 2017 if 2022 is not found. It will look in the
> following locations:
>
> C:("Program Files"|"Program Files (x86)")\Microsoft Visual
> Studio(2022|2019|2017)\
> (Enterprise|Professional|Community)\VC\Auxiliary\Build
>
> 2. Use `VSCOMNTOOLS` as the primary way for a developer to specify a custom
> location for Visual Studio, with `VS150COMNTOOLS` as a legacy fallback (a
> warning is printed if `VS150COMNTOOLS` is set). If a developer installs
> Visual Studio in the default location, there will be no need to set this any
> more.
> 3. Removed support for older compilers (VS2010 and VS2013 in particular),
> since they will no longer work anyway. We were still using the 32-bit version
> of the script in many cases, which is unnecessary and no longer makes sense,
> so as part of this cleanup, I now consistently use `vcvars64.bat` to set up
> the environment.
> 4. Modified the GitHub actions build to no longer specify `VS150COMNTOOLS`,
> since it will now find it automatically.
> 5. Removed the following unused variables:
>
> DEVENVCMD
> DXSDK_DIR
> VS_VER
>
> 6. Changed most of the defaults in `win.gradle` to the empty string. The
> defaults were a misguided attempt to set values to something in case the
> Visual Studio installation cannot be found. By not having defaults, it will
> be more obvious if something goes wrong (and will fail fast).
> 7. If the Visual Studio Installation cannot be found, it will print a
> sensible error message and retry the next time the build is run (making it
> less likely that a manual `rm -rf build` is needed).
> 8. Fixed a bug where the Microsoft redist files were not being located and
> copied into the build dir (build/sdk/bin).
>
>
> I left some debug print statements in, and will remove them in a follow-on
> commit.
>
> I have tested this with a local installation of Visual Studio 2022 Community
> and, via GitHub Actions, an installation of Visual Studio 2022 Enterprise. On
> my local system, I built with and without Media and WebKit.
Kevin Rushforth has updated the pull request incrementally with one additional
commit since the last revision:
Look for BuildTools if other VS editions not found
-
Changes:
- all: https://git.openjdk.org/jfx/pull/1534/files
- new: https://git.openjdk.org/jfx/pull/1534/files/4bd166ea..6cd3f239
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jfx&pr=1534&range=03
- incr: https://webrevs.openjdk.org/?repo=jfx&pr=1534&range=02-03
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jfx/pull/1534.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1534/head:pull/1534
PR: https://git.openjdk.org/jfx/pull/1534
RFR: 8337827: [XWayland] Skip failing tests on Wayland
This changeset introduces the `Util.isOnWayland()` method and skips failing tests on Wayland. After that, all tests run on Wayland without failures. - Commit messages: - add javadoc - 8337827: [XWayland] Skip failing tests on Wayland Changes: https://git.openjdk.org/jfx/pull/1536/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1536&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8337827 Stats: 31 lines in 4 files changed: 25 ins; 5 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1536.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1536/head:pull/1536 PR: https://git.openjdk.org/jfx/pull/1536
