Correct branch for PR?
I have a testcase + fix ready for JDK-8237372 (A null pointer exception in TabPaneSkin if only a mouse release event is sent to the skin). To avoid the kind of confusion I created with my last pull request ;-) * For what branch should I create the pull request? (From my point of view jfx14 is preferable, since this is a real bug affecting our software) * Will pull requests for jfx14 (like my last one) be automatically pushed forward to the master branch? Best regards, Robert
RE: [EXTERNAL] CssStyleHelper canReuseStyleHelper
I'd create a scenegraph
R
.-+-.
A B
.+.
CD
Where C and D are Labels. Then I'd set a font style on A and a different font
style on B.
C and D should pick up the font style of A. Then move D to B and see if it
still has A's
font style.
It may be necessary to have a more deeply nested scene graph.
The potential issue is that the node that got moved could pick up the old set
of styles
if the style helper is not renewed.
Ultimately, canReuseStyleHelper will return false if the set of styles for the
new parent (line 110 or so in CssStyleHelper.java) are different.
> -Original Message-
> From: openjfx-dev On Behalf Of
> Dean Wookey
> Sent: Thursday, January 16, 2020 11:34 AM
> To: openjfx-dev@openjdk.java.net Mailing d...@openjdk.java.net>; David Grieve
> Cc: aghai...@openjdk.org
> Subject: [EXTERNAL] CssStyleHelper canReuseStyleHelper
>
> Hi Ajit, David,
>
> I came accross a potential issue introduced by JDK-8090462 (
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs
> .openjdk.java.net%2Fbrowse%2FJDK-
> 8090462&data=02%7C01%7Cdavid.grieve%40microsoft.com%7C235930d
> fab7f4f7f15ba08d79aa21774%7C72f988bf86f141af91ab2d7cd011db47%7C1%7
> C0%7C637147893881930150&sdata=awsMTPYkp7GmSrYsoy9I6M%2F6uj
> X7thgZhBVY9UKcjpU%3D&reserved=0), (
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2Fopenjdk%2Fjfx%2Fcommit%2F834b0d05e7a2a8351403ec4a121ea
> b312d80fd24%23diff-
> 9ec098280fa1aeed53c70243347e76ab&data=02%7C01%7Cdavid.grieve%
> 40microsoft.com%7C235930dfab7f4f7f15ba08d79aa21774%7C72f988bf86f141
> af91ab2d7cd011db47%7C1%7C0%7C637147893881930150&sdata=ZhWS
> dmlJ9rLzIkqnohWTacJKPb8FNrG5yApF0fwP8g4%3D&reserved=0).
> The issue is in the canReuseStyleHelper method. canReuseStyleHelper is
> called as a result of changes to the scene graph and for other reasons. The
> original code found the parent helper in the following way:
>
> Styleable parent = node.getStyleableParent();
> while (parent != null) {
> if (parent instanceof Node) {
> parentHelper = ((Node) parent).styleHelper;
> if (parentHelper != null) break;
> }
> parent = parent.getStyleableParent();
> }
>
> This gets the parent helper for the new parent of the node. The code now (
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2Fopenjdk%2Fjfx%2Fblob%2F20325e1c3ec4c4e81af74d3d43bf3a803
> dbe1a51%2Fmodules%2Fjavafx.graphics%2Fsrc%2Fmain%2Fjava%2Fjavafx%
> 2Fscene%2FCssStyleHelper.java%23L322&data=02%7C01%7Cdavid.griev
> e%40microsoft.com%7C235930dfab7f4f7f15ba08d79aa21774%7C72f988bf86f
> 141af91ab2d7cd011db47%7C1%7C0%7C637147893881930150&sdata=nJR
> 3ocKaIsf6HWNjHtseEbqLbEvbW7%2FzQS1B%2F39GVyI%3D&reserved=
> 0
> ):
>
> CssStyleHelper parentHelper =
> getStyleHelper(node.styleHelper.firstStyleableAncestor);
>
> gets the helper of the previous parent of the node since
> firstStyleableAncestor hasn't been updated to reflect the current state of the
> scene graph yet. This means if you move a node, it could keep its
> CssStyleHelper even if it's under completely new parents.
>
> I'm not sure if this is actually causing any problems. It would be helpful if
> I
> knew the kind of things that could go wrong if you reuse a style helper so
> that I could design a test that highlighted the issue.
>
> Can you perhaps think of cases where this could go badly?
>
> Dean
Re: [Rev 02] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Thu, 16 Jan 2020 17:00:47 GMT, Frederic Thevenet
wrote:
>> This PR aims to address the following issue: JDK-8088198 Exception thrown
>> from snapshot if dimensions are larger than max texture size
>>
>> In order to do that, it simply captures snapshots in multiple tiles of
>> maxTextureSize^2 dimensions (or less, as needed), and then recomposes all
>> the tiles into a a single image.
>> Other than that, the logic used to do the actual snapshot is unchanged.
>>
>> Tests using the existing SnapshotCommon class have been added in a new file
>> named Snapshot3Test under SystemTest/test/javafx/scene.
>> These tests pass with the proposed fix, and fail without, throwing "
>> java.lang.IllegalArgumentException: Unrecognized image loader: null"
>
> The pull request has been updated with 2 additional commits.
I tested this fix against the repro code in
https://github.com/javafxports/openjdk-jfx/issues/433 (which is
[JDK-838](https://bugs.openjdk.java.net/browse/JDK-838)), but there is
still an NPE. I'm not certain that this fix is supposed to solve that bug, but
according to the comments, the root cause is the same as
[JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082), which is
related to this one. It's worth to take a look to see if something was missed.
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1290:
> 1289: int yMin = (int)Math.floor(y);
> 1290: int xMax = (int)Math.ceil(x + w);
> 1291: int yMax = (int)Math.ceil(y + h);
While you're working in the area, this code can be written as
int width, height;
if (wimg == null) {
int xMax = (int)Math.ceil(x + w);
int yMax = (int)Math.ceil(y + h);
width = Math.max(xMax - xMin, 1);
height = Math.max(yMax - yMin, 1);
wimg = new WritableImage(width, height);
} else {
to avoid unnecessary computations.
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1313:
> 1312: int tileHeight = Math.min(maxTextureSize, height -
> yOffset);
> 1313: WritableImage tile = doSnapshotTile(scene, xMin +
> xOffset, yMin + yOffset, tileWidth, tileHeight, root, transform, depthBuffer,
> fill, camera, null);
> 1314: wimg.getPixelWriter().setPixels(xOffset, yOffset,
> tileWidth, tileHeight, tile.getPixelReader(), 0, 0);
This line is too long and needs to break into 2. I think that the limit is 135
characters.
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1316:
> 1315: }
> 1316: }
> 1317: } else {
I would extract this code into its own method similar to `doSnapshotTile`:
`assemble(scene, xMin, yMin, width, height, root, transform, depthBuffer, fill,
camera, wimg, maxTextureSize);`
(`assemble` is a bad name, I didn't think about a better one).
The method can return he resulting `WritableImage`, but it is not needed since
it is manipulated via "side-effects". I would, however, bring it line with the
`else` clause - either both use `wimg = methodName(..., wimg, ...);` or just
`methodName(..., wimg, ...);`. This is fine since the input `WritableImage` is
never `null`. From a readability point of view, using return values seems
better.
-
PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 01] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Thu, 16 Jan 2020 14:26:24 GMT, Kevin Rushforth wrote: >> I think it is not worth it. > > I agree. I do like the suggestion to rename the variables, though. done in 501917284e0490d16b1831fcd854e31a779449b9 - PR: https://git.openjdk.java.net/jfx/pull/68
CssStyleHelper canReuseStyleHelper
Hi Ajit, David,
I came accross a potential issue introduced by JDK-8090462 (
https://bugs.openjdk.java.net/browse/JDK-8090462), (
https://github.com/openjdk/jfx/commit/834b0d05e7a2a8351403ec4a121eab312d80fd24#diff-9ec098280fa1aeed53c70243347e76ab).
The issue is in the canReuseStyleHelper method. canReuseStyleHelper is
called as a result of changes to the scene graph and for other reasons. The
original code found the parent helper in the following way:
Styleable parent = node.getStyleableParent();
while (parent != null) {
if (parent instanceof Node) {
parentHelper = ((Node) parent).styleHelper;
if (parentHelper != null) break;
}
parent = parent.getStyleableParent();
}
This gets the parent helper for the new parent of the node. The code now (
https://github.com/openjdk/jfx/blob/20325e1c3ec4c4e81af74d3d43bf3a803dbe1a51/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java#L322
):
CssStyleHelper parentHelper =
getStyleHelper(node.styleHelper.firstStyleableAncestor);
gets the helper of the previous parent of the node since
firstStyleableAncestor hasn't been updated to reflect the current state of
the scene graph yet. This means if you move a node, it could keep its
CssStyleHelper even if it's under completely new parents.
I'm not sure if this is actually causing any problems. It would be helpful
if I knew the kind of things that could go wrong if you reuse a style
helper so that I could design a test that highlighted the issue.
Can you perhaps think of cases where this could go badly?
Dean
Re: [Rev 01] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Thu, 16 Jan 2020 10:47:26 GMT, Ambarish Rapte wrote:
>> The pull request has been updated with 1 additional commit.
>
> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1303:
>
>> 1302: if (height > maxTextureSize || width > maxTextureSize) {
>> 1303: // The requested size for the screenshot is to big to fit
>> a single texture,
>> 1304: // so we need to take several snapshot tiles and merge
>> them into single image (fixes JDK-8088198)
>
> Should be `too` big to fit
done in 501917284e0490d16b1831fcd854e31a779449b9
-
PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 01] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Thu, 16 Jan 2020 10:58:12 GMT, Ambarish Rapte wrote: >> The pull request has been updated with 1 additional commit. > > modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1312: > >> 1311: int tileWidth = Math.min(maxTextureSize, width - >> xOffset); >> 1312: int tileHeight = Math.min(maxTextureSize, height - >> yOffset); >> 1313: WritableImage tile = doSnapshotTile(scene, xMin + >> xOffset, yMin + yOffset, tileWidth, tileHeight, root, transform, >> depthBuffer, fill, camera, null); > > `int xOffset = i * maxTextureSize;` > `int tileWidth = Math.min(maxTextureSize, width - xOffset);` > > These two lines should be moved to the outer loop of horizontal tabs. done in 501917284e0490d16b1831fcd854e31a779449b9 - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 01] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Thu, 16 Jan 2020 14:16:37 GMT, Michael Paus wrote: >> Indeed, substituting the floating math for integer math requires an extra >> test on the remainder to be correct. >> Since this code isn't located in a tight loop, I'm not sure this >> optimization is worth making the code less straight forward. What do you all >> think? > > I think it is not worth it. I agree. I do like the suggestion to rename the variables, though. - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 01] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Thu, 16 Jan 2020 13:56:44 GMT, Frederic Thevenet wrote: >> I think the proposed code changes are wrong in case that `height / >> maxTextureSize` is an exact integer. (Same for width). I normally add the 1 >> only if `height % maxTextureSize != 0` > > Indeed, substituting the floating math for integer math requires an extra > test on the remainder to be correct. > Since this code isn't located in a tight loop, I'm not sure this optimization > is worth making the code less straight forward. What do you all think? I think it is not worth it. - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 02] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
> This PR aims to address the following issue: JDK-8088198 Exception thrown > from snapshot if dimensions are larger than max texture size > > In order to do that, it simply captures snapshots in multiple tiles of > maxTextureSize^2 dimensions (or less, as needed), and then recomposes all the > tiles into a a single image. > Other than that, the logic used to do the actual snapshot is unchanged. > > Tests using the existing SnapshotCommon class have been added in a new file > named Snapshot3Test under SystemTest/test/javafx/scene. > These tests pass with the proposed fix, and fail without, throwing " > java.lang.IllegalArgumentException: Unrecognized image loader: null" The pull request has been updated with 2 additional commits. - Added commits: - 50191728: Several changes following code review - d4ecb734: Removed Snapshot3Test.java and re-enabled ignored tests in Snapshot2Test.java Changes: - all: https://git.openjdk.java.net/jfx/pull/68/files - new: https://git.openjdk.java.net/jfx/pull/68/files/9986809d..50191728 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/68/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/68/webrev.01-02 Stats: 82 lines in 3 files changed: 1 ins; 71 del; 10 mod Patch: https://git.openjdk.java.net/jfx/pull/68.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/68/head:pull/68 PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 01] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Thu, 16 Jan 2020 11:56:10 GMT, Michael Paus wrote: >> Assuming `Nb` in `verticalTileNb` stands for number, I would recommend to >> change the names as `numVerticalTiles` and `numHorizontalTiles` > > I think the proposed code changes are wrong in case that `height / > maxTextureSize` is an exact integer. (Same for width). I normally add the 1 > only if `height % maxTextureSize != 0` Indeed, substituting the floating math for integer math requires an extra test on the remainder to be correct. Since this code isn't located in a tight loop, I'm not sure this optimization is worth making the code less straight forward. What do you all think? - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 01] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Thu, 16 Jan 2020 11:28:21 GMT, Ambarish Rapte wrote: >> The pull request has been updated with 1 additional commit. > > The fix itself looks good and seems safe for 14. > I do have few minor changes in test file and suggestions in fix code, please > take a look. > Also verified that large sized snapshots are created correctly. > > > Upon closer inspection, I believe that the tests I added in Snapshot3Test are > indeed redundant and less complete than the 4 ignored test in Snapshot2Test. > I therefore propose to remove Snapshot3Test.java entirely and to re-enable > the 4 testSnapshotBig* test instead. I did miss this comment, The test in `Snapshot2Test` look sufficient and you can get rid of `Snapshot3Test.java` and so all my comments :) - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 01] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Thu, 16 Jan 2020 10:55:38 GMT, Ambarish Rapte wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1306:
>>
>>> 1305: int verticalTileNb = (int) Math.ceil(height / (double)
>>> maxTextureSize);
>>> 1306: int horizontalTileNb = (int) Math.ceil(width / (double)
>>> maxTextureSize);
>>> 1307: for (int i = 0; i < horizontalTileNb; i++) {
>>
>> A suggestion for this arithmetic.
>> int verticalTileNb = height / maxTextureSize + 1;
>> int horizontalTileNb = width / maxTextureSize + 1;
>> This will avoid the type casting and floating point operations. However I
>> leave it to you to change or not.
>
> Assuming `Nb` in `verticalTileNb` stands for number, I would recommend to
> change the names as `numVerticalTiles` and `numHorizontalTiles`
I think the proposed code changes are wrong in case that `height /
maxTextureSize` is an exact integer. (Same for width). I normally add the 1
only if `height % maxTextureSize != 0`
-
PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 01] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Thu, 16 Jan 2020 11:28:46 GMT, Frederic Thevenet
wrote:
>> This PR aims to address the following issue: JDK-8088198 Exception thrown
>> from snapshot if dimensions are larger than max texture size
>>
>> In order to do that, it simply captures snapshots in multiple tiles of
>> maxTextureSize^2 dimensions (or less, as needed), and then recomposes all
>> the tiles into a a single image.
>> Other than that, the logic used to do the actual snapshot is unchanged.
>>
>> Tests using the existing SnapshotCommon class have been added in a new file
>> named Snapshot3Test under SystemTest/test/javafx/scene.
>> These tests pass with the proposed fix, and fail without, throwing "
>> java.lang.IllegalArgumentException: Unrecognized image loader: null"
>
> The pull request has been updated with 1 additional commit.
The fix itself looks good and seems safe for 14.
I do have few minor changes in test file and suggestions in fix code, please
take a look.
Also verified that large sized snapshots are created correctly.
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1303:
> 1302: if (height > maxTextureSize || width > maxTextureSize) {
> 1303: // The requested size for the screenshot is to big to fit a
> single texture,
> 1304: // so we need to take several snapshot tiles and merge them
> into single image (fixes JDK-8088198)
Should be `too` big to fit
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1306:
> 1305: int verticalTileNb = (int) Math.ceil(height / (double)
> maxTextureSize);
> 1306: int horizontalTileNb = (int) Math.ceil(width / (double)
> maxTextureSize);
> 1307: for (int i = 0; i < horizontalTileNb; i++) {
A suggestion for this arithmetic.
int verticalTileNb = height / maxTextureSize + 1;
int horizontalTileNb = width / maxTextureSize + 1;
This will avoid the type casting and floating point operations. However I leave
it to you to change or not.
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1312:
> 1311: int tileWidth = Math.min(maxTextureSize, width -
> xOffset);
> 1312: int tileHeight = Math.min(maxTextureSize, height -
> yOffset);
> 1313: WritableImage tile = doSnapshotTile(scene, xMin +
> xOffset, yMin + yOffset, tileWidth, tileHeight, root, transform, depthBuffer,
> fill, camera, null);
`int xOffset = i * maxTextureSize;`
`int tileWidth = Math.min(maxTextureSize, width - xOffset);`
These two lines should be moved to the outer loop of horizontal tabs.
tests/system/src/test/java/test/javafx/scene/Snapshot3Test.java line 1:
> 1: package test.javafx.scene;
> 2:
Copyright header should be added to all new files. You can copy the header from
any other file ([header
sample](https://github.com/openjdk/jfx/blob/20325e1c3ec4c4e81af74d3d43bf3a803dbe1a51/tests/system/src/test/java/test/javafx/scene/Snapshot2Test.java#L1).)
Only change would be that, this file would contain only year `2020` in the
copyright header.
tests/system/src/test/java/test/javafx/scene/Snapshot3Test.java line 39:
> 38: public void teardownEach() {
> 39: }
> 40:
This empty method is not needed.
tests/system/src/test/java/test/javafx/scene/Snapshot3Test.java line 65:
> 64:
> 65:
Please remove the extra empty lines, 3, 62, 65.
tests/system/src/test/java/test/javafx/scene/Snapshot3Test.java line 59:
> 58: Image image = rect.snapshot(params, null);
> 59: assertEquals(VALUE_LARGER_THAN_TEXTURE_SIZE, (int)
> image.getHeight());
> 60: });
Should add `assertNull(image);` before the `assertEquals`
tests/system/src/test/java/test/javafx/scene/Snapshot3Test.java line 11:
> 10:
> 11: import static org.junit.Assert.*;
> 12:
Usually we avoid wild imports and import only the specific classes.
-
Changes requested by arapte (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 01] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Thu, 16 Jan 2020 10:55:11 GMT, Ambarish Rapte wrote:
>> The pull request has been updated with 1 additional commit.
>
> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1306:
>
>> 1305: int verticalTileNb = (int) Math.ceil(height / (double)
>> maxTextureSize);
>> 1306: int horizontalTileNb = (int) Math.ceil(width / (double)
>> maxTextureSize);
>> 1307: for (int i = 0; i < horizontalTileNb; i++) {
>
> A suggestion for this arithmetic.
> int verticalTileNb = height / maxTextureSize + 1;
> int horizontalTileNb = width / maxTextureSize + 1;
> This will avoid the type casting and floating point operations. However I
> leave it to you to change or not.
Assuming `Nb` in `verticalTileNb` stands for number, I would recommend to
change the names as `numVerticalTiles` and `numHorizontalTiles`
-
PR: https://git.openjdk.java.net/jfx/pull/68
Re: DnD Move does not work on OS-X
I filed https://bugs.openjdk.java.net/browse/JDK-8237329 Tom On 09.01.20 15:48, Tom Schindl wrote: > I also tried with > https://docs.oracle.com/javafx/2/drag_drop/jfxpub-drag_drop.htm and it > does not work either. -- Tom Schindl, CTO BestSolution.at EDV Systemhaus GmbH Salurnerstrasse 15. A-6020 Innsbruck Reg. Nr. FN 222302s am Firmenbuchgericht Innsbruck
