Integrated: 8269244: [IDE] Dependency verification of *-sources.jar fails when doing gradle sync
On Mon, 28 Jun 2021 14:38:32 GMT, Marius Hanl wrote: > This PR fixes the issue identified and discussed in PR > https://github.com/openjdk/jfx/pull/506 which will make the gradle sync in > IntelliJ fail because of the failing dependency verification for source files. > > Gradle provides a way to skip the verification of source files, documented > here: > https://docs.gradle.org/current/userguide/dependency_verification.html#sec:skipping-javadocs > > Also the **README.txt** file is adjusted to include the instructions > neccessary when updating the **verification-metadata.xml** file. This pull request has now been integrated. Changeset: 98e51669 Author:Marius Hanl Committer: Nir Lisker URL: https://git.openjdk.java.net/jfx/commit/98e516698f02bfd3484ca5721c014dae8ddf80c9 Stats: 13 lines in 2 files changed: 12 ins; 0 del; 1 mod 8269244: [IDE] Dependency verification of *-sources.jar fails when doing gradle sync Reviewed-by: kcr, nlisker - PR: https://git.openjdk.java.net/jfx/pull/549
Withdrawn: 8194924: Checking for selection size before update
On Thu, 24 Jun 2021 11:29:23 GMT, Alexander Shaklein wrote: > It is possible situation when `clearSelection()` is invoked during > `onChange()` notify. In such case `selectedCellsSeq` is clearing and possible > `IndexOutOfBoundsException` on `GenericAddRemoveChange` creation. > So we should check it to create correct `GenericAddRemoveChange`. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jfx/pull/542
Re: RFR: 8269259: Remove obsolete apps, tests, and scripts
On Fri, 25 Jun 2021 14:59:00 GMT, Kevin Rushforth wrote: > This PR deletes the following applications, tests, and scripts that are > either obsolete > or unmaintained: > > apps/performance/* > > apps/tests/HelloTest > > apps/toys/FXSlideShow > apps/toys/Industrial > apps/toys/Shape3DToy > apps/toys/StretchyGrid > apps/toys/TouchSuite > > tests/functional/* > tests/performance/VMPerformance > > tools/* > > See the JBS issue and [this > thread](https://mail.openjdk.java.net/pipermail/openjfx-dev/2021-June/030894.html) > on the openjfx-dev mailing list for more info. > > There are two commits for ease of review. The first one is generated by doing > a "git rm -r" on each of the above listed directories. The second is to > remove any remaining references to the removed directories. Looks fine. I don't know which of these are used frequently and which aren't, but I tested that there are no errors as a result of this patch. Under tests/system/src there are several projects called "testappN". Are they named cryptically on purpose? - Marked as reviewed by nlisker (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/545
RFR: 8268683: JavaFX MediaPlayer onEndOfMedia behaviour different from Javadoc
Fixed javadoc to indicate that onEndOfMedia is invoked each time when end of cycle is reached regardless if it is repeating or not. - Commit messages: - 8268683: JavaFX MediaPlayer onEndOfMedia behaviour different from Javadoc Changes: https://git.openjdk.java.net/jfx/pull/552/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=552=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268683 Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/552.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/552/head:pull/552 PR: https://git.openjdk.java.net/jfx/pull/552
Re: RFR: 8269244: [IDE] Dependency verification of *-sources.jar fails when doing gradle sync
On Mon, 28 Jun 2021 14:38:32 GMT, Marius Hanl wrote: > This PR fixes the issue identified and discussed in PR > https://github.com/openjdk/jfx/pull/506 which will make the gradle sync in > IntelliJ fail because of the failing dependency verification for source files. > > Gradle provides a way to skip the verification of source files, documented > here: > https://docs.gradle.org/current/userguide/dependency_verification.html#sec:skipping-javadocs > > Also the **README.txt** file is adjusted to include the instructions > neccessary when updating the **verification-metadata.xml** file. I tested this on Eclipse and it resolves the error of not finding the JUnit source jar file during dependency verification. Eclipse Buildship is happy now. - Marked as reviewed by nlisker (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/549
Re: RFR: 8260528: Clean glass-gtk sizing and positioning code [v27]
> This is a new approach to rewrite parts of gtk glass backend to be more clean. > > I will provide small "manageable" PR to incrementally make the backend better. > > This PR adresses cleanup of the Size and Positioning code. It makes code more > "straightforward" and easier to maintain. > > Current status (Ubuntu 20.04): >  > > (*) Some of the iconify tests are also failing on the main branch. > > `gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests > test.robot.javafx.stage.IconifyTest` on a second run produces 4 tests, 2 > failures. Thiago Milczarek Sayao has updated the pull request incrementally with one additional commit since the last revision: Fix build gtk_widget_set_sensitive has no effect - Changes: - all: https://git.openjdk.java.net/jfx/pull/367/files - new: https://git.openjdk.java.net/jfx/pull/367/files/f4a13314..9ff5a7b6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=367=26 - incr: https://webrevs.openjdk.java.net/?repo=jfx=367=25-26 Stats: 3 lines in 1 file changed: 1 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/367.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/367/head:pull/367 PR: https://git.openjdk.java.net/jfx/pull/367
Re: RFR: 8260528: Clean glass-gtk sizing and positioning code [v26]
> This is a new approach to rewrite parts of gtk glass backend to be more clean. > > I will provide small "manageable" PR to incrementally make the backend better. > > This PR adresses cleanup of the Size and Positioning code. It makes code more > "straightforward" and easier to maintain. > > Current status (Ubuntu 20.04): >  > > (*) Some of the iconify tests are also failing on the main branch. > > `gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests > test.robot.javafx.stage.IconifyTest` on a second run produces 4 tests, 2 > failures. 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 20 commits: - Small compilation fix for ubuntu 16.04 - Merge branch 'openjdk:master' into master - Merge pull request #18 from openjdk/master Merge master - Fix bug in content oriented child windows - Small compilation fix for ubuntu 16.04 - Replace the window size & positining code Default to 320x200 if no size is assigned Hopefully fix all "extra resize" problems due to frame extents. Small change to reuse get_net_frame_extents_atom() Fix more tests (restore 1 behaviour) More test fixes Partial progress Adjust positioning (not 100% yet) It's looking good. Except for fixed initial extents, but it seems a reasonable fix due to nature o frame extents. It's probably good now One more small issue - Merge pull request #17 from openjdk/master Pull from origin - Merge pull request #16 from openjdk/master Update - Merge pull request #15 from openjdk/master Update from jfx - Merge pull request #14 from openjdk/master Merge master - ... and 10 more: https://git.openjdk.java.net/jfx/compare/78179be1...f4a13314 - Changes: https://git.openjdk.java.net/jfx/pull/367/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=367=25 Stats: 615 lines in 7 files changed: 150 ins; 311 del; 154 mod Patch: https://git.openjdk.java.net/jfx/pull/367.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/367/head:pull/367 PR: https://git.openjdk.java.net/jfx/pull/367
Re: RFR: 8194924: Checking for selection size before update
On Thu, 24 Jun 2021 11:29:23 GMT, Alexander Shaklein wrote: > It is possible situation when `clearSelection()` is invoked during > `onChange()` notify. In such case `selectedCellsSeq` is clearing and possible > `IndexOutOfBoundsException` on `GenericAddRemoveChange` creation. > So we should check it to create correct `GenericAddRemoveChange`. As mentioned above, and also in the JBS issue, it is likely that we will close the bug as "not an issue", in which case this PR should be closed (withdrawn). - PR: https://git.openjdk.java.net/jfx/pull/542
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Fri, 25 Jun 2021 04:08:30 GMT, Prasanta Sadhukhan
wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8223717: javafx printing: Support Specifying Print to File in the API
>
> modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinterJob.java
> line 352:
>
>> 350: try {
>> 351:
>> settings.setOutputFile(dest.getURI().toURL().toString());
>> 352: } catch (MalformedURLException e) {
>
> I guess in 2D RasterPrinterJob, if there is any exception, we try to form a
> default file "out.prn"
> [https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/print/RasterPrinterJob.java#L1137]
> and try to print there. Can't we do it here too?
I am going to just change to support file so this becomes a non-issue
> modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinterJob.java
> line 356:
>
>> 354: } else {
>> 355: settings.setOutputFile("");
>> 356: }
>
> Actually in 2D , it seems if dest is null, we redirect printing to actual
> printer instead of file. Not sure if that is what we need to do here too!!
> [https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/print/RasterPrinterJob.java#L1148]
that is what we are doing ..
> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 485:
>
>> 483: *
>> 484: * If the application does not have permission to write to the
>> specified
>> 485: * file, a {@code SecurityException} may be thrown when printing.
>
> As I see in 2D atleast in Win32PrintService, if there is a SecurityException
> for creating a Destination object from a URI, it retries again by creating a
> new URL with default file "file:out.prn"
> Is it not needed here?
> [https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#L1181]
No. We are doing our own checking if a SM is running and disallow it.
-
PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8269244: [IDE] Dependency verification of *-sources.jar fails when doing gradle sync
On Mon, 28 Jun 2021 14:38:32 GMT, Marius Hanl wrote: > This PR fixes the issue identified and discussed in PR > https://github.com/openjdk/jfx/pull/506 which will make the gradle sync in > IntelliJ fail because of the failing dependency verification for source files. > > Gradle provides a way to skip the verification of source files, documented > here: > https://docs.gradle.org/current/userguide/dependency_verification.html#sec:skipping-javadocs > > Also the **README.txt** file is adjusted to include the instructions > neccessary when updating the **verification-metadata.xml** file. Looks good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/549
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v2]
On Fri, 25 Jun 2021 21:10:16 GMT, Kevin Rushforth wrote: >> But the JobSettings class is final .. is it still necessary ? > > It's still a good idea to follow the pattern, so yes let's make the new > methods final. We can file a cleanup bug for the existing ones (and since the > class is final, we can do it without compatibility concerns). > > Also, I built the docs, and it is better to not add any javadoc comments on > the setter and getter. I'll file a clean up bug - PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Fri, 25 Jun 2021 21:13:31 GMT, Kevin Rushforth wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8223717: javafx printing: Support Specifying Print to File in the API
>
> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 492:
>
>> 490: * a writable file it may be ignored, or a SecurityException may be
>> thrown.
>> 491: * The default value is an empty string, which is interpreted as
>> unset,
>> 492: * which means output is sent to the printer.
>
> Can you add a `* @defaultValue empty string` javadoc tag?
will do ..
> tests/manual/printing/PrintToFileTest.java line 134:
>
>> 132: job.printPage(printNode);
>> 133: job.endJob();
>> 134: if (f.exists()) {
>
> I have only tried this on Windows. This check is failing for me when I run
> the test, and yet the file is created correctly. If I add a short sleep, it
> works (so the printing is likely being done on a different thread).
interesting. I guess I will have to add a sleep as you did.
> tests/manual/printing/PrintToFileTest.java line 137:
>
>> 135: System.out.println("created file");
>> 136: passed = true;
>> 137: f.delete();
>
> Maybe leave the file? I found it useful to verify its contents.
ok
-
PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v4]
On Thu, 24 Jun 2021 22:53:33 GMT, Phil Race wrote: >> modules/javafx.graphics/src/main/java/javafx/print/JobSettings.java line 490: >> >>> 488: * setting will be ignored. >>> 489: * If the URL specifies a non-existent path, or does not specify >>> 490: * a writable file it may be ignored, or a SecurityException may >>> be thrown. >> >> I want to look at what we do for other properties like this. I'm not sure >> how feasible it is to throw an exception back to the application. And I >> wouldn't want to single out a security exception (what about a read-only >> file system?). I think that whatever the reason, if the file can't be >> written to, we should treat it consistently. Probably by ignoring the >> request. I presume that's what will happen if the print driver doesn't >> support print-to-file? > > Not easy to specify exactly. We are handing it off to 2D > We aren't doing any up-front verification here, but I don't want to copy 2D > behaviour into the docs, even if I could be sure about that .. I may just > want to be more waffly about what happens. > Could be printed to the printer, could be a failed job, could be an exception > of some kind if some nice piece of Java code recognises a problem up-front So the SE will occur only if there is a SM and the code in J2DPrinterJob.checkPermission() rejects it. - PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v4]
On Fri, 25 Jun 2021 21:08:31 GMT, Kevin Rushforth wrote:
>> I agree with @kevinrushforth that if a String without protocol is passed, it
>> should be treated as a file (absolute or relative to ?)
>> I'm also not sure that the URL should be exposed here. I understand it's
>> needed in the lower-level print API but you already do the conversion in the
>> `syncOutputFile` method. Hence, since only the file protocol is supported,
>> it might be easier for API users to pass the location of the file instead of
>> a URL.
>> In case later other URL protocols are supported, a property `outputURL`
>> might be introduced?
>
> I like the flexibility and consistency of defining it as a URL, as long as we
> also interpret a url without a scheme as a file name. Borrowing language from
> the Image docs, perhaps something like this?
>
>
> The URL string can either be a URL with a "file:" protocol that can be
> resolved
> by @link java.net.URL} or a file path that can be resolved by {@link
> java.io.File}.
I am changing it to be just a file path. We can extend it later if ever needed.
It does simplify the code and the potential for errors.
-
PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
On Fri, 25 Jun 2021 21:20:53 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/com/sun/prism/j2d/print/J2DPrinterJob.java
>> line 839:
>>
>>> 837: security.checkPrintJobAccess();
>>> 838: String file = settings.getOutputFile();
>>> 839: if (!file.isEmpty()) {
>>
>> Don't we need to check for file!= null?
>
> The default value for the property is the empty string. But it does bring up
> a good point that we should either check and throw NPE if `setOutputFile` is
> called with `null` or we should map null to the empty string.
we do remap null to the empty string .. the code in
JobSettings.outputFileProperty() does it.
So this will never be null
-
PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v4]
> This enhancement adds the String property outputFileProperty() to the > JobSettings class. > The value should be a string that references a local file encoded as a URL. > If this is non-null and set to a location that the user has permission to > write to, > then the printer output will be spooled there instead of the printer, so long > as the platform printing system supports this. > The user can of course also set a print-to-file destination in the platform > printer dialogs which may over-ride what the application set. But now the > application can also see what it was set to, and cancel or alter it if > necessary. > > A simple manual test is provided, manual mainly because the few real printing > functional tests are all manual as they are only useful if run with a printer > configured. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8223717: javafx printing: Support Specifying Print to File in the API - Changes: - all: https://git.openjdk.java.net/jfx/pull/543/files - new: https://git.openjdk.java.net/jfx/pull/543/files/8050d5f9..34221a95 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=543=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=543=02-03 Stats: 85 lines in 3 files changed: 21 ins; 38 del; 26 mod Patch: https://git.openjdk.java.net/jfx/pull/543.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/543/head:pull/543 PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8223717: javafx printing: Support Specifying Print to File in the API [v3]
Yes .. we still need to deal with it until it is actually removed. Its going to be here for all the life of JDK 17 LTS which I expect FX will want to support for all that time. -phil. On 6/25/21 6:42 PM, Eric Bresie wrote: security manager That’s not the same security manager being discussed as being deprecated for Java 17 and beyond is it? Eric Bresie ebre...@gmail.com (mailto:ebre...@gmail.com) On June 25, 2021 at 4:20:03 PM CDT, Kevin Rushforth mailto:k...@openjdk.java.net)> wrote: On Thu, 24 Jun 2021 23:00:42 GMT, Phil Race mailto:p...@openjdk.org)> wrote: Overall the new API and functionality looks good, and fills a void that was discussed [in this thread](https://mail.openjdk.java.net/pipermail/openjfx-dev/2019-May/023292.html) a couple years ago. General comments: The first, which is the one we had been discussing, is where the URL is a valid file URL, but the program can't write to it, either because we are running with a security manager and the application doesn't have permission, or because we can't write to the output file (e.g., the path doesn't exist or is read-only). PR: https://git.openjdk.java.net/jfx/pull/543
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin
On Mon, 28 Jun 2021 13:18:38 GMT, Marius Hanl wrote:
>> The issue is about memory leaks and side-effects (like NPEs) when switching
>> skins.
>>
>> Details (copied from issue for convenience):
>>
>> memory leak in TextInputControlBehavior:
>> - listener accidentally added twice (removed once)
>> - keyPad mappings not properly installed/disposed
>>
>> memory leak TextFieldBehavior:
>> - listeners to scene/focusOwner property not removed,
>>
>> memory leak in TextInputControlSkin:
>> - event handlers related to inputMethods not removed
>>
>> issues in TextFieldSkin:
>> - memory leak due to behavior leaking
>> - memory leaks due to manually added change/invalidation listeners that are
>> not removed
>> - memory leaks due to not removing children with strong references to skin
>> - side-effects (f.i. NPEs) due to listeners/bindings that are still active
>> after dispose
>>
>> Fix was to properly install/remove those listeners/handlers. Added tests
>> that failed/passed before/after the fix, respectively, also added tests
>> passing before that must pass after the fix.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
> line 390:
>
>> 388: @Override public void dispose() {
>> 389: if (getSkinnable() == null) return;
>> 390: getChildren().removeAll(textGroup, handleGroup);
>
> Also curious: Most of the skins don't remove their children in **dispose()**.
> Are they all wrong, or is this a special case here?
they are all wrong ;) When starting with the cleanup, we (me at least ;)
weren't yet entirely certain - but not removing them let them hang in the
hierarchy, reachable by strong references from their parent. Which in itself
isn't pretty (and might lead to unwanted side-effects) - but if they in turn
have any references to the skin (even not so obvious - for me again - like
being a not static nested class of the skin ;) the skin is strongly reachable
as well, making it leaky.
> modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorCleanupTest.java
> line 162:
>
>> 160: secondStage.hide();
>> 161: }
>> 162:
>
> minor: this empty line can be removed
yeah, can do .. but do we have conventions about vertical whitespace? Tend to
keep what suits my eyes :)
-
PR: https://git.openjdk.java.net/jfx/pull/534
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin
On Mon, 28 Jun 2021 12:55:47 GMT, Marius Hanl wrote:
>> The issue is about memory leaks and side-effects (like NPEs) when switching
>> skins.
>>
>> Details (copied from issue for convenience):
>>
>> memory leak in TextInputControlBehavior:
>> - listener accidentally added twice (removed once)
>> - keyPad mappings not properly installed/disposed
>>
>> memory leak TextFieldBehavior:
>> - listeners to scene/focusOwner property not removed,
>>
>> memory leak in TextInputControlSkin:
>> - event handlers related to inputMethods not removed
>>
>> issues in TextFieldSkin:
>> - memory leak due to behavior leaking
>> - memory leaks due to manually added change/invalidation listeners that are
>> not removed
>> - memory leaks due to not removing children with strong references to skin
>> - side-effects (f.i. NPEs) due to listeners/bindings that are still active
>> after dispose
>>
>> Fix was to properly install/remove those listeners/handlers. Added tests
>> that failed/passed before/after the fix, respectively, also added tests
>> passing before that must pass after the fix.
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextFieldBehavior.java
> line 75:
>
>> 73: }
>> 74:
>> 75: focusListener = (observable, oldValue, newValue) -> {
>
> Shouldn't this focusListener also be wrapped in a weakListener? Also this
> lambda expression can be a one-liner (no braces needed)
we are a bit inconsistent in wrapping (or not) listeners into their weak
counterparts - behaviors tend to not :) That's okay - and less crowded by
additional code - as long as they are removed properly, IMO. But just seeing:
good question, as the focusOwner listener is wrapped, need to consult my notes.
Thanks for the heads up!
As to the single vs. multiple lines: ersonally, I tend to not change more than
absolutely needed - it had the brackets before the fix so I kept them.
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
> line 389:
>
>> 387: /** {@inheritDoc} */
>> 388: @Override public void dispose() {
>> 389: if (getSkinnable() == null) return;
>
> Just curious: Can the skinnable be null here? And is it fine then to never
> call **super.dispose()** ?
dispose has no precondition - it can be called multiple times (explicitly
specified in its javadoc), so has to guard itself against having cleaned
already. And super is called the first time.
-
PR: https://git.openjdk.java.net/jfx/pull/534
RFR: 8269244: [IDE] Dependency verification of *-sources.jar fails when doing gradle sync
This PR fixes the issue identified and discussed in PR https://github.com/openjdk/jfx/pull/506 which will make the gradle sync in IntelliJ fail because of the failing dependency verification for source files. Gradle provides a way to skip the verification of source files, documented here: https://docs.gradle.org/current/userguide/dependency_verification.html#sec:skipping-javadocs Also the **README.txt** file is adjusted to include the instructions neccessary when updating the **verification-metadata.xml** file. - Commit messages: - Fixed dependency verification of *-sources.jar fails when doing gradle sync Changes: https://git.openjdk.java.net/jfx/pull/549/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=549=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269244 Stats: 13 lines in 2 files changed: 12 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/549.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/549/head:pull/549 PR: https://git.openjdk.java.net/jfx/pull/549
Re: RFR: 8268642: Expand the javafx.beans.property.Property documentation [v2]
On Thu, 24 Jun 2021 01:53:53 GMT, Michael Strauß wrote: >> * Expand the `Property.bind` and `Property.bindBidirectional` documentation >> * Change the name of the formal parameter of `Property.bind` to "source" >> (currently, it is inconsistently named "observable", "rawObservable" or >> "newObservable") > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > changes as per review I like this change, it makes the binding framework doc much more clear. But I agree with Nir, I think 'the' reads better then 'a' on the occurrences. - PR: https://git.openjdk.java.net/jfx/pull/533
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin
On Thu, 17 Jun 2021 12:41:56 GMT, Jeanette Winzenburg
wrote:
> The issue is about memory leaks and side-effects (like NPEs) when switching
> skins.
>
> Details (copied from issue for convenience):
>
> memory leak in TextInputControlBehavior:
> - listener accidentally added twice (removed once)
> - keyPad mappings not properly installed/disposed
>
> memory leak TextFieldBehavior:
> - listeners to scene/focusOwner property not removed,
>
> memory leak in TextInputControlSkin:
> - event handlers related to inputMethods not removed
>
> issues in TextFieldSkin:
> - memory leak due to behavior leaking
> - memory leaks due to manually added change/invalidation listeners that are
> not removed
> - memory leaks due to not removing children with strong references to skin
> - side-effects (f.i. NPEs) due to listeners/bindings that are still active
> after dispose
>
> Fix was to properly install/remove those listeners/handlers. Added tests that
> failed/passed before/after the fix, respectively, also added tests passing
> before that must pass after the fix.
Just a formal review, I left some comments inline
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextFieldBehavior.java
line 75:
> 73: }
> 74:
> 75: focusListener = (observable, oldValue, newValue) -> {
Shouldn't this focusListener also be wrapped in a weakListener? Also this
lambda expression can be a one-liner (no braces needed)
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
line 389:
> 387: /** {@inheritDoc} */
> 388: @Override public void dispose() {
> 389: if (getSkinnable() == null) return;
Just curious: Can the skinnable be null here? And is it fine then to never call
**super.dispose()** ?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
line 390:
> 388: @Override public void dispose() {
> 389: if (getSkinnable() == null) return;
> 390: getChildren().removeAll(textGroup, handleGroup);
Also curious: Most of the skins don't remove their children in **dispose()**.
Are they all wrong, or is this a special case here?
modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorCleanupTest.java
line 162:
> 160: secondStage.hide();
> 161: }
> 162:
minor: this empty line can be removed
-
Changes requested by mhanl (Author).
PR: https://git.openjdk.java.net/jfx/pull/534
RFR: 8234921: Add DirectionalLight to the selection of 3D light types
Adds a directional light as a subclass of `LightBase`. I think that this is the correct hierarchy for it. I tried to simulate a directional light by putting a point light far away, but I got artifacts when the distance was large. Instead, I added an on/off attenuation flag as the 4th component of the attenuation 4-vector. When it is 0, a simpler computation is used in the pixel/fragment shader that calculates the illumination based on the light direction only (making the position variables meaningless). When it is 1, the point/spot light computation is used. It's possible that the vertex shader can also be simplified in this case since it does not need to transform the position vectors, but I left this optimization avenue for another time. I noticed a drop of ~1 fps in the stress test of 5000 meshes. I added a system test that verifies the correct color result from a few directions. I also updated the lighting sample application to include 3 directional lights and tested them on all the models visually. The lights seem to behave the way I would expect. - Commit messages: - Small corrections to class members docs - Removed whitespace - Reordered setLight arguments in NGShape3D - Updated class doc - Initial commit of the opengl pipeline - Updated tests - Initial java and d3d implementation Changes: https://git.openjdk.java.net/jfx/pull/548/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=548=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8234921 Stats: 533 lines in 29 files changed: 472 ins; 11 del; 50 mod Patch: https://git.openjdk.java.net/jfx/pull/548.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/548/head:pull/548 PR: https://git.openjdk.java.net/jfx/pull/548
Re: RFR: 8231558: [macos] Platform.exit causes assertion error on macOS 10.15 or later [v2]
On Mon, 28 Jun 2021 12:27:34 GMT, Kevin Rushforth wrote: >> This is a fix for the assertion error message that is printed to the console >> on macOS 10.15 or later when an application calls `Platform.exit` while a >> `Stage` is showing. >> >> The root cause is a latent bug in the JavaFX glass code that was revealed by >> an apparent change of behavior in macOS. A few of the object deallocation >> methods, which are called by the Objective C auto-release mechanism, use the >> standard `GET_MAIN_JENV` macro to get the JNI environment. The macro will >> print an assertion warning if Java has been detached. I instrumented the >> code and can see that `GlassViewDelegate::dealloc` is now called after the >> `GlassApplication` main loop has detached Java. Since we don't control when >> the dealloc method is called, it is not correct to do the assertion check in >> those cases. Some of the dealloc methods already skip this assertion check >> by grabbing the jEnv pointer directly, so we need to fix the others. I added >> a new variant of the macro called `GET_MAIN_JENV_NOWARN` with a comment >> indicating that is suitable for use by the dealloc methods. >> >> In addition to verifying that the test program attached to JBS now exits >> cleanly with no assertion failure message, I added an automated system test >> that fails on macOS before the fix and passes after the fix. On other >> platforms it passes already. > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Fix value of ERROR_TIMEOUT constant per code review Looks good - Marked as reviewed by pbansal (Committer). PR: https://git.openjdk.java.net/jfx/pull/540
Re: RFR: 8231558: [macos] Platform.exit causes assertion error on macOS 10.15 or later [v2]
On Sun, 27 Jun 2021 13:16:40 GMT, Pankaj Bansal wrote: >> Kevin Rushforth has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix value of ERROR_TIMEOUT constant per code review > > tests/system/src/test/java/test/launchertest/Constants.java line 70: > >> 68: static final int ERROR_ASSERTION_FAILURE = 28; >> 69: >> 70: static final int ERROR_TIMEOUT = 28; > > Should not this be assigned error code 29 instead of 28? Yes, thanks for catching this. I fixed it. - PR: https://git.openjdk.java.net/jfx/pull/540
Re: RFR: 8231558: [macos] Platform.exit causes assertion error on macOS 10.15 or later [v2]
> This is a fix for the assertion error message that is printed to the console > on macOS 10.15 or later when an application calls `Platform.exit` while a > `Stage` is showing. > > The root cause is a latent bug in the JavaFX glass code that was revealed by > an apparent change of behavior in macOS. A few of the object deallocation > methods, which are called by the Objective C auto-release mechanism, use the > standard `GET_MAIN_JENV` macro to get the JNI environment. The macro will > print an assertion warning if Java has been detached. I instrumented the code > and can see that `GlassViewDelegate::dealloc` is now called after the > `GlassApplication` main loop has detached Java. Since we don't control when > the dealloc method is called, it is not correct to do the assertion check in > those cases. Some of the dealloc methods already skip this assertion check by > grabbing the jEnv pointer directly, so we need to fix the others. I added a > new variant of the macro called `GET_MAIN_JENV_NOWARN` with a comment > indicating that is suitable for use by the dealloc methods. > > In addition to verifying that the test program attached to JBS now exits > cleanly with no assertion failure message, I added an automated system test > that fails on macOS before the fix and passes after the fix. On other > platforms it passes already. Kevin Rushforth has updated the pull request incrementally with one additional commit since the last revision: Fix value of ERROR_TIMEOUT constant per code review - Changes: - all: https://git.openjdk.java.net/jfx/pull/540/files - new: https://git.openjdk.java.net/jfx/pull/540/files/5abf1b66..68abfaf5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=540=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=540=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/540.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/540/head:pull/540 PR: https://git.openjdk.java.net/jfx/pull/540
Integrated: 8196065: ListChangeListener getRemoved() returns items that were not removed.
On Fri, 23 Apr 2021 03:19:03 GMT, Michael Strauß wrote: > The documentation for `ObservableListBase.nextRemove` states that a single > change always refers to the current state of the list, which likely means > that multiple disjoint removed ranges need to be applied in order, otherwise > the next change's `getFrom` doesn't refer to the correct index. > > `SelectedItemsReadOnlyObservableList` doesn't apply removals to > `itemsRefList`, which means that subsequent removals will refer to the wrong > index when retrieving the removed elements. This PR fixes the calculation of > the current index. This pull request has now been integrated. Changeset: c4cc9988 Author:Michael Strauß Committer: Ambarish Rapte URL: https://git.openjdk.java.net/jfx/commit/c4cc99882928e423003d54cae0a6b0de2ec05007 Stats: 321 lines in 8 files changed: 291 ins; 6 del; 24 mod 8196065: ListChangeListener getRemoved() returns items that were not removed. Reviewed-by: arapte, kcr - PR: https://git.openjdk.java.net/jfx/pull/478
Re: RFR: 8267418: IntelliJ build and test of JavaFX does not work [v4]
On Wed, 2 Jun 2021 07:11:54 GMT, Marius Hanl wrote: >> ~~Question: I was wondering, should I create a ticket for this as well? >> Given the fact that I don't have an https://bugs.openjdk.java.net account, I >> need to use the official bug reporting tool, which looked a bit overkill to >> me since someone needs to check my created ticket, while this PR is only >> affecting the IntelliJ IDE with OpenJFX and not the JavaFX platform >> directly.~~ >> EDIT: Thank you, Kevin. :) >> >> This PR fixes the errors you get when cloning and working with OpenJFX in >> IntelliJ IDE: >> - The **.idea/misc.xml** is modified to use **JDK_11** as language level >> instead of JDK_8. >> -> This is the language level shown inside the **Project Structure**. (File >> -> Project Structure...) >> - The **.idea/base.iml, .idea/controls.iml, .idea/fxml.iml, .idea/web.iml, >> .idea/graphics.iml** are modified to include/recognize the shims (as test >> resource, this is very similar to the configuration inside the .classpath >> file from Eclipse) >> - EDIT: The projects are now recognized by IntelliJ-gradle >> (**.idea/gradle.xml**, **.idea/compiler.xml**) >> >> **-> With this, I can run all normal tests with IntelliJ** >> >> ### What I couldn't fix yet (When I tried, it looked like IntelliJ is >> overriding the settings on next gradle reload): >> - IntelliJ is not detecting javafx.graphic inside the shims >> - All javafx.* dependencies are not found for the system tests >> >> **-> If someone has a solution, feel free to comment :)** > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > Reverted whitespace made by IntelliJ Approving, I re-verified the change: Loading project in IntelliJ does not cause any other changes in idea files. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/506
