Merge lp:~frankban/juju-gui/preserve-zoom into lp:juju-gui/experimental

[Francesco Banconi]

Preserve zoom settings in the environment view.

The zoom scale and translate are preserved after a scene redraw
(e.g. when adding/removing relations, destroying services,
navigating to the service view and back to the front page).

https://codereview.appspot.com/6720048/

[Francesco Banconi (frankban)]

Reviewers: mp+130087_code.launchpad.net,

Message:
Please take a look.

Description:
Preserve zoom settings in the environment view.

The zoom scale and translate are preserved after a scene redraw
(e.g. when adding/removing relations, destroying services,
navigating to the service view and back to the front page).

https://code.launchpad.net/~frankban/juju-gui/preserve-zoom/+merge/130087

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/views/environment.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/environment.js
=== modified file 'app/views/environment.js'
--- app/views/environment.js 2012-10-15 19:22:40 +0000
+++ app/views/environment.js 2012-10-17 10:43:51 +0000
@@ -237,6 +237,13 @@
          .y(this.yscale)
          .scaleExtent([0.25, 2.0])
          .on('zoom', function() {
+ var sourceEvent = d3.event.sourceEvent;
+ // Avoid spurious zooming if the event is a double click
inside
+ // a service box.
+ if (sourceEvent.type === 'dblclick' &&
+ Y.one(sourceEvent.target).ancestor('.service')) {
+ return;
+ }
                  // Keep the slider up to date with the scale on other sorts
                  // of zoom interactions
                  var s = self.slider;
@@ -386,7 +393,7 @@
          },

          /*
- * Sync view models with curent db.models.
+ * Sync view models with current db.models.
           */
          updateData: function() {
            //model data
@@ -872,14 +879,17 @@
            });
          },
          renderSlider: function() {
- var self = this;
+ var self = this,
+ value = 100,
+ currentScale = this.get('scale');
            // Build a slider to control zoom level
- // TODO once we have a stored value in view models, use that
- // for the value property, but for now, zoom to 100%
+ if (currentScale) {
+ value = currentScale * 100;
+ }
            var slider = new Y.Slider({
              min: 25,
              max: 200,
- value: 100
+ value: value
            });
            slider.render('#slider-parent');
            slider.after('valueChange', function(evt) {
@@ -949,6 +959,22 @@
                .setAttribute('x', -width / 2);
            });

+ // Preserve zoom when the scene is updated.
+ var changed = false,
+ currentScale = this.get('scale'),
+ currentTranslate = this.get('translate');
+ if (currentTranslate && currentTranslate !==
this.zoom.translate()) {
+ this.zoom.translate(currentTranslate);
+ changed = true;
+ }
+ if (currentScale && currentScale !== this.z...

[Madison Scott-Clary (makyo)]

Trainee review by Matthew. Looks good to me and works well. Thanks for
the catch on double-clicking a service causing a zoom! One minor about
a comment below, and as always, wait for the blessed review.

https://codereview.appspot.com/6720048/diff/1/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/6720048/diff/1/app/views/environment.js#newcode1163
app/views/environment.js:1163: // Store the current value of translate
by copying the event array
Minor: I know the above line was there before, but maybe it deserves a
comment as well, possibly just merged with this comment.

https://codereview.appspot.com/6720048/

[lp:~frankban/juju-gui/preserve-zoom]

191. By Francesco Banconi on 2012-10-17

Added comment where we store the value of scale.

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/6720048/

[Francesco Banconi (frankban)]

Thanks for the review Matthew.

https://codereview.appspot.com/6720048/diff/1/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/6720048/diff/1/app/views/environment.js#newcode1163
app/views/environment.js:1163: // Store the current value of translate
by copying the event array
On 2012/10/17 14:11:34, matthew.scott wrote:
> Minor: I know the above line was there before, but maybe it deserves a
comment
> as well, possibly just merged with this comment.

Done.

https://codereview.appspot.com/6720048/

[Benjamin Saller (bcsaller)]

LGTM with the minor noted.

Thanks

https://codereview.appspot.com/6720048/diff/4001/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/6720048/diff/4001/app/views/environment.js#newcode243
app/views/environment.js:243: if (sourceEvent.type === 'dblclick' &&
.on("dblclick.zoom", null) during the bind will remove the dblclick
behavior.

https://codereview.appspot.com/6720048/

[lp:~frankban/juju-gui/preserve-zoom]

192. By Francesco Banconi on 2012-10-17

Disable zoom everywhere on dblclick.

193. By Francesco Banconi on 2012-10-17

Lint.

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/6720048/

[Francesco Banconi (frankban)]

Thanks Ben.

https://codereview.appspot.com/6720048/diff/4001/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/6720048/diff/4001/app/views/environment.js#newcode243
app/views/environment.js:243: if (sourceEvent.type === 'dblclick' &&
On 2012/10/17 14:35:11, benjamin.saller wrote:
> .on("dblclick.zoom", null) during the bind will remove the dblclick
behavior.

Done.

https://codereview.appspot.com/6720048/

[Francesco Banconi (frankban)]

*** Submitted:

Preserve zoom settings in the environment view.

The zoom scale and translate are preserved after a scene redraw
(e.g. when adding/removing relations, destroying services,
navigating to the service view and back to the front page).

R=matthew.scott, benjamin.saller
CC=
https://codereview.appspot.com/6720048

https://codereview.appspot.com/6720048/

[Kapil Thangavelu (hazmat)]

The dbl click causing zoom, is actually a nice effect that would be nice to
preserve as a transition .. Transient though without perm zoom changes.
Part of the feel of going into a svc details is meant to be zoom into focus
on this service.

On Wednesday, October 17, 2012, Francesco Banconi wrote:

> Thanks Ben.
>
>
> https://codereview.appspot.com/6720048/diff/4001/app/views/environment.js
> File app/views/environment.js (right):
>
>
> https://codereview.appspot.com/6720048/diff/4001/app/views/environment.js#newcode243
> app/views/environment.js:243: if (sourceEvent.type === 'dblclick' &&
> On 2012/10/17 14:35:11, benjamin.saller wrote:
> > .on("dblclick.zoom", null) during the bind will remove the dblclick
> behavior.
>
> Done.
>
> https://codereview.appspot.com/6720048/
>
> --
> https://code.launchpad.net/~frankban/juju-gui/preserve-zoom/+merge/130087
> Your team Juju GUI Hackers is requested to review the proposed merge of
> lp:~frankban/juju-gui/preserve-zoom into lp:juju-gui.
>

=== modified file 'app/views/environment.js'
--- app/views/environment.js	2012-10-15 19:22:40 +0000
+++ app/views/environment.js	2012-10-17 14:57:22 +0000
@@ -254,6 +254,8 @@
             .attr('height', height)
             .append('svg:g')
             .call(zoom)
+              // Disable zoom on double click.
+            .on('dblclick.zoom', null)
             .append('g');
 
           vis.append('svg:rect')
@@ -386,7 +388,7 @@
         },
 
         /*
-         * Sync view models with curent db.models.
+         * Sync view models with current db.models.
          */
         updateData: function() {
           //model data
@@ -872,14 +874,17 @@
           });
         },
         renderSlider: function() {
-          var self = this;
+          var self = this,
+              value = 100,
+              currentScale = this.get('scale');
           // Build a slider to control zoom level
-          // TODO once we have a stored value in view models, use that
-          // for the value property, but for now, zoom to 100%
+          if (currentScale) {
+            value = currentScale * 100;
+          }
           var slider = new Y.Slider({
             min: 25,
             max: 200,
-            value: 100
+            value: value
           });
           slider.render('#slider-parent');
           slider.after('valueChange', function(evt) {
@@ -949,6 +954,22 @@
               .setAttribute('x', -width / 2);
           });
 
+          // Preserve zoom when the scene is updated.
+          var changed = false,
+              currentScale = this.get('scale'),
+              currentTranslate = this.get('translate');
+          if (currentTranslate && currentTranslate !== this.zoom.translate()) {
+            this.zoom.translate(currentTranslate);
+            changed = true;
+          }
+          if (currentScale && currentScale !== this.zoom.scale()) {
+            this.zoom.scale(currentScale);
+            changed = true;
+          }
+          if (changed) {
+            this._fire_zoom(0);
+          }
+
           // Render the slider after the view is attached.
           // Although there is a .syncUI() method on sliders, it does not
           // seem to play well with the app framework: the slider will render
@@ -1133,7 +1154,11 @@
           if (new_scale < 25 || new_scale > 200) {
             evt.scale = this.get('scale');
           }
+          // Store the current value of scale so that it can be restored later.
           this.set('scale', evt.scale);
+          // Store the current value of translate as well, by copying the event
+          // array in order to avoid reference sharing.
+          this.set('translate', evt.translate.slice(0));
           vis.attr('transform', 'translate(' + evt.translate + ')' +
               ' scale(' + evt.scale + ')');
           this.updateServiceMenuLocation();