RFR: 8272808: Update constant collections to use the new immutable collections - leftovers
Replacements of more immutable collections. One thing I found was that the field `idMap` in `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it if that's indeed the case. - Commit messages: - Fix a previous change - More leftovers - Unified style - Fix mistake from previous commit - Replace map in JSCL - Initial commit of 2 files Changes: https://git.openjdk.java.net/jfx/pull/627/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=627=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272808 Stats: 162 lines in 7 files changed: 0 ins; 28 del; 134 mod Patch: https://git.openjdk.java.net/jfx/pull/627.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/627/head:pull/627 PR: https://git.openjdk.java.net/jfx/pull/627
[jfx17u] Integrated: 8269374: Menu inoperable after setting stage to second monitor
On Wed, 22 Sep 2021 19:25:41 GMT, Johan Vos wrote: > Clean backport of JDK-8269374 (already in 11.0.13) > > Reviewed-by: kcr, arapte This pull request has now been integrated. Changeset: 483e53b4 Author:Johan Vos URL: https://git.openjdk.java.net/jfx17u/commit/483e53b4763b384532f748a71a282c07ca133208 Stats: 9 lines in 1 file changed: 9 ins; 0 del; 0 mod 8269374: Menu inoperable after setting stage to second monitor Backport-of: c490ddfdb1255add00dd7b0f14fe03857c6946c5 - PR: https://git.openjdk.java.net/jfx17u/pull/12
[jfx17u] Integrated: 8273754: Re-introduce Automatic-Module-Name in empty jars
On Wed, 22 Sep 2021 19:21:27 GMT, Johan Vos wrote: > Clean backport of JDK-8273754 which also appears in hotfix build 17.0.0.1 > > Reviewed-by: kcr, jvos This pull request has now been integrated. Changeset: 9bd87081 Author:Johan Vos URL: https://git.openjdk.java.net/jfx17u/commit/9bd87081be18e787d7fec41d9cb5c6db7544c932 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod 8273754: Re-introduce Automatic-Module-Name in empty jars Backport-of: 7329279e504070355ba9aa0517f0279e7e72c2cf - PR: https://git.openjdk.java.net/jfx17u/pull/11
[jfx17u] RFR: 8269374: Menu inoperable after setting stage to second monitor
Clean backport of JDK-8269374 (already in 11.0.13) Reviewed-by: kcr, arapte - Commit messages: - 8269374: Menu inoperable after setting stage to second monitor Changes: https://git.openjdk.java.net/jfx17u/pull/12/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx17u=12=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269374 Stats: 9 lines in 1 file changed: 9 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx17u/pull/12.diff Fetch: git fetch https://git.openjdk.java.net/jfx17u pull/12/head:pull/12 PR: https://git.openjdk.java.net/jfx17u/pull/12
[jfx17u] RFR: 8273754: Re-introduce Automatic-Module-Name in empty jars
Clean backport of JDK-8273754 which also appears in hotfix build 17.0.0.1 Reviewed-by: kcr, jvos - Commit messages: - 8273754: Re-introduce Automatic-Module-Name in empty jars Changes: https://git.openjdk.java.net/jfx17u/pull/11/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx17u=11=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273754 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx17u/pull/11.diff Fetch: git fetch https://git.openjdk.java.net/jfx17u pull/11/head:pull/11 PR: https://git.openjdk.java.net/jfx17u/pull/11
Re: RFR: 8264591: HBox/VBox child widths pixel-snap to wrong value
On Mon, 29 Mar 2021 19:57:17 GMT, Michael Strauß wrote: > The children of HBox/VBox don't always pixel-snap to the same value as the > container itself when a render scale other than 1 is used. This can lead to a > visual glitch where the content bounds don't line up with the container > bounds. In this case, the container will extend beyond its content, or vice > versa. > > The following program shows the problem for HBox: > > Region r1 = new Region(); > Region r2 = new Region(); > Region r3 = new Region(); > Region r4 = new Region(); > Region r5 = new Region(); > Region r6 = new Region(); > r1.setBackground(new Background(new BackgroundFill(Color.GREY, null, null))); > r2.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, > null))); > r3.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null))); > r4.setBackground(new Background(new BackgroundFill(Color.GREY, null, null))); > r5.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, > null))); > r6.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null))); > r1.setPrefWidth(25.3); > r2.setPrefWidth(25.3); > r3.setPrefWidth(25.4); > r4.setPrefWidth(25.3); > r5.setPrefWidth(25.3); > r6.setPrefWidth(25.4); > r1.setMaxHeight(30); > r2.setMaxHeight(30); > r3.setMaxHeight(30); > r4.setMaxHeight(30); > r5.setMaxHeight(30); > r6.setMaxHeight(30); > HBox hbox1 = new HBox(r1, r2, r3, r4, r5, r6); > hbox1.setBackground(new Background(new BackgroundFill(Color.RED, null, > null))); > hbox1.setPrefHeight(40); > > r1 = new Region(); > r2 = new Region(); > r3 = new Region(); > r4 = new Region(); > r5 = new Region(); > r6 = new Region(); > r1.setBackground(new Background(new BackgroundFill(Color.GREY, null, null))); > r2.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, > null))); > r3.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null))); > r4.setBackground(new Background(new BackgroundFill(Color.GREY, null, null))); > r5.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, > null))); > r6.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null))); > r1.setPrefWidth(25.3); > r2.setPrefWidth(25.3); > r3.setPrefWidth(25.4); > r4.setPrefWidth(25.3); > r5.setPrefWidth(25.3); > r6.setPrefWidth(25.4); > r1.setMaxHeight(30); > r2.setMaxHeight(30); > r3.setMaxHeight(30); > r4.setMaxHeight(30); > r5.setMaxHeight(30); > r6.setMaxHeight(30); > HBox hbox2 = new HBox(r1, r2, r3, r4, r5, r6); > hbox2.setBackground(new Background(new BackgroundFill(Color.RED, null, > null))); > hbox2.setPrefHeight(40); > hbox2.setPrefWidth(152); > > VBox root = new VBox(new HBox(hbox1), new HBox(hbox2)); > root.setSpacing(20); > Scene scene = new Scene(root, 500, 250); > > primaryStage.setScene(scene); > primaryStage.show(); > > > Here's a before-and-after comparison of the program above after applying the > fix. Note that 'before', the backgrounds of the container (red) and its > content (black) don't align perfectly. The render scale is 175% in this > example. >  > > I've filed a bug report and will change the title of the GitHub issue as soon > as it's posted. I had a look at the PR. But it took quite a while to fully understand the changes (also the current implementation ). I think it make sense to improve it a bit e.g. by adding javadoc to the new methods, maybe also the existing? Other ideas are also welcome. With that said maybe more people will review it then. I will have a full look soon as well. :) - PR: https://git.openjdk.java.net/jfx/pull/445
Re: RFR: 8271474: Tree-/TableCell: inconsistent edit event firing pattern
On Tue, 7 Sep 2021 14:53:50 GMT, Jeanette Winzenburg wrote: > this PR fixes the inconsistent event firing pattern in cell's xxEdit methods > (please see the issue for more details): > > - fires event if column != null > - accesses table state if table != null > > The first requires a change in CellEditEvent which now allows null table in > its constructor. > > Added tests that failed/passed before/after the fix (along with some that > also passed before for complete coverage of state permutations). Changed two > old test methods which passed/failed before/after the fix - but did test the > wrong thingy anyway ;) Looks good to me. - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/620
Re: RFR: 8271054: [REDO] Wrong stage gets focused after modal stage creation [v3]
> Found the problem thru this path:
>
> **WindowStage.java**
>
> final void handleFocusDisabled() {
> if (activeWindows.isEmpty()) {
> return;
> }
> WindowStage window = activeWindows.get(activeWindows.size() - 1);
> window.setIconified(false);
> window.requestToFront();
> window.requestFocus();
> }
>
>
> **glass_window.cpp**
>
> void WindowContextBase::process_focus(GdkEventFocus* event) {
>...
>
> if (jwindow) {
> if (!event->in || isEnabled()) {
> mainEnv->CallVoidMethod(jwindow, jWindowNotifyFocus,
> event->in ? com_sun_glass_events_WindowEvent_FOCUS_GAINED
> : com_sun_glass_events_WindowEvent_FOCUS_LOST);
> CHECK_JNI_EXCEPTION(mainEnv)
> } else {
> mainEnv->CallVoidMethod(jwindow, jWindowNotifyFocusDisabled);
> CHECK_JNI_EXCEPTION(mainEnv)
> }
> }
> }
>
>
> So `glass_window.cpp` was triggering `jWindowNotifyFocusDisabled` which
> triggered the java code on the Primary Stage (on the bug reproduce code).
>
> The docs states:
>
> /**
> * Enables or disables the window.
> *
> * A disabled window is unfocusable by definition.
> * Also, key or mouse events aren't generated for disabled windows.
> *
> * When a user tries to activate a disabled window, or the window gets
> * accidentally brought to the top of the stacking order, the window
> * generates the FOCUS_DISABLED window event. A Glass client should react
> * to this event and bring the currently active modal blocker of the
> * disabled window to top by calling blocker's minimize(false), toFront(),
> * and requestFocus() methods. It may also 'blink' the blocker window to
> * further attract user's attention.
> *
> .
>
>
> So I guess the C++ side looks ok, java side on `handleFocusDisabled` I did
> not understand why it `requestToFront` and `requestFocus` on the latest
> window.
>
> The solution makes disabled windows unfocusable and prevents mouse and
> keyboard events as the docs states, making it compatible with other platforms.
Thiago Milczarek Sayao has updated the pull request incrementally with one
additional commit since the last revision:
Break if reach self
-
Changes:
- all: https://git.openjdk.java.net/jfx/pull/598/files
- new: https://git.openjdk.java.net/jfx/pull/598/files/850cfc8b..eb93b8b9
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jfx=598=02
- incr: https://webrevs.openjdk.java.net/?repo=jfx=598=01-02
Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jfx/pull/598.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/598/head:pull/598
PR: https://git.openjdk.java.net/jfx/pull/598
Re: RFR: 8273946: Move clearQuad method to BaseShaderGraphics superclass
On Wed, 22 Sep 2021 13:59:43 GMT, Nir Lisker wrote:
> > We added `@SuppressWarnings("removal")` annotations for JDK-8264139, so I'm
> > surprised the IDE is still flagging it.
>
> The warning is on the `import` of the class, which can be suppressed by
> adding the annotation to the class declaration. Maybe it's only this way in
> the Eclipse compiler.
This sounds like an Eclipse problem then. In any case, I would not consider
adding annotations to the classes. We went to some effort to make the scope of
the annotations as local as possible (as did the JDK).
> > We could consider a follow-up issue to clean this up, but I'm not really
> > inclined to change anything.
>
> The question is if this can cause any bugs? Is the method supposed to
> override? If yes and it isn't, it sounds like something could go wrong.
It isn't intended to be an override. The context parameter is passed up to the
superclass and stored there as well as in the subclass. We could explore a
different design pattern, but I don't think it is necessary.
-
PR: https://git.openjdk.java.net/jfx/pull/628
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight
On Mon, 20 Sep 2021 14:11:50 GMT, Kevin Rushforth wrote: >> @kevinrushforth >>> The fix looks good. I tested it both in isolation and with PR #334 and it >>> works on both a retina and non-retina display. >>> >>> If you have time to write an automated test, that would be useful, but if >>> not then a manual test would be OK. >> >> Ok, I will try to write an automated test case which draws a sphere in a >> SubScene and then calculates the average color of the generated image. The >> test will be passed if the calculated average does not differ from the >> excepted average color by a certain tolerance value. I'm not sure if I will >> manage to do this... but I will give it a try. > > @andreas-heger are you able to come up with an automated test for this bug? @kevinrushforth yes, I still want to write an automated test for this bug. Sorry for the delay... I plan to do it within the next two weeks. - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8273946: Move clearQuad method to BaseShaderGraphics superclass
On Tue, 21 Sep 2021 12:45:20 GMT, Kevin Rushforth wrote:
> As mentioned in JBS, the `clearQuad` methods in `D3DGraphics` and
> `ES2Graphics` are now identical, which should have been the case all along.
> The fact that they weren't identical was the source of a bug that was only
> discovered during the testing of
> [JDK-8090547](https://bugs.openjdk.java.net/browse/JDK-8090547) on the es2
> pipeline.
>
> This PR moves the `clearQuad` method to the `BaseShaderGraphics` superclass
> to get rid of the unnecessary code duplication. This will be helpful when
> implementing the metal pipeline as well. As a note, I made the `context`
> field in the `D3DGraphics` final, which it should have been. This is
> necessary to ensure that moving the method to the super-class is equivalent.
>
> No new tests are needed, since this is just refactoring.
> We added `@SuppressWarnings("removal")` annotations for JDK-8264139, so I'm
> surprised the IDE is still flagging it.
The warning is on the `import` of the class, which can be suppressed by adding
the annotation to the class declaration. Maybe it's only this way in the
Eclipse compiler.
> We could consider a follow-up issue to clean this up, but I'm not really
> inclined to change anything.
The question is if this can cause any bugs? Is the method supposed to override?
If yes and it isn't, it sounds like something could go wrong.
-
PR: https://git.openjdk.java.net/jfx/pull/628
Integrated: 8273946: Move clearQuad method to BaseShaderGraphics superclass
On Tue, 21 Sep 2021 12:45:20 GMT, Kevin Rushforth wrote: > As mentioned in JBS, the `clearQuad` methods in `D3DGraphics` and > `ES2Graphics` are now identical, which should have been the case all along. > The fact that they weren't identical was the source of a bug that was only > discovered during the testing of > [JDK-8090547](https://bugs.openjdk.java.net/browse/JDK-8090547) on the es2 > pipeline. > > This PR moves the `clearQuad` method to the `BaseShaderGraphics` superclass > to get rid of the unnecessary code duplication. This will be helpful when > implementing the metal pipeline as well. As a note, I made the `context` > field in the `D3DGraphics` final, which it should have been. This is > necessary to ensure that moving the method to the super-class is equivalent. > > No new tests are needed, since this is just refactoring. This pull request has now been integrated. Changeset: 338b9994 Author:Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/338b999414ed5dc8fea19c12bd3e656fd9cab6f4 Stats: 59 lines in 3 files changed: 18 ins; 40 del; 1 mod 8273946: Move clearQuad method to BaseShaderGraphics superclass Reviewed-by: jpereda, nlisker - PR: https://git.openjdk.java.net/jfx/pull/628
Re: RFR: 8273946: Move clearQuad method to BaseShaderGraphics superclass
On Tue, 21 Sep 2021 12:45:20 GMT, Kevin Rushforth wrote:
> As mentioned in JBS, the `clearQuad` methods in `D3DGraphics` and
> `ES2Graphics` are now identical, which should have been the case all along.
> The fact that they weren't identical was the source of a bug that was only
> discovered during the testing of
> [JDK-8090547](https://bugs.openjdk.java.net/browse/JDK-8090547) on the es2
> pipeline.
>
> This PR moves the `clearQuad` method to the `BaseShaderGraphics` superclass
> to get rid of the unnecessary code duplication. This will be helpful when
> implementing the metal pipeline as well. As a note, I made the `context`
> field in the `D3DGraphics` final, which it should have been. This is
> necessary to ensure that moving the method to the super-class is equivalent.
>
> No new tests are needed, since this is just refactoring.
Thanks for the review.
> Two unrelated comments that I have observed during the review:
>
> 1. `java.security.AccessController` is deprecated for removal since 17. We
> need to do something about this. It's used in `BaseShaderGraphics`, which is
> where I saw the warning, but it's used in other places too.
We added `@SuppressWarnings("removal")` annotations for
[JDK-8264139](https://bugs.openjdk.java.net/browse/JDK-8264139), so I'm
surprised the IDE is still flagging it. FWIW, it will be several years before
we can remove any of these calls, since running with a security manager is
still supported in JDK 17 LTS.
> 2. Eclipse graciously informs me that "The method `D3DGraphics.getContext()`
> does not override the inherited method from `BaseShaderGraphics` since it is
> private to a different package". It detected the `@Override` annotation there
> and noted that the superclass method is not visible to this class since they
> are in different packages, so it is not actually overriding. This could lead
> to another subtle bug. Maybe that method should be protected. I didn't
> investigate much further.
Interesting. I guess the only reason the IDE is warning in this case is that it
notices the `@Override` annotation, which is there because it implements the
method in D3DGraphicsContextSource, and warns that it doesn't override the
method of the same name in the superclass. We could consider a follow-up issue
to clean this up, but I'm not really inclined to change anything.
-
PR: https://git.openjdk.java.net/jfx/pull/628
Re: RFR: 8273969: Memory Leak on the Runnable provided to Platform.startup [v2]
On Wed, 22 Sep 2021 10:18:09 GMT, Florian Kirmaier wrote: >> Probably my most boring PR. ;) >> Setting the lambda to null, after it has been used, fixes the leak. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8273969 > Some changes based on code review Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/626
Integrated: 8090158: Wrong implementation of adjustValue in scrollBars
On Tue, 20 Jul 2021 09:12:05 GMT, Hadzic Samir wrote: > JBS bug: [JDK-8090158](https://bugs.openjdk.java.net/browse/JDK-8090158) > > The javadoc of the ScrollBar#adjustValue specifically says that it will > adjust the value based on the block increment value. Therefore, there is no > reason to stop at the given value when reaching an end. This pull request has now been integrated. Changeset: 5c355fa2 Author:Hadzic Samir Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/5c355fa231e862a9db9b524cf14715066df142f5 Stats: 32 lines in 2 files changed: 28 ins; 3 del; 1 mod 8090158: Wrong implementation of adjustValue in scrollBars Reviewed-by: aghaisas, kcr - PR: https://git.openjdk.java.net/jfx/pull/582
Re: RFR: 8273969: Memory Leak on the Runnable provided to Platform.startup [v2]
> Probably my most boring PR. ;) > Setting the lambda to null, after it has been used, fixes the leak. Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision: JDK-8273969 Some changes based on code review - Changes: - all: https://git.openjdk.java.net/jfx/pull/626/files - new: https://git.openjdk.java.net/jfx/pull/626/files/7897fd64..9620ed5e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=626=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=626=00-01 Stats: 103 lines in 2 files changed: 55 ins; 48 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/626.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/626/head:pull/626 PR: https://git.openjdk.java.net/jfx/pull/626
Re: RFR: 8273969: Memory Leak on the Runnable provided to Platform.startup [v2]
On Mon, 20 Sep 2021 12:56:39 GMT, Kevin Rushforth wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8273969
>> Some changes based on code review
>
> tests/system/src/test/java/test/javafx/scene/PlatformStartupMemoryLeakTest.java
> line 26:
>
>> 24: */
>> 25:
>> 26: package test.javafx.scene;
>
> Similar tests are in the `test.com.sun.javafx.application` package, so I
> recommend putting this test there.
done
> tests/system/src/test/java/test/javafx/scene/PlatformStartupMemoryLeakTest.java
> line 34:
>
>> 32: public class PlatformStartupMemoryLeakTest {
>> 33:
>> 34: @Test
>
> Since this test calls `Platform::startup`, this class must only have a single
> `@Test` method. I recommend adding a comment to that effect, so that someone
> doesn't try to add a second test method later.
I think that's redundant after moving it to the application package.
In the application package, most of the test classes only have 1 test for this
reason.
> tests/system/src/test/java/test/javafx/scene/PlatformStartupMemoryLeakTest.java
> line 37:
>
>> 35: public void testStartupLeak() {
>> 36: JMemoryBuddy.memoryTest((checker) -> {
>> 37: // Doesn't work as lambda for some reason, due to
>> "BoundMethodHandle$Species_L"
>
> Based on Tom's comment, and also my reading of the spec, this is expected (if
> somewhat counter-intuitive), so please modify this comment to indicate that
> the `Runnable` must not be turned into a lambda.
done
> tests/system/src/test/java/test/javafx/scene/PlatformStartupMemoryLeakTest.java
> line 47:
>
>> 45: checker.assertCollectable(r);
>> 46: });
>> 47: }
>
> I recommend adding an `@After` (or `@AfterClass`) cleanup method that calls
> `Platform::exit`.
done
-
PR: https://git.openjdk.java.net/jfx/pull/626
