Hi all,
> but imho it still beats adding an extra method to the API
> and shifting the burden to the API user.
Just a question, would you be an "API user" of this method
directly? It seems to me that dispose() could be called somewhere
knowing more about the lifetime of this object (possibly from
BasicFileChooserUI) so that it remains an implementation
detail to most users of JFileChooser.
The WeakReference trick would then just be a (poor) fallback
mechanism.
So this trick (introduced somewhere between Java 6 and 8)
never worked. As mentioned, the key is to ensure the listeneris
a static inner class. For example, this works:
private PropertyChangeListener pcl;
public FileSystemView() {
pcl = createPropertyChangeListener();
}
private static createPropertyChangeListener() {
// current content of constructor goes here
}
The 'static' is all-important, so that the anonymous listener class
becomes static also, and doesn't have a synthetic reference to
the outer class.
> Thanks to the weak listener, the FileSystemView will get GC-ed as
> the listener no longer keeps a hard reference to it.
I don't think a Weak Listener helps - the code you referenced earlier
suffers the same limitation as the current code: The reference
isn't freedunless a new property change event comes along. Property
changeevents are very infrequent from UIManager - typically never.
I've also been struggling to see a good alternative to dispose().
Finalizers came to mind also but ... (UPDATE: deprecated now huh?
OK forget it then.)
An alternative could be a timer, but that's a lot of extra baggage.
IF THIS WERE APPLICATION CODE a trick like this might work,
removing the need for both the listener and dispose():
private Boolean useSystemExtensionHiding = null;
private boolean getUseSystemExtensionHiding() {
assert SwingUtilities.isEventDispatchThread();
if (useSystemExtensionHiding == null) {
useSystemExtensionHiding =
UIManager.getDefaults().getBoolean("FileChooser.useSystemExtensionHiding");
SwingUtilities.invokeLater(() -> useSystemExtensionHiding
= null);
}
return useSystemExtensionHiding;
}
After all, the caching is just an optimisation to avoid repeated
map-lookups during painting (right?) The above reduces it to
once per full component paint, which is a small cost.
But being library code I'm not sure if it's safe to make such
assumptions (that the EDT event loop is running, for example).
(Expert opinion?)
ACTUALLY if avoiding dispose() is preferred - then I wonder if the
optimisation is "past its expiry date"? The "free lunch may be
over" now, but it was still being delivered since Java 1.1 :-)
What is the cost of one map lookup (per file) compared to the cost
of painting an icon for each file, for example? I'd guess they're
about on-par.
Would it be so bad to just drop the caching of this boolean
altogether?
Kind regards,
Luke
On 07/07/2017 09:21, Robin Stevens wrote:
I still do not think you need a public dispose method.
If you use the weak listener (the correct implementation), the
FileSystemView instance can be GC-ed, even when the
PropertyChangeListener is still attached to the UIManager.
What you can do is:
- Store a reference to the PropertyChangeListener in the
FileSystemView class as a field.
- Override the finalize method of the FileSystemView class to
de-register the PropertyChangeListener from the UIManager. Thanks to
the weak listener, the FileSystemView will get GC-ed as the listener
no longer keeps a hard reference to it.
Not the nicest way to do such cleanup (which should probably happen
on
the EDT) in a finalizer, but imho it still beats adding an extra
method to the API and shifting the burden to the API user.
Robin
On Thu, Jul 6, 2017 at 3:15 PM, Prasanta Sadhukhan
<prasanta.sadhuk...@oracle.com
<mailto:prasanta.sadhuk...@oracle.com>>wrote:
Hi Sergey,
I tried with the proposed WeakPropertyChangeListener but as far
I
understand & tested, it seems also to rely on propertyChange()
API
to be called, so the scenario mentioned in the bug will still
fail.
I cannot find any other way rather than calling dispose() for
the
scenario mentioned there.
Regards
Prasanta
On 7/5/2017 4:35 AM, Sergey Bylokhov wrote:
Hi, Prasanta.
Probably it is possible to implement it without users
interaction and new api?
----- prasanta.sadhuk...@oracle.com
<mailto:prasanta.sadhuk...@oracle.com>wrote:
Hi All,
Please review a fix for a memory leak issue where
PropertyChangeListener
object added by FileSystemView constructor is never
removed.
Proposed fix is to add dispose() method to be called by
app when they
want to remove this resource.
Bug: https://bugs.openjdk.java.net/browse/JDK-8175968
<https://bugs.openjdk.java.net/browse/JDK-8175968>
webrev:
http://cr.openjdk.java.net/~psadhukhan/8175968/webrev.00/
<http://cr.openjdk.java.net/%7Epsadhukhan/8175968/webrev.00/>
Regards
Prasanta
