Re: RFR: 8264591: HBox/VBox child widths pixel-snap to wrong value
On Wed, 22 Sep 2021 18:48:13 GMT, Marius Hanl wrote: > 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. :) I've added a bit of documentation. - PR: https://git.openjdk.java.net/jfx/pull/445
Re: RFR: 8264591: HBox/VBox child widths pixel-snap to wrong value [v2]
> 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. Michael Strauß 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 four additional commits since the last revision: - added documentation, improved method names - Merge branch 'master' into fixes/box-snap-to-pixel - Fix pixel-snapping glitches in VBox/HBox - Failing test - Changes: - all: https://git.openjdk.java.net/jfx/pull/445/files - new: https://git.openjdk.java.net/jfx/pull/445/files/c218b809..744f19f5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=445&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=445&range=00-01 Stats: 449813 lines in 8109 files changed: 251439 ins; 126257 del; 72117 mod Patch: https://git.openjdk.java.net/jfx/pull/445.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/445/head:pull/445 PR: https://git.openjdk.java.net/jfx/pull/445
RFR: 8275723: Crash on macOS 12 in GlassRunnable::dealloc
GlassRunnable uses jni environment (jEnv) associated with the main application
thread both for run() and dealloc() methods. Both these methods are supposed to
be scheduled for execution on the main thread:
if (jEnv != NULL)
{
GlassRunnable *runnable = [[GlassRunnable alloc]
initWithRunnable:(*env)->NewGlobalRef(env, jRunnable)];
[runnable performSelectorOnMainThread:@selector(run) withObject:nil
waitUntilDone:NO];
}
However, it appears that on macOS 12 only the run() method is executed the main
thread, whereas the dealloc() method is executed on the InvokeLaterDispatcher
thread, that leads to usage of the main thread jni env in the context of
another thread. This problem is more visible on aarch64, where the thread check
is triggered by the W^X machinery, but the problem is present on x64 as well.
Proposed fix just encapsulates all jni-related work in the run() method,
reducing risks to misuse the jni environment of the main thread.
-
Commit messages:
- 8275723: Crash on macOS 12 in GlassRunnable::dealloc
Changes: https://git.openjdk.java.net/jfx/pull/661/files
Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=661&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8275723
Stats: 18 lines in 1 file changed: 4 ins; 13 del; 1 mod
Patch: https://git.openjdk.java.net/jfx/pull/661.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/661/head:pull/661
PR: https://git.openjdk.java.net/jfx/pull/661
Re: RFR: Draft - 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v3]
On Mon, 1 Nov 2021 12:53:43 GMT, Jeanette Winzenburg wrote: >> Cleanup of Tree-/TableRowSkin to support switching skins >> >> The misbehavior/s >> - memory leaks due to manually registered listeners that were not removed >> - side-effects due to listeners still active on old skin (like NPEs) >> >> Fix >> - use skin api for all listener registration (for automatic removal in >> dispose) >> - do not install listeners that are not needed (fixedCellSize, same as in >> fix of ListCellSkin >> [JDK-8246745](https://bugs.openjdk.java.net/browse/JDK-8246745)) >> >> Added tests for each listener involved in the fix to guarantee it's still >> working and does not have unwanted side-effects after switching skins. >> >> Note: there are pecularities in row skins (like not updating themselves on >> property changes of its control, throwing NPEs when not added to a >> VirtualFlow) which are not part of this issue but covered in >> [JDK-8274065](https://bugs.openjdk.java.net/browse/JDK-8274065) > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > removed unused code, fixed doc of test methods Changed to draft because just noted an issue with initial horizontal scrollBar for many columns and fixedCellSize which I don't quite understand yet. - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274854: Mnemonics for menu containing numeric text not working [v4]
On Sat, 30 Oct 2021 20:40:12 GMT, Michael Strauß wrote: >> This PR fixes an issue with mnemonic parsing by removing the restriction >> that a mnemonic symbol must be a letter. Now, it can be any character except >> whitespace. > > Michael Strauß has updated the pull request incrementally with four > additional commits since the last revision: > > - revert rename > - revert rename > - test for KeyCombination > - renamed TextBinding to MnemonicParser, refactored tests The latest version looks good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/647
Integrated: 8276179 PrismFontFile.isInstalledFont is dead code and should be removed
On Fri, 29 Oct 2021 15:10:35 GMT, Florian Kirmaier wrote: > removing dead code. > When looking into the font code, I've noticed that this code is no longer > used, so I thought I should make a PR with a minor cleanup. This pull request has now been integrated. Changeset: cde72c8e Author:Florian Kirmaier Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/cde72c8e2988f267230625d976fff85576858766 Stats: 34 lines in 2 files changed: 0 ins; 34 del; 0 mod 8276179: PrismFontFile.isInstalledFont is dead code and should be removed Reviewed-by: prr, kcr - PR: https://git.openjdk.java.net/jfx/pull/658
Re: RFR: 8273485: Deadlock when also using Swing and exiting Fullscreen on Mac [v7]
On Mon, 18 Oct 2021 08:20:22 GMT, Florian Kirmaier wrote: >> When using Swing it's possible to generate a Deadlock. >> It's related to the nested eventloop started in enterFullScreenExitingLoop >> - and the RenderLock aquired when using setView in Scene. >> Sample Programm and Threaddump are added to the ticket. >> >> Removing the nested loop fixes the Problem. >> I hope this doesn't have any side effect - so far i don't know of any. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8273485 > Fixing toggle fullscreen! I've been using macOS 11.6. It is also not so easy for me to test another version. I guess the exception can easily be "fixed" by checking whether the application is null, in the `Screen.initScreens` method. Should I change it this way? - PR: https://git.openjdk.java.net/jfx/pull/622
Re: RFR: 8273485: Deadlock when also using Swing and exiting Fullscreen on Mac [v7]
On Sat, 30 Oct 2021 13:35:39 GMT, Kevin Rushforth wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8273485
>> Fixing toggle fullscreen!
>
> modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line
> 1345:
>
>> 1343: if (self->nativeFullScreenModeWindow)
>> 1344: {
>> 1345: [[self->nsView window] toggleFullScreen:self];
>
> I see you added the call to `toggleFullScreen` back in, but you now pass
> `self` to the method, where the previous code passed `nil`. Unless there is a
> compelling reason why you need to change it, I recommend to restore _exactly_
> the former code, such that there are no diffs:
>
>
> [self->nativeFullScreenModeWindow
> performSelector:@selector(toggleFullScreen:) withObject:nil];
With the previous version, I get a "beep" sound when closing a window.
But with the changed line, it works for me and I don't get any issues.
The beep sound also was the original reason, why I've removed the line.
-
PR: https://git.openjdk.java.net/jfx/pull/622
RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height
This PR fixes an issue where the scroll delta of ScrollPane incorrectly depends on the size of its content. This leads to extremely slow scrolling when the content is only slightly larger than the ScrollPane. - Commit messages: - Fixed scrolling speed for ScrollPaneSkin - failing test Changes: https://git.openjdk.java.net/jfx/pull/660/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=660&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276313 Stats: 52 lines in 2 files changed: 49 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/660.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/660/head:pull/660 PR: https://git.openjdk.java.net/jfx/pull/660
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v3]
On Tue, 2 Nov 2021 10:15:41 GMT, Florian Kirmaier wrote: >> After thinking about this issue for some time, I've now got a solution. >> I know put the scene in the state it is, before is was shown, when the >> dirtyNodes are unset, the whole scene is basically considered dirty. >> This has the drawback of rerendering, whenever a window is "reshown", but it >> restores sanity about memory behaviour, which should be considered more >> important. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8269907 > We now require the rendering lock when cleaning up dirty nodes. To do so, > we moved some code required for snapshot into a reusable method. Thank you for your feedback! To solidify my understanding - the "RenderScenegraph" is only allowed to be changed when the render lock is held. This scene graph is represented by all these NG classes, correct? I've now updated my PR. I found some code for snapshot, which solves the same problem about aquiring the render lock. I've moved it into an own method and used it for my fix. The regular sync, and the new sync shouldn't happen in the same pulse - because the regular sync checks whether the Window is closed by `if (peer != null)`. At least it shouldn't require the lock. If it would for some reason aquire the lock, then it should basically be a noop - so i don't really see why this would be an issue. Performance wise aquiring an additional lock can cause a very minor slowdown, but because it only happens once per closed window, it is realy minor. - PR: https://git.openjdk.java.net/jfx/pull/584
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v3]
> After thinking about this issue for some time, I've now got a solution. > I know put the scene in the state it is, before is was shown, when the > dirtyNodes are unset, the whole scene is basically considered dirty. > This has the drawback of rerendering, whenever a window is "reshown", but it > restores sanity about memory behaviour, which should be considered more > important. Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision: JDK-8269907 We now require the rendering lock when cleaning up dirty nodes. To do so, we moved some code required for snapshot into a reusable method. - Changes: - all: https://git.openjdk.java.net/jfx/pull/584/files - new: https://git.openjdk.java.net/jfx/pull/584/files/c603d2b1..28b793b5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=584&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=584&range=01-02 Stats: 7 lines in 1 file changed: 5 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/584.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/584/head:pull/584 PR: https://git.openjdk.java.net/jfx/pull/584
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v3]
On Tue, 2 Nov 2021 08:16:41 GMT, Florian Kirmaier wrote: >> This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17. >> To fix it, I've made the Value of the WeakHashMap also weak. >> We only keep this value to remove it as a listener later on. Therefore there >> shouldn't be issues by making this value weak. >> >> >> I've seen this Bug very very often, in the last weeks. Most of the >> applications I've seen are somehow affected by this bug. >> >> It basically **breaks every application with menu bars and multiple stages** >> - which is the majority of enterprise applications. It's especially annoying >> because it takes some time until the application gets unstable. >> >> Therefore **I would recommend** after this fix is approved, **to make a new >> version for JavaFX17** with this fix because this bug is so severe. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > 8274022 > Simplified code related to WeakHashMaps Marked as reviewed by mstrauss (Author). - PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v3]
> This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17. > To fix it, I've made the Value of the WeakHashMap also weak. > We only keep this value to remove it as a listener later on. Therefore there > shouldn't be issues by making this value weak. > > > I've seen this Bug very very often, in the last weeks. Most of the > applications I've seen are somehow affected by this bug. > > It basically **breaks every application with menu bars and multiple stages** > - which is the majority of enterprise applications. It's especially annoying > because it takes some time until the application gets unstable. > > Therefore **I would recommend** after this fix is approved, **to make a new > version for JavaFX17** with this fix because this bug is so severe. Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision: 8274022 Simplified code related to WeakHashMaps - Changes: - all: https://git.openjdk.java.net/jfx/pull/659/files - new: https://git.openjdk.java.net/jfx/pull/659/files/402fcd27..bbc39f24 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=659&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=659&range=01-02 Stats: 6 lines in 1 file changed: 2 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/659.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/659/head:pull/659 PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v3]
On Sun, 31 Oct 2021 16:32:49 GMT, Michael Strauß wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8274022
>> Simplified code related to WeakHashMaps
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
> line 88:
>
>> 86: // Remove previously added listener if any
>> 87: if (sceneChangeListenerMap.containsKey(anchor)) {
>> 88: ChangeListener listener =
>> sceneChangeListenerMap.get(anchor).get();
>
> It seems unusual to check for the existence of a weak key (containsKey), and
> then just assume it still exists (get). You should probably get() the value
> directly without checking containsKey() first, and then check whether the
> returned value is null.
Yes, you are right.
This issue also existed in the previous version.
It can't cause problems, because the key is held as a parameter in the method,
and the keys equal method is implemented by comparing references. But that's an
unnecessarily complicated argument.
It's way easier to just make one call without any timing issue.
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
> line 254:
>
>> 252: // at the time of installing the accelerators.
>> 253: if (sceneChangeListenerMap.containsKey(anchor)) {
>> 254: ChangeListener listener =
>> sceneChangeListenerMap.get(anchor).get();
>
> It seems unusual to check for the existence of a weak key (containsKey), and
> then just assume it still exists (get). You should probably get() the value
> directly without checking containsKey() first, and then check whether the
> returned value is null.
The conversation is below in the other reviewed code.
-
PR: https://git.openjdk.java.net/jfx/pull/659
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport [v2]
> This fixes the new ControlAcceleratorBug which was Introduced in JavaFX17. > To fix it, I've made the Value of the WeakHashMap also weak. > We only keep this value to remove it as a listener later on. Therefore there > shouldn't be issues by making this value weak. > > > I've seen this Bug very very often, in the last weeks. Most of the > applications I've seen are somehow affected by this bug. > > It basically **breaks every application with menu bars and multiple stages** > - which is the majority of enterprise applications. It's especially annoying > because it takes some time until the application gets unstable. > > Therefore **I would recommend** after this fix is approved, **to make a new > version for JavaFX17** with this fix because this bug is so severe. Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision: 8274022 fixed some whitesapce formatting issues - Changes: - all: https://git.openjdk.java.net/jfx/pull/659/files - new: https://git.openjdk.java.net/jfx/pull/659/files/324188f1..402fcd27 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=659&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=659&range=00-01 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/659.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/659/head:pull/659 PR: https://git.openjdk.java.net/jfx/pull/659
