On Thu, 16 Jan 2020 16:08:05 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> The pull request has been updated with 2 additional commits.
>
> 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.
I'm not 100% convinced this would really add much to the readability of the
code; I extracted the code from `doSnapshotTile` in its own method because it
is called twice (on both sides of the `if (height > maxTextureSize || width >
maxTextureSize)` condition, actually), but this isn't the case here.
I've got no strong feeling against it either, so I don't know; anybody else
care to comment?
-------------
PR: https://git.openjdk.java.net/jfx/pull/68
