Thank you for reporting this bug. We will make it our priority to review this report.
  1. #1
    Ext User
    Join Date
    Apr 2008
    Posts
    57
    Vote Rating
    0
    azbok is on a distinguished road

      0  

    Default [TENT][3.0.0] Bug with RowEditor and destroying DatePickers with fix

    [TENT][3.0.0] Bug with RowEditor and destroying DatePickers with fix


    Using the code from the row-editor.js example in Ext 3.0.0, make these adjustments

    Code:
        var layout = new Ext.Panel({
            title: 'Employee Salary by Month',
            layout: 'border',
            layoutConfig: {
                columns: 1
            },
            width:600,
            height: 600,
            items: [chart, grid]
        });
    
        var win = new Ext.Window({
            width:630,
            id:'winid',
            height:630,
            layout:'fit',
            border:false,
            closable:true,
            title:'Example',
            items:[layout]
        });
    
        win.show();
    
        // layout.render(Ext.getBody());
    Here's the list of steps
    1) Click to get the row editor active
    2) Click the date picker to make it display the menu
    3) Close the menu
    4) Close the window

    Then upon closing the window, you should get "this.keyNav" is null

    What happens is that the row editor is holding onto the date menu. I found this out by putting a breakpoint on line 17698 and seeing the parent of all the items.

    Code:
    17696 beforeDestroy : function(){
    17697          if(this.items){
    17698                Ext.destroy.apply(Ext, this.items.items);
    17699          }
    The fix in RowEditor.js. I think no matter which way you do it, the items should be removed from the panel, otherwise they get destroyed again!

    Code:
    
            var scope = this;
    
            grid.on({
                scope: this,
                keydown: this.onGridKey,
                columnresize: this.verifyLayout,
                columnmove: this.refreshFields,
                reconfigure: this.refreshFields,
                destroy : function () { scope.removeAll(false); scope.destroy(); },
                bodyscroll: {
                    buffer: 250,
                    fn: this.positionButtons
                }
            });

  2. #2
    Ext User
    Join Date
    Jul 2007
    Location
    Florida
    Posts
    9,996
    Vote Rating
    6
    mjlecomte will become famous soon enough mjlecomte will become famous soon enough

      0  

    Default


    I had a similar issue to contend with using the GridFilters extension actually.

    The datepicker assumes the keyNav exists an attempts to disable it again. The fix I came up with avoided the attempt to destroy the picker twice, but I'm wondering if that's the best way if other classes have similar problems.

    If the keyNav.disable() had been wrapped with a check to see if keyNav existed I would never have found that I wasn't destroying it properly the first time though. So in that sense it was good not to just bandaid it by wrapping inside an if (keyNav) check.

  3. #3
    Ext User
    Join Date
    Jul 2007
    Location
    Florida
    Posts
    9,996
    Vote Rating
    6
    mjlecomte will become famous soon enough mjlecomte will become famous soon enough

      0  

    Default


    Side note, discussion. Seems that wrapping things in a window is useful for inspecting cleanup. I've exposed problems of items not being cleaned up properly or at all wrapping something in a window and closing it.

  4. #4
    Ext User
    Join Date
    Apr 2008
    Posts
    57
    Vote Rating
    0
    azbok is on a distinguished road

      0  

    Default


    The talk about keyNav.disable() being wrapped with a check is the "hurtful" defensive programming you can get yourself in trouble with! I've found that certain types of defensive programming is extremely bad because they hide errors like that. It's good that the code was just blindly trying to access null elements with no checks (in that case anyway) because it shouldn't be called twice anyway.

    The whole wrapping thing in a window is a good thought. I just happened upon it but didn't realize it could be a good strategy as well.

    What do you think the proper fix is for the row editor and the date picker?

  5. #5
    Ext User
    Join Date
    Jul 2007
    Location
    Florida
    Posts
    9,996
    Vote Rating
    6
    mjlecomte will become famous soon enough mjlecomte will become famous soon enough

      0  

    Default


    I defer answering what the proper fix is at the moment.

    I will add another comment though, bringing up an old request again, that it would be good if there were some template methods in place to destroy plugins as well. I mean there is a check to initPlugins if plugins exists, so I don't see why we wouldn't have some kind of template method to call upon destruction (beforeDestroy, destroy, onDestroy).

  6. #6
    Sencha - Ext JS Dev Team evant's Avatar
    Join Date
    Apr 2007
    Location
    Sydney, Australia
    Posts
    17,004
    Vote Rating
    650
    evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute evant has a reputation beyond repute

      0  

    Default


    Fixed in SVN.
    Evan Trimboli
    Sencha Developer
    Twitter - @evantrimboli
    Don't be afraid of the source code!

  7. #7
    Ext JS Premium Member prophet's Avatar
    Join Date
    Mar 2007
    Location
    Greenwich, CT
    Posts
    187
    Vote Rating
    0
    prophet is on a distinguished road

      0  

    Default


    Just had this problem in Ext 3.1 - Resolved with the above fix. Thanks guys!

    PS: Also worth noting that I was not using a DatePicker component, just a NumberField, TextField, and a few booleancolumn / checkboxes.
    Last edited by prophet; 10 Aug 2010 at 7:22 AM. Reason: some more detail
    Brad Baumann