Hi,
I would like to propose a new review as a solution which I think should solve
the problem:
Bug: https://bugs.openjdk.java.net/browse/JDK-8175968
Webrev can be found at: http://cr.openjdk.java.net/~serb/8175968/webrev.00
----- ldubox-coding...@yahoo.co.uk wrote:
> 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
> >
> >
> >
