tags:

views:

2398

answers:

4

I'm adding a custom context menu to a TreePanel. This was all working when I had a separate function for the context menu, but I was having problems where the context menu items would end up doubled/tripling up if I clicked on one of the options and then viewed the context menu again.

I had a look around for other contextmenu examples and came up with this one by Aaron Conran I pretty much "stole" it wholesale with a few additions, tacking the function directly into the Ext.ext.treePanel config. This gave me an error about "oe is undefined" which seemed to refer to "contextmenu: this.onContextMenu" in the tree config.

I figured it was probably something to do with the way I was defining all of this, so I decided to look at extending Ext.ext.TreePanel with my function in it as a learning exercise as much as anything.

Unfortunately, having managed to sort out extending TreePanel I'm now back to getting "oe is undefined" when the page tries to build the TreePanel. I've had a look around and I'm not really sure whats causing the problem, so any help or suggestions would be greatly appreciated.

Here is the code that is used to define/build the tree panel. I hope its not too horrible.

siteTree = Ext.extend(Ext.tree.TreePanel,{
  constructor : function(config){
   siteTree.superclass.constructor.call(this, config);
  },
        onContextMenu: function(n,e){
     if (!this.contextMenu){
      console.log('treeContextMenu',n,e);

      if (n.parentNode.id == 'treeroot'){
       var menuitems = [{text:'Add Child',id:'child'}];
      } else {
       var menuitems = 
         [{text:'Add Child',id:'child'},
                {text:'Add Above',id:'above'},
                {text:'Add Below',id:'below'}];
      }

      this.contextMenu = new Ext.menu.Menu({
       id:'treeContextMenu',
       defaults :{
        handler : treeContextClick,
        fqResourceURL : n.id
       },
       items : menuitems
      });
     }
     var xy = e.getXY();
     this.contextMenu.showAt(xy);
    }   
});

var treePanel = new siteTree({
    id: 'tree-panel',
 title : 'Site Tree',
 region : 'center',
 height : 300,
    minSize: 150,
    autoScroll: true,

    // tree-specific configs:
    rootVisible: false,
    lines: false,
    singleExpand: true,
    useArrows: true,

    dataUrl:'admin.page.getSiteTreeChildren?'+queryString,
 root: {
     id: 'treeroot',
        nodeType: 'async',
        text: 'nowt here',
        draggable: false
    },
    listeners:{
        contextmenu: this.onContextMenu
    }
});

As a total aside; Is there a better way to do this in my context menu function?

if (n.parentNode.id == 'treeroot') { Basically, if the clicked node is the top level I only want to give the user an add Child option, not add above/below.

Thanks in advance for your help

+1  A: 

I had this problem yesterday. The issue with the duplicate and triplicate items in the context menu is due to extjs adding multiple elements to the page with the same ID. Each time you call this.contextMenu.showAt(xy) you are adding a div with the ID 'treeContextMenu' to the page. Most browsers, IE especially, deal with this poorly. The solution is to remove the old context menu before adding the new one.

Here is an abridged version of my code:

var old = Ext.get("nodeContextMenu");
if(!Ext.isEmpty(old)) {
    old.remove();
}
var menu = new Ext.menu.Menu({
    id:'nodeContextMenu',
    shadow:'drop',
    items: [ ... ]
});
menu.showAt(e.xy);
alumb
Makes sense. I'd kind of hoped "if (!this.contextMenu){" would have handled that for me though. :/
Stephen Moretti
Yeah, ideally you would create it once and reuse it. That's generally what Ext tries to do internally for things like this.
bmoeskau
yea... bmoeskau is correct. that would be a better solution.
alumb
+1  A: 

I suggest never using hardcoded IDs. @aplumb suggests cleaning the DOM to reuse an existing ID. OK, but I suggest you cleanup the DOM when you no longer need the widgets/elements in the DOM and you should never reuse an ID.

var someId = Ext.id( null, 'myWidgetId' );
var someElement = new SuperWidget({
    id: someId,
    ...
});
Upper Stage
I had no idea about the Ext.id function. I still think the best solution would be to re-use the same object, but I'll defiantly use this function for other things.
alumb
You may also supply no ID and Ext will assign a unique ID for you. A problem with this approach is the ID may not be handy when (and where) you need it.
Upper Stage
Ok - sure. I understand what you're saying about not reusing ids and agree totally.However, wouldn't it be better to reuse the same menu rather than build it everytime? (I realise that there are currently flaws in my context menu function that mean this wouldn't work properly at the minute)
Stephen Moretti
also, wouldn't the approach of using a unique id for every call to make the menu mean that, potentially, my application is going to leech client-side memory for the duration that the application is in context?
Stephen Moretti
To the first comment - yes, of course. To the second comment, no. You are obligated to ensure no memory leaks - reusing one DOM object certainly makes that easier - but there is nothing inherent about unique DOM IDs that will contribute to memory leaks.
Upper Stage
I guess really, as you say, I'm obligated to ensure that no memory leaks and this would involve "destroying" the menu with its unique DOM id once it has been finished with.
Stephen Moretti
+3  A: 

In your instantiation of your siteTree class you have:

listeners: {
    contextmenu: this.onContextMenu
}

However, at the time of the instantiation this.onContextMenu is not pointing to the onContextMenu method you defined in siteTree.

One way of fixing it is to call the method from within a wrapper function:

listeners: {
    contextmenu: function() {
        this.onContextMenu();
    }
}

Assuming you don't override the scope in the listeners config 'this' will be pointing to the siteTree instance at the time the listener is executed.

However, since you are already defining the context menu in the siteTree class, you may as well define the listener there:

constructor: function( config ) {
    siteTree.superclass.constructor.call(this, config);
    this.on('contextmenu', this.onContextMenu);
}

Ensuring the context menu is removed with the tree is also a good idea. This makes your siteTree definition:

var siteTree = Ext.extend(Ext.tree.TreePanel, {
    constructor: function( config ) {
        siteTree.superclass.constructor.call(this, config);
        this.on('contextmenu', this.onContextMenu);
        this.on('beforedestroy', this.onBeforeDestroy);
    },
    onContextMenu: function( node, event ) {
        /* create and show this.contextMenu as needed */
    },
    onBeforeDestroy: function() {
        if ( this.contextMenu ) {
            this.contextMenu.destroy();
            delete this.contextMenu;
        }
    }
});
owlness
I'm probably not going to get a chance today to look at this properly, but didn't want to leave this without some form of comment. This looks really good to me. Thank you. I'll mark it as "the answer" when I've had a play. That said, all the answers have been useful. Thanks to all of you.
Stephen Moretti
A: 

Just to add to owlness's answer

This bit here:

listeners: {
  contextmenu: this.onContextMenu
}

Gets executed when the javascript file is loaded. this at that stage is most likely pointing to the window object.

Igor Zevaka