Making sure that Display, *Opts and overlay instances are GC'd

appropriately. Some things are broken due to weakref-related changes in
the props package.

fsl/data/featimage.py

@@ -58,6 +58,10 @@ class FEATImage(fslimage.Image):
             self.name = '{}: {}'.format(self.__analysisName, self.name)
 
 
+    def getFEATDir(self):
+        return self.__featDir
+
+
     def getAnalysisName(self):
         return self.__analysisName
         

fsl/fslview/controls/clusterpanel.py

@@ -65,7 +65,7 @@ class ClusterPanel(fslpanel.FSLViewPanel):
 
         overlayList.addListener('overlays',
                                 self._name,
-                                self.__selectedOverlayChanged)
+                                self.__overlayListChanged)
         displayCtx .addListener('selectedOverlay',
                                 self._name,
                                 self.__selectedOverlayChanged)
@@ -137,26 +137,60 @@ class ClusterPanel(fslpanel.FSLViewPanel):
         self._displayCtx.getDisplay(mask).overlayType = 'label'
 
 
-    def __selectedOverlayChanged(self, *a):
+    def __overlayListChanged(self, *a):
+        self.__selectedOverlayChanged()
+        self.__enableOverlayButtons()
 
-        self.__statSelect .Clear()
-        self.__clusterList.ClearGrid()
 
-        self.__selectedOverlay = None
+    def __enableOverlayButtons(self):
+        
+        if self.__selectedOverlay is None:
+            return
+
+        overlay  = self.__selectedOverlay
+        contrast = self.__statSelect.GetSelection()
 
+        zstat     = overlay.getZStats(     contrast)
+        clustMask = overlay.getClusterMask(contrast)
+
+        dss = [ovl.dataSource for ovl in self._overlayList]
+
+        self.__addZStats     .Enable(zstat    .dataSource not in dss)
+        self.__addClusterMask.Enable(clustMask.dataSource not in dss)
+        
+    
+    def __selectedOverlayChanged(self, *a):
+
+        prevOverlay            = self.__selectedOverlay
+        self.__selectedOverlay = None
+        
         # No overlays are loaded
         if len(self._overlayList) == 0:
             self.__disable(strings.messages[self, 'noOverlays'])
             return
 
         overlay = self._displayCtx.getSelectedOverlay()
-
+        
         # Not a FEAT image, can't 
         # do anything with that
         if not isinstance(overlay, featimage.FEATImage):
             self.__disable(strings.messages[self, 'notFEAT'])
             return
 
+        # Selected overlay is either the
+        # same one (maybe the overlay list,
+        # rather than the selected overlay,
+        # changed) or the newly selected
+        # overlay is from the same FEAT
+        # analysis. No need to do anything.
+        if prevOverlay is not None and (prevOverlay is overlay or 
+           prevOverlay.getFEATDir() == overlay.getFEATDir()):
+            self.__selectedOverlay = overlay
+            return
+            
+        self.__statSelect .Clear()
+        self.__clusterList.ClearGrid()
+
         self.__selectedOverlay = overlay
 
         self.__sizer.Show(self.__disabledText, False)
@@ -190,6 +224,7 @@ class ClusterPanel(fslpanel.FSLViewPanel):
             self.__statSelect.Append(name, clusterList)
             
         self.__overlayName.SetLabel(overlay.getAnalysisName())
+
         self.__statSelect.SetSelection(0)
         self.__displayClusterData(clusts[0][1])
 
@@ -201,6 +236,7 @@ class ClusterPanel(fslpanel.FSLViewPanel):
         idx  = self.__statSelect.GetSelection()
         data = self.__statSelect.GetClientData(idx)
         self.__displayClusterData(data)
+        self.__enableOverlayButtons()
 
         
     def __displayClusterData(self, clusters):

fsl/fslview/displaycontext/display.py

@@ -49,7 +49,14 @@ class DisplayOpts(props.SyncableHasProperties):
 
         
     def destroy(self):
-        pass
+        """If overridden, this method should be called by the subclass
+        implementation.
+        """
+
+        self.overlay     = None
+        self.display     = None
+        self.overlayList = None
+        self.displayCtx  = None
 
 
     def getReferenceImage(self):
@@ -193,6 +200,23 @@ class Display(props.SyncableHasProperties):
         self.__displayOpts = None
         self.__overlayTypeChanged()
 
+
+
+    def destroy(self):
+        """This method should be called when this ``Display`` instance
+        is no longer needed.
+        """
+        
+        if self.__displayOpts is not None:
+            self.__displayOpts.destroy()
+
+        self.removeListener('overlayType', 'Display_{}'.format(id(self)))
+
+        self.detachFromParent()
+        
+        self.__displayOpts = None
+        self.__overlay     = None
+        
         
     def getDisplayOpts(self):
         """

fsl/fslview/displaycontext/displaycontext.py

@@ -208,8 +208,25 @@ class DisplayContext(props.SyncableHasProperties):
         constraints on the :attr:`selectedOverlay` property.
         """
 
+        # Discard all Display instances
+        # which refer to overlays that
+        # are no longer in the list
+        for overlay in list(self.__displays.keys()):
+            if overlay not in self.__overlayList:
+                
+                display = self.__displays.pop(overlay)
+                opts    = display.getDisplayOpts()
+
+                display.removeListener('overlayType', self.__name)
+                opts.removeGlobalListener(self.__name)
+                
+                # The display instance will destroy the
+                # opts instance, so we don't do it here
+                display.destroy()
+ 
         # Ensure that a Display object
-        # exists for every overlay
+        # exists for every overlay in
+        # the list
         for overlay in self.__overlayList:
 
             # The getDisplay method

fsl/fslview/displaycontext/volumeopts.py

@@ -88,7 +88,7 @@ class ImageOpts(fsldisplay.DisplayOpts):
 
 
     def destroy(self):
-        pass
+        fsldisplay.DisplayOpts.destroy(self)
 
 
     def getDisplayBounds(self):
@@ -327,11 +327,10 @@ class VolumeOpts(ImageOpts):
                            display.getSyncPropertyName('brightness'))
             self.bindProps(self   .getSyncPropertyName('displayRange'), 
                            display,
-                           display.getSyncPropertyName('contrast')) 
+                           display.getSyncPropertyName('contrast'))
 
-    def destroy(self):
 
-        ImageOpts.destroy(self)
+    def destroy(self):
 
         if self.getParent() is not None:
             display = self.display
@@ -343,7 +342,9 @@ class VolumeOpts(ImageOpts):
                              display.getSyncPropertyName('brightness'))
             self.unbindProps(self   .getSyncPropertyName('displayRange'), 
                              display,
-                             display.getSyncPropertyName('contrast')) 
+                             display.getSyncPropertyName('contrast'))
+
+        ImageOpts.destroy(self)
 
 
     def __toggleListeners(self, enable=True):

fsl/fslview/gl/globject.py

@@ -226,6 +226,16 @@ class GLImageObject(GLObject):
         self.displayOpts = display.getDisplayOpts()
 
 
+    def destroy(self):
+        """If this method is overridden, it should be called by the subclass
+        implementation. It clears references to the :class:`.Image`,
+        :class:`.Display`, and :class:`.DisplayOpts` instances.
+        """
+        self.image       = None
+        self.display     = None
+        self.displayOpts = None
+
+
     def getDisplayBounds(self):
         return self.displayOpts.getDisplayBounds()
 

fsl/fslview/gl/glvolume.py

@@ -111,6 +111,8 @@ class GLVolume(globject.GLImageObject):
         
         self.removeDisplayListeners()
         fslgl.glvolume_funcs.destroy(self)
+        
+        globject.GLImageObject.destroy(self)
 
         
     def testUnsynced(self):
@@ -121,6 +123,8 @@ class GLVolume(globject.GLImageObject):
         :class:`GLVolume` instance needs to create its own image texture;
         returns ``False`` otherwise.
         """
+        if self.displayOpts.getParent() is None:
+            return True
         return (not self.displayOpts.isSyncedToParent('volume')     or
                 not self.displayOpts.isSyncedToParent('resolution') or
                 not self.displayOpts.isSyncedToParent('interpolation'))
@@ -225,9 +229,11 @@ class GLVolume(globject.GLImageObject):
         opts   .addListener(          'volume',         lName, imageUpdate)
         opts   .addListener(          'resolution',     lName, imageUpdate)
         opts   .addListener(          'interpolation',  lName, imageUpdate)
-        opts   .addSyncChangeListener('volume',         lName, imageRefresh)
-        opts   .addSyncChangeListener('resolution',     lName, imageRefresh)
-        opts   .addSyncChangeListener('interpolation',  lName, imageRefresh)
+
+        if opts.getParent() is not None:
+            opts   .addSyncChangeListener('volume',        lName, imageRefresh)
+            opts   .addSyncChangeListener('resolution',    lName, imageRefresh)
+            opts   .addSyncChangeListener('interpolation', lName, imageRefresh)
 
 
     def removeDisplayListeners(self):
@@ -250,9 +256,10 @@ class GLVolume(globject.GLImageObject):
         opts   .removeListener(          'volume',         lName)
         opts   .removeListener(          'resolution',     lName)
         opts   .removeListener(          'interpolation',  lName)
-        opts   .removeSyncChangeListener('volume',         lName)
-        opts   .removeSyncChangeListener('resolution',     lName)
-        opts   .removeSyncChangeListener('interpolation',  lName)
+        if opts.getParent() is not None:
+            opts   .removeSyncChangeListener('volume',        lName)
+            opts   .removeSyncChangeListener('resolution',    lName)
+            opts   .removeSyncChangeListener('interpolation', lName)
 
         
     def preDraw(self):

fsl/fslview/gl/slicecanvas.py

@@ -505,6 +505,60 @@ class SliceCanvas(props.HasProperties):
         # display axes. Easiest way to do this
         # is to destroy and re-create them
         self._renderModeChange()
+
+
+    def __overlayTypeChanged(self, value, valid, display, name):
+        """Called when the :attr:`.Display.overlayType` setting for any
+        overlay changes. Makes sure that an appropriate :class:`.GLObject`
+        has been created for the overlay (see the :meth:`__genGLObject`
+        method).
+        """
+
+        log.debug('GLObject representation for {} '
+                  'changed to {}'.format(display.name,
+                                         display.overlayType))
+
+        self.__genGLObject(display.getOverlay(), display)
+        self._refresh()
+
+
+    def __genGLObject(self, overlay, display, updateRenderTextures=True):
+        """Creates a :class:`.GLObject` instance for the given ``overlay``,
+        destroying any existing instance.
+
+        If ``updateRenderTextures`` is ``True`` (the default), and the
+        :attr:`.renderMode` is ``offscreen`` or ``prerender``, any
+        render texture associated with the overlay is destroyed.
+        """
+        
+        if not self._setGLContext():
+            return
+
+        # Tell the previous GLObject (if
+        # any) to clean up after itself
+        globj = self._glObjects.get(overlay, None)
+        if globj is not None:
+            globj.destroy()
+
+            if updateRenderTextures:
+                if self.renderMode == 'offscreen':
+                    tex = self._offscreenTextures.pop(overlay, None)
+                    if tex is not None:
+                        tex.destroy()
+
+                elif self.renderMode == 'prerender':
+                    tex, name = self._prerenderTextures.pop(
+                        overlay, (None, None))
+                    if tex is not None:
+                        glresources.delete(name)
+
+        globj = globject.createGLObject(overlay, display)
+
+        if globj is not None:
+            globj.setAxes(self.xax, self.yax)
+            globj.addUpdateListener(self.name, self._refresh)
+
+        self._glObjects[overlay] = globj
  
             
     def _overlayListChanged(self, *a):
@@ -537,64 +591,15 @@ class SliceCanvas(props.HasProperties):
 
             display = self.displayCtx.getDisplay(overlay)
 
-            # Called when the GL object representation
-            # of the overlay needs to be re-created
-            # 
-            # TODO make this an instance method, 
-            #      instead of an inner method
-            def genGLObject(value=None,
-                            valid=None,
-                            ctx=None,
-                            name=None,
-                            disp=display,
-                            ovl=overlay,
-                            updateRenderTextures=True):
-
-                log.debug('GLObject representation for {} '
-                          'changed to {}'.format(disp.name,
-                                                 disp.overlayType))
-
-                if not self._setGLContext():
-                    return
-
-                # Tell the previous GLObject (if
-                # any) to clean up after itself
-                globj = self._glObjects.get(ovl, None)
-                if globj is not None:
-                    globj.destroy()
-                    
-                    if updateRenderTextures:
-                        if self.renderMode == 'offscreen':
-                            tex = self._offscreenTextures.pop(ovl, None)
-                            if tex is not None:
-                                tex.destroy()
-                                
-                        elif self.renderMode == 'prerender':
-                            tex, name = self._prerenderTextures.pop(
-                                ovl, (None, None))
-                            if tex is not None:
-                                glresources.delete(name)
-
-                globj = globject.createGLObject(ovl, disp)
-
-                if globj is not None:
-                    globj.setAxes(self.xax, self.yax)
-
-                self._glObjects[ovl] = globj
-
-                if updateRenderTextures:
-                    self._updateRenderTextures()
-
-                globj.addUpdateListener(self.name, self._refresh)
-                
-                self._refresh()
-                
-            genGLObject(updateRenderTextures=False)
+            self.__genGLObject(overlay, display, updateRenderTextures=False)
 
             # Bind Display.softwareMode to SliceCanvas.softwareMode
             display.bindProps('softwareMode', self)
 
-            display.addListener('overlayType',   self.name, genGLObject)
+            display.addListener('overlayType',
+                                self.name,
+                                self.__overlayTypeChanged)
+            
             display.addListener('enabled',       self.name, self._refresh)
             display.addListener('softwareMode',  self.name, self._refresh)