public inbox for [email protected]  
help / color / mirror / Atom feed
From: Ashesh Vashi <[email protected]>
To: Dave Page <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: Dialogues not closing
Date: Fri, 15 Apr 2016 13:36:13 +0530
Message-ID: <CAG7mmow_OwW2YeBT_Br6Tjm_bQMthAxzecvetRq5h8C725wRbA@mail.gmail.com> (raw)
In-Reply-To: <CA+OCxoyJ5BQQYXJ_r3UNGkCOXHeCe1NMfyKwoVUTEK44b+tUeg@mail.gmail.com>
References: <CA+OCxozjZ9kLDNJnKwvPnSPju=jAhn=0ejfiQ+rpX7GSvqGjMQ@mail.gmail.com>
	<CAG7mmowx=R3_-qSbuWU4=wm2M-n+qP2udC+pbxU+1Xe8dxv=Gg@mail.gmail.com>
	<CA+OCxoyJ5BQQYXJ_r3UNGkCOXHeCe1NMfyKwoVUTEK44b+tUeg@mail.gmail.com>
List-Unsubscribe:  <mailto:[email protected]?body=unsub%20pgadmin-hackers>

Hi Dave,


On Fri, Apr 15, 2016 at 1:20 PM, Dave Page <[email protected]> wrote:

>
>
> On Fri, Apr 15, 2016 at 6:32 AM, Ashesh Vashi <
> [email protected]> wrote:
>
>> On Fri, Apr 15, 2016 at 2:23 AM, Dave Page <[email protected]> wrote:
>>
>>> Hey Ashesh,
>>>
>>> We have an issue with dialogues not closing on Save button presses.
>>> One of the team told me yesterday or the day before that it's a
>>> general issue, not one specific to the patch I was reviewing at the
>>> time, which I've been finding seems to be the case.
>>>
>>> Any idea what this issue is?
>>>
>> Sure - I will look into it.
>>
>
> Thanks - please make it a priority; I'm at PG Conf NYC next week and
> expect to be doing lots of demos.
>
Please find the patch for the same.

This also includes some of the cleanup required to solve some of the memory
leak issues.

I was not able to reproduce the issue at my end, but - I've idea of when it
could fail to close the panel, which I have resolved in this patch.
Can you please test it?

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company
<http://www.enterprisedb.com/;


*http://www.linkedin.com/in/asheshvashi*
<http://www.linkedin.com/in/asheshvashi;


>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


-- 
Sent via pgadmin-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers


Attachments:

  [application/octet-stream] dialog_close_n_memory_leak.patch (12.1K, 3-dialog_close_n_memory_leak.patch)
  download | inline diff:
diff --git a/web/pgadmin/browser/static/js/datamodel.js b/web/pgadmin/browser/static/js/datamodel.js
index b28e041..6eb3ae5 100644
--- a/web/pgadmin/browser/static/js/datamodel.js
+++ b/web/pgadmin/browser/static/js/datamodel.js
@@ -199,6 +199,36 @@ function(_, pgAdmin, $, Backbone) {
 
         return self;
       },
+      // Create a reset function, which allow us to remove the nested object.
+      reset: function() {
+        var obj;
+        for(id in this.objects) {
+          obj = this.get(id);
+
+          if (obj) {
+            if (obj instanceof pgBrowser.DataModel) {
+              obj.reset();
+              delete obj;
+            } else if (obj instanceof Backbone.Model) {
+              obj.clear({silent: true});
+              delete obj;
+            } else if (obj instanceof pgBrowser.DataCollection) {
+              obj.reset({silent: true});
+              delete obj;
+            } else if (obj instanceof Backbone.Collection) {
+              obj.each(function(m) {
+                if (m instanceof Bakbone.DataModel) {
+                  obj.reset();
+                  obj.clear({silent: true});
+                }
+              });
+              Backbone.Collection.prototype.reset.apply(obj, {silent: true});
+              delete obj;
+            }
+          }
+        }
+        this.clear({silent: true});
+      },
       sessChanged: function() {
         var self = this;
 
@@ -755,6 +785,20 @@ function(_, pgAdmin, $, Backbone) {
           return (_.size(res) == 0 ? null : res);
         }
       },
