Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Wed, 27 Oct 2021 09:56:46 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: > > re-added forgotten code comments modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 365: > 363: // Fix for RT-27782: Need to set isDirty to true, rather > than the > 364: // cheaper updateCells, as otherwise the text > indentation will not > 365: // be recalculated in > TreeTableCellSkin.leftLabelPadding() Actually this comment is not correct anymore since my PR got merged (https://github.com/openjdk/jfx/pull/568). Instead, it should be `TreeTableCellSkin.calculateIndentation()`. - PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]
On Wed, 27 Oct 2021 09:56:46 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:
>
> re-added forgotten code comments
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
line 299:
> 297: @Override protected ObjectProperty graphicProperty() {
> 298: TreeTableRow treeTableRow = getSkinnable();
> 299: // FIXME: illegal access if skinnable is null
What is the purpose of removing the null check, but leaving getSkinnable() in
there?
Given the signature of the method, it seems that it shouldn't ever return
`null` in any case.
-
PR: https://git.openjdk.java.net/jfx/pull/632
Re: RFR: 8201538: Remove implementation support for applets from JavaFX
On Tue, 31 Aug 2021 16:28:53 GMT, Kevin Rushforth wrote: > This PR removes the obsolete applet implementation from JavaFX. It is an > ongoing maintenance burden to carry around this legacy code. Also, cleaning > this up could help in the implementation of GTK4, Wayland, and Metal, since > we won't have to account for the way applet windows are created and managed. > > ## Notes to reviewers: > > The first part of the removal was to eliminate the methods and classes on the > Java side that are associated with creating and managing an applet window, > and which are no longer called. After these were removed, I then removed the > corresponding methods and classes on the native side that are no longer > called. > > ### Shared Code > > The following were removed from the shared code. > > Removed Java classes > > > com.sun.javafx.tk.AppletWindow > com.sun.javafx.tk.quantum.GlassAppletWindow > > > Removed methods > > The following methods were removed in the parent class and all subclasses. > > > com.sun.glass.ui.Application: > public abstract Window createWindow(long parent) > > com.sun.glass.ui.Window: > public boolean getAppletMode() > public void setAppletMode(boolean appletMode) > public void dispatchNpapiEvent(Map eventInfo) > protected abstract long _createChildWindow(long parent) > protected Window(long parent) > protected abstract int _getEmbeddedX(long ptr) > protected abstract int _getEmbeddedY(long ptr) > > com.sun.javafx.tk.Toolkit: > public abstract AppletWindow createAppletWindow(...) > public abstract void closeAppletWindow() > > com.sun.javafx.tk.quantum.WindowStage: > static void setAppletWindow(GlassAppletWindow aw) > static GlassAppletWindow getAppletWindow() > > > > ### Linux (Gtk) Java code > > The following classes or methods were removed: > > > com.sun.glass.ui.gtk.GtkChildWindow (class removed) > > com.sun.glass.ui.gtk.GtkWindow: > protected GtkWindow(long parent) > > > ### Linux (Gtk) native glass: > > The following native classes were removed: > > > WindowContextChild > WindowContextPlug > > > ### macOS Java code > > The following classes or methods were removed: > > > com.sun.glass.events.mac.NpapiEvent (class removed) > > com.sun.glass.ui.mac.MacApplication: > native protected String _getRemoteLayerServerName() > > com.sun.glass.ui.View: > public int getNativeRemoteLayerId(String serverName) > > com.sun.glass.ui.mac.MacView: > native protected int _getNativeRemoteLayerId(long ptr, String serverName) > native protected void _hostRemoteLayerId(long ptr, int nativeLayerId) > > com.sun.glass.ui.mac.MacWindow: > protected MacWindow(long parent) > > > ### macOS native code > > The following native classes were removed: > > > GlassEmbeddedWindow* > GlassNSEvent > GlassView3D+Remote > RemoteLayerSupport > > > I also removed the `jIsChild` parameter from the window creation code which > allowed for removing a lot of dead blocks of code. The main window creation > method was: > > > - (id)_initWithContentRect:(NSRect)contentRect > styleMask:(NSUInteger)windowStyle > screen:(NSScreen *)screen jwindow:(jobject)jwindow > jIsChild:(jboolean)jIsChild > > > This created a `GlassEmbeddedWindow` iff `jIsChild == JNI_TRUE`. Since > `jIsChild` was only set to true by the (now removed) > `_createChildWindow(long)` method, we can remove the parameter, the > `GlassEmbeddedWindow*` classes, and all code blocks that are qualified by `if > (jIsChild)`. > > ### Windows Java code > > The following classes or methods were removed: > > > com.sun.glass.ui.win.WinChildWindow (class removed) > > com.sun.glass.ui.win.WinWindow: > protected WinWindow(long parent) > > > ### Windows native code > > After removing all references to `IsChild()`, which was only ever true for > `_createChildWindow()`, we can also remove the following: > > > GlassApplication::InstallMouseLLHook > GlassApplication::UninstallMouseLLHook > > > ### iOS Java code > > > com.sun.glass.ui.ios.IosWindow: > protected IosWindow(long parent) > > > ### iOS native code > > With the removal of the `_createChildWindow` method, the following JNI method > in `IosWindow` can be removed: > > > Java_com_sun_glass_ui_ios_IosWindow__1createChildWindow(JNIEnv *, jobject, > jlong) > > > As a note, I don't have a setup to build this. It is a simple, safe change, > but should be double-checked by someone from Gluon. This PR does what I would expect it to do. I tested it by building an application on all three desktop platforms and it works as usual. - Marked as reviewed by mstrauss (Author). PR: https://git.openjdk.java.net/jfx/pull/615
Re: RFR: 8274022 fixing critical memory leak in the ControlAcceleratorSupport
On Sat, 30 Oct 2021 10:56:40 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.
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
line 31:
> 29: import javafx.beans.property.ReadOnlyObjectProperty;
> 30: import javafx.beans.value.ChangeListener;
> 31: import javafx.beans.value.WeakChangeListener;
Minor: whitespace at the beginning of the line
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.
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
line 89:
> 87: if (sceneChangeListenerMap.containsKey(anchor)) {
> 88: ChangeListener listener =
> sceneChangeListenerMap.get(anchor).get();
> 89: if(listener != null) {
Minor: space before brace
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.
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
line 255:
> 253: if (sceneChangeListenerMap.containsKey(anchor)) {
> 254: ChangeListener listener =
> sceneChangeListenerMap.get(anchor).get();
> 255: if(listener != null) {
Minor: space before brace
-
PR: https://git.openjdk.java.net/jfx/pull/659
