Merge lp:~bcsaller/juju-gui/update-reductions into lp:juju-gui/experimental

[Benjamin Saller]

Remove some db.update calls

Given the current model we can't remove all calls
to db.fire('update') but we can remove them from
anything using the topology. Change 'update' events
(which redispatch) so simply call topo.update().

service and charm views still redispatch.

https://codereview.appspot.com/8083043/

[Benjamin Saller (bcsaller)]

Reviewers: mp+155972_code.launchpad.net,

Message:
Please take a look.

Description:
Remove some db.update calls

Given the current model we can't remove all calls
to db.fire('update') but we can remove them from
anything using the topology. Change 'update' events
(which redispatch) so simply call topo.update().

service and charm views still redispatch.

https://code.launchpad.net/~bcsaller/juju-gui/update-reductions/+merge/155972

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8083043/

Affected files:
   A [revision details]
   M app/views/topology/relation.js
   M app/views/topology/service.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: app/views/topology/relation.js
=== modified file 'app/views/topology/relation.js'
--- app/views/topology/relation.js 2013-03-22 10:28:31 +0000
+++ app/views/topology/relation.js 2013-03-28 14:22:42 +0000
@@ -470,7 +470,8 @@

      _removeRelationCallback: function(view,
              relationElement, relationId, confirmButton, ev) {
- var db = this.get('component').get('db');
+ var topo = this.get('component'),
+ db = topo.get('db');
        var service = this.get('model');
        if (ev.err) {
          db.notifications.add(
@@ -486,7 +487,7 @@
          // Remove the relation from the DB.
          db.relations.remove(db.relations.getById(relationId));
          // Redraw the graph and reattach events.
- db.fire('update');
+ topo.update();
        }
        view.get('rmrelation_dialog').hide();
        view.get('rmrelation_dialog').destroy();
@@ -697,19 +698,20 @@
       * role: 'client or server'
       * }]
       */
- addRelationEnd: function(endpoints, view, context) {
+ addRelationEnd: function(endpoints, module) {
        // Redisplay all services
- view.cancelRelationBuild();
+ module.cancelRelationBuild();

        // Get the vis, and links, build the new relation.
- var vis = view.get('component').vis;
- var env = view.get('component').get('env');
- var db = view.get('component').get('db');
- var source = view.get('addRelationStart_service');
+ var topo = module.get('component');
+ var vis = topo.vis;
+ var env = topo.get('env');
+ var db = topo.get('db');
+ var source = module.get('addRelationStart_service');
        var relation_id = 'pending-' + endpoints[0][0] + endpoints[1][0];

        if (endpoints[0][0] === endpoints[1][0]) {
- view.set('currentServiceClickAction', 'hideServiceMenu');
+ module.set('currentServiceClickAction', 'hideServiceMenu');
          return;
        }

@@ -724,21 +726,20 @@

        // Firing the update event on the db will properly redraw the
        // graph and reattach events.
- //db.fire('update');
- view.get('component').bindAllD3Events();
- view.update();
+ topo.update();
+ topo.bindAllD3Events();
...

[Jeff Pihach (hatch)]

Thanks for this cleanup LGTM with trivial

https://codereview.appspot.com/8083043/diff/1/app/views/topology/relation.js
File app/views/topology/relation.js (right):

https://codereview.appspot.com/8083043/diff/1/app/views/topology/relation.js#newcode701
app/views/topology/relation.js:701: addRelationEnd: function(endpoints,
module) {
Could you document this method please :)

https://codereview.appspot.com/8083043/

[lp:~bcsaller/juju-gui/update-reductions]

467. By Benjamin Saller on 2013-03-28

expanded doc as requested in review

[Benjamin Saller (bcsaller)]

Thanks for review.

https://codereview.appspot.com/8083043/diff/1/app/views/topology/relation.js
File app/views/topology/relation.js (right):

https://codereview.appspot.com/8083043/diff/1/app/views/topology/relation.js#newcode701
app/views/topology/relation.js:701: addRelationEnd: function(endpoints,
module) {
On 2013/03/28 14:50:33, jeff.pihach wrote:
> Could you document this method please :)

Updated doc to yuidoc. Pushed but not reproposed.

https://codereview.appspot.com/8083043/

[Gary Poster (gary)]

LGTM. nice cleanup. Thank you.

Gary

https://codereview.appspot.com/8083043/

[Benjamin Saller (bcsaller)]

*** Submitted:

Remove some db.update calls

Given the current model we can't remove all calls
to db.fire('update') but we can remove them from
anything using the topology. Change 'update' events
(which redispatch) so simply call topo.update().

service and charm views still redispatch.

R=jeff.pihach, gary.poster
CC=
https://codereview.appspot.com/8083043

https://codereview.appspot.com/8083043/

=== modified file 'app/views/topology/relation.js'
--- app/views/topology/relation.js	2013-03-22 10:28:31 +0000
+++ app/views/topology/relation.js	2013-03-28 15:14:21 +0000
@@ -470,7 +470,8 @@
 
     _removeRelationCallback: function(view,
             relationElement, relationId, confirmButton, ev) {
-      var db = this.get('component').get('db');
+      var topo = this.get('component'),
+          db = topo.get('db');
       var service = this.get('model');
       if (ev.err) {
         db.notifications.add(
@@ -486,7 +487,7 @@
         // Remove the relation from the DB.
         db.relations.remove(db.relations.getById(relationId));
         // Redraw the graph and reattach events.
-        db.fire('update');
+        topo.update();
       }
       view.get('rmrelation_dialog').hide();
       view.get('rmrelation_dialog').destroy();
@@ -688,28 +689,33 @@
     },
 
     /*
-     * Fired when clicking the second service is clicked in the
+     *
+     * Fired when the second service is clicked in the
      * add relation flow.
      *
-     * :param endpoints: array of two endpoints, each in the form
+     * @method addRelationEnd
+     * @param endpoints {Array} array of two endpoints, each in the form
      *   ['service name', {
      *     name: 'endpoint type',
      *     role: 'client or server'
-     *   }]
+     *   }].
+     * @param module {RelationModule}
+     * @return undefined Side-effects only.
      */
-    addRelationEnd: function(endpoints, view, context) {
+    addRelationEnd: function(endpoints, module) {
       // Redisplay all services
-      view.cancelRelationBuild();
+      module.cancelRelationBuild();
 
       // Get the vis, and links, build the new relation.
-      var vis = view.get('component').vis;
-      var env = view.get('component').get('env');
-      var db = view.get('component').get('db');
-      var source = view.get('addRelationStart_service');
+      var topo = module.get('component');
+      var vis = topo.vis;
+      var env = topo.get('env');
+      var db = topo.get('db');
+      var source = module.get('addRelationStart_service');
       var relation_id = 'pending-' + endpoints[0][0] + endpoints[1][0];
 
       if (endpoints[0][0] === endpoints[1][0]) {
-        view.set('currentServiceClickAction', 'hideServiceMenu');
+        module.set('currentServiceClickAction', 'hideServiceMenu');
         return;
       }
 
@@ -724,21 +730,20 @@
 
       // Firing the update event on the db will properly redraw the
       // graph and reattach events.
-      //db.fire('update');
-      view.get('component').bindAllD3Events();
-      view.update();
+      topo.update();
+      topo.bindAllD3Events();
 
       // Fire event to add relation in juju.
       // This needs to specify interface in the future.
       env.add_relation(endpoints[0], endpoints[1],
-          Y.bind(this._addRelationCallback, this, view, relation_id)
+          Y.bind(this._addRelationCallback, this, module, relation_id)
       );
-      view.set('currentServiceClickAction', 'hideServiceMenu');
+      module.set('currentServiceClickAction', 'hideServiceMenu');
     },
 
-    _addRelationCallback: function(view, relation_id, ev) {
+    _addRelationCallback: function(module, relation_id, ev) {
       console.log('addRelationCallback reached');
-      var topo = view.get('component');
+      var topo = module.get('component');
       var db = topo.get('db');
       var vis = topo.vis;
       // Remove our pending relation from the DB, error or no.
@@ -772,10 +777,8 @@
           display_name: endpoints[0][1].name
         });
       }
-      // Redraw the graph and reattach events.
-      //db.fire('update');
-      view.get('component').bindAllD3Events();
-      view.update();
+      topo.update();
+      topo.bindAllD3Events();
     },
 
     /*

=== modified file 'app/views/topology/service.js'
--- app/views/topology/service.js	2013-03-27 18:01:33 +0000
+++ app/views/topology/service.js	2013-03-28 15:14:21 +0000
@@ -1060,7 +1060,8 @@
 
     _destroyCallback: function(service, btn, ev) {
       var getModelURL = this.get('component').get('getModelURL'),
-          db = this.get('component').get('db');
+          topo = this.get('component'),
+          db = topo.get('db');
       if (ev.err) {
         db.notifications.add(
             new models.Notification({
@@ -1076,7 +1077,7 @@
           relation.destroy();
         });
         service.destroy();
-        db.fire('update');
+        topo.update();
       }
       this.get('destroy_dialog').hide();
       btn.set('disabled', false);