+      // Override the reset function, so that - we can reset the model
+      // properly.
+      reset: function(opts) {
+        this.each(function(m) {
+          if (!m)
+            return;
+          if (m instanceof pgBrowser.DataModel) {
+            m.reset();
+          } else {
+            m.clear({silent: true});
+          }
+        });
+        Backbone.Collection.prototype.reset.apply(this, arguments);
+      },
       objFindInSession: function(m, type) {
         var hasPrimaryKey = m.primary_key &&
               typeof(m.primary_key) == 'function',
diff --git a/web/pgadmin/browser/templates/browser/js/collection.js b/web/pgadmin/browser/templates/browser/js/collection.js
index fb1dd42..cb9b650 100644
--- a/web/pgadmin/browser/templates/browser/js/collection.js
+++ b/web/pgadmin/browser/templates/browser/js/collection.js
@@ -67,16 +67,20 @@ function($, _, S, pgAdmin, Backbone, Alertify, Backform) {
         gridView = {
           'remove': function() {
             if (this.grid) {
+              if (this.grid.collection) {
+                this.grid.collection.reset(null, {silent: true});
+                delete (this.grid.collection);
+              }
               delete (this.grid);
               this.grid = null;
             }
-          }
+          },
+          grid: grid
         };
-        gridView.grid = grid;
 
       if (view) {
         // Release the view
-        view.remove();
+        view.remove({data: true});
         // Deallocate the view
         delete view;
         view = null;
diff --git a/web/pgadmin/browser/templates/browser/js/node.js b/web/pgadmin/browser/templates/browser/js/node.js
index 54ddfb8..9b43e4f 100644
--- a/web/pgadmin/browser/templates/browser/js/node.js
+++ b/web/pgadmin/browser/templates/browser/js/node.js
@@ -652,7 +652,7 @@ function($, _, S, pgAdmin, Menu, Backbone, Alertify, pgBrowser, Backform) {
           // creating new view.
           if (view) {
             // Release the view
-            view.remove();
+            view.remove({data: true});
             // Deallocate the view
             delete view;
             view = null;
@@ -773,7 +773,7 @@ function($, _, S, pgAdmin, Menu, Backbone, Alertify, pgBrowser, Backform) {
           // creating the new view.
           if (view) {
             // Release the view
-            view.remove();
+            view.remove({data: true});
             // Deallocate the view
             delete view;
             view = null;
@@ -907,8 +907,8 @@ function($, _, S, pgAdmin, Menu, Backbone, Alertify, pgBrowser, Backform) {
         }.bind(panel),
         closePanel = function() {
           // Closing this panel
-          this.close()
-        },
+          this.close();
+        }.bind(panel),
         updateTreeItem = function() {
           var panel = this;
 
@@ -940,9 +940,7 @@ function($, _, S, pgAdmin, Menu, Backbone, Alertify, pgBrowser, Backform) {
           if (view.model.tnode && '_id' in view.model.tnode) {
             var d = _.extend({}, view.model.tnode),
               func = function(i) {
-                setTimeout(function() {
-                  closePanel();
-                }, 0);
+                setTimeout(function() {closePanel();}, 0);
                 if (i) {
                   tree.select(i, {focus: true});
                 }
@@ -962,6 +960,11 @@ function($, _, S, pgAdmin, Menu, Backbone, Alertify, pgBrowser, Backform) {
                     itemData: d,
                     success: function(i, o) {
                       func(o.items.eq(0));
+                    },
+                    fail: function() {
+                      // We still want to close the panel
+                      console.log(arguments);
+                      func(null);
                     }
                   });
                   return;
@@ -1017,7 +1020,9 @@ function($, _, S, pgAdmin, Menu, Backbone, Alertify, pgBrowser, Backform) {
                                 func(null);
                               },
                               fail: function() {
+                                // We would still like to close it.
                                 console.log(arguments);
+                                func(null);
                               }
                             });
                           }
@@ -1052,13 +1057,17 @@ function($, _, S, pgAdmin, Menu, Backbone, Alertify, pgBrowser, Backform) {
                         func(i);
                       },
                       fail: function() {
+                        // We would still like to close it.
                         console.log(arguments);
+                        func(null);
                       }
                     });
 
                   },
                   fail: function() {
+                    // We would still like to close it.
                     console.log(arguments);
+                    func(null);
                   }
                 });
               }
