Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#369 closed defect (fixed)

LayerLegend: add destructor

Reported by: bartvde Owned by:
Priority: major Milestone: 1.1
Component: GeoExt.LayerLegend Version: 1.0
Keywords: Cc:
State: Commit

Description

The LayerLegend class should set layerRecord and layerStore to null in its destructor.

Attachments (1)

geoext-369.patch (649 bytes) - added by bartvde 6 years ago.

Download all attachments as: .zip

Change History (7)

Changed 6 years ago by bartvde

comment:1 Changed 6 years ago by bartvde

  • State changed from None to Review

All legend tests pass in Safari 5 and FF 3.6, please review.

comment:2 Changed 6 years ago by fredj

  • State changed from Review to Commit

Looks good, please commit.

BTW, we're using beforeDestroy and/or onDestroy in our classes, what's the best practice ?

comment:3 Changed 6 years ago by bartvde

I had to use onDestroy (instead of destroy) since the beforeDestroy handler of WMSLegend uses this.layerRecord.

Normally beforeDestroy is used to cancel the destroy by using return false, but in our cases we do not cancel the destroy at all. I think we need to discuss this on the geoext-dev list.

Thanks for the review fredj.

comment:4 Changed 6 years ago by bartvde

  • Resolution set to fixed
  • Status changed from new to closed

(In [2473]) LayerLegend: set layerRecord and layerStore to null, r=fredj (closes #369)

comment:5 Changed 6 years ago by bartvde

Sorry only the beforedestroy event handler can be used to cancel the destroy. Not the beforeDestroy. So my comment was wrong.

in beforeDestroy the DOM elements of the component are still there.

    /**
     * Destroys this component by purging any event listeners, removing the component's element from the DOM,
     * removing the component from its {@link Ext.Container} (if applicable) and unregistering it from
     * {@link Ext.ComponentMgr}.  Destruction is generally handled automatically by the framework and this method
     * should usually not need to be called directly.
     *
     */
    destroy : function(){
        if(!this.isDestroyed){
            if(this.fireEvent('beforedestroy', this) !== false){
                this.destroying = true;
                this.beforeDestroy();
                if(this.ownerCt && this.ownerCt.remove){
                    this.ownerCt.remove(this, false);
                }
                if(this.rendered){
                    this.el.remove();
                    if(this.actionMode == 'container' || this.removeMode == 'container'){
                        this.container.remove();
                    }
                }
                // Stop any buffered tasks
                if(this.focusTask && this.focusTask.cancel){
                    this.focusTask.cancel();
                }
                this.onDestroy();
                Ext.ComponentMgr.unregister(this);
                this.fireEvent('destroy', this);
                this.purgeListeners();
                this.destroying = false;
                this.isDestroyed = true;
            }
        }
    },

comment:6 Changed 6 years ago by bartvde

Both of them (beforeDestroy and onDestroy) are private btw, so instead I think we should be relying on the events (destroy and beforedestroy) in GeoExt instead.

Note: See TracTickets for help on using tickets.