@@ -1067,53 +1076,60 @@ function($, _, S, pgAdmin, Menu, Backbone, Alertify, pgBrowser, Backform) {
                 itemData: d,
                 success: function(i, o) {
                   func(i);
+                },
+                fail: function() {
+                  // We would still like to close it.
+                  console.log(arguments);
+                  func(null);
                 }
               });
             }
           } else {
-          /*
-           * Sometime we don't get node in response even though it's saved
-           * on server. In such case just reload the collection to get newly
-           * created nodes.
-           */
-
-           var children = tree.children(item, false, false),
-            openNode = function(item, animation){
-              tree.open(item, {
-                success: function (item, options){
-                  setTimeout(function() {
-                    closePanel();
-                  }, 0);
-                },
-                fail: function (item, options){
-                },
-                unanimated: animation
-              });
-            };
+            /*
+             * Sometime we don't get node in response even though it's saved
+             * on server. In such case just reload the collection to get newly
+             * created nodes.
+             */
+
+            var children = tree.children(item, false, false),
+                openNode = function(item, animation){
+                  tree.open(item, {
+                    success: function (item, options){
+                      setTimeout(function() {closePanel();}, 0);
+                    },
+                    fail: function (item, options){
+                      setTimeout(function() {closePanel();}, 0);
+                    },
+                    unanimated: animation
+                  });
+                };
 
             if (children) {
               _.each(children, function(child) {
-                var $child = $(child);
-                var data = tree.itemData($child)
-                  if (data._type == that.collection_type){
-                    // We found collection which need to reload.
-                    if (tree.wasLoad($child)) {
-                      tree.unload($child, {
-                        success: function (item, options){
-                          openNode(item, true);
-                        },
-                        fail: function (item, options){
-                        },
-                        unanimated: true
-                      });
-                    } else {
-                      openNode($child, false);
-                    }
+                var $child = $(child),
+                    data = tree.itemData($child);
+                if (data._type == that.collection_type){
+                  // We found collection which need to reload.
+                  if (tree.wasLoad($child)) {
+                    tree.unload($child, {
+                      success: function (item, options){
+                        openNode(item, true);
+                      },
+                      fail: function (item, options){
+                        setTimeout(function() {closePanel();}, 0);
+                      },
+                      unanimated: true
+                    });
+                  } else {
+                    openNode($child, false);
                   }
+                }
               });
+              return;
             }
+            setTimeout(function() {closePanel();}, 0);
           }
-        },
+        }.bind(panel),
         editInNewPanel = function() {
           // Open edit in separate panel
           setTimeout(function() {
@@ -1123,14 +1139,14 @@ function($, _, S, pgAdmin, Menu, Backbone, Alertify, pgBrowser, Backform) {
             }]);
           }, 0);
         },
-        onCancelFunc = closePanel.bind(panel),
+        onCancelFunc = closePanel,
         onSaveFunc = updateTreeItem.bind(panel),
         onEdit = editFunc.bind(panel);
 
       if (action) {
         if (action == 'create'){
-          onCancelFunc = closePanel.bind(panel);
-          onSaveFunc = saveNewNode.bind(panel);
+          onCancelFunc = closePanel;
+          onSaveFunc = saveNewNode;
         }
         if (action != 'properties') {
           // We need to keep track edit/create mode for this panel.
@@ -1143,6 +1159,17 @@ function($, _, S, pgAdmin, Menu, Backbone, Alertify, pgBrowser, Backform) {
         properties();
         onEdit = editInNewPanel.bind(panel);
       }
+      if (panel.closeable()) {
+        var onCloseFunc = function() {
+          var j = this.$container.find('.obj_properties').first(),
+              view = j && j.data('obj-view');
+
+          if (view) {
+            view.remove({data: true});
+          }
+        }.bind(panel);
+        panel.on(wcDocker.EVENT.CLOSED, onCloseFunc);
+      }
     },
     /**********************************************************************
      * Generate the URL for different operations
diff --git a/web/pgadmin/static/js/backform.pgadmin.js b/web/pgadmin/static/js/backform.pgadmin.js
index 2f5ac15..580573c 100644
--- a/web/pgadmin/static/js/backform.pgadmin.js
+++ b/web/pgadmin/static/js/backform.pgadmin.js
@@ -594,6 +594,23 @@
       }
 
       return this;
+    },
+    remove: function(opts) {
+      if (opts && opts.data) {
+        if (this.model) {
+          if (this.model.reset) {
+            this.model.reset();
+          }
+          this.model.clear({silent: true});
+          delete (this.model);
+        }
+        if (this.errorModel) {
+          this.errorModel.clear({silent: true});
+          delete (this.errorModel);
+        }
+      }
+      this.cleanup();
+      Backform.Form.prototype.remove.apply(this, arguments);
     }
   });
 


view thread (7+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: Dialogues not closing
  In-Reply-To: <CAG7mmow_OwW2YeBT_Br6Tjm_bQMthAxzecvetRq5h8C725wRbA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox