Merge lp:~bcsaller/juju-gui/exportXY into lp:juju-gui/experimental
- exportXY
- Merge into trunk
| Status: | Merged |
|---|---|
| Merged at revision: | 1136 |
| Proposed branch: | lp:~bcsaller/juju-gui/exportXY |
| Merge into: | lp:juju-gui/experimental |
| Diff against target: |
797 lines (+184/-216) 15 files modified
app/models/models.js (+9/-22) app/store/env/fakebackend.js (+3/-3) app/views/ghost-inspector.js (+21/-42) app/views/topology/relation.js (+5/-6) app/views/topology/service.js (+77/-130) app/views/topology/topology.js (+46/-0) app/views/utils.js (+1/-1) test/test_conflict_ux.js (+2/-1) test/test_environment_view.js (+8/-7) test/test_fakebackend.js (+5/-1) test/test_inspector_constraints.js (+1/-0) test/test_inspector_overview.js (+1/-0) test/test_inspector_settings.js (+1/-0) test/test_model.js (+3/-2) test/test_service_module.js (+1/-1) |
| To merge this branch: | bzr merge lp:~bcsaller/juju-gui/exportXY |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Juju GUI Hackers | Pending | ||
|
Review via email:
|
|||
Commit message
Description of the change
Redo Service Placement
This changes a number of aspects of how service placement is handled. There
may still be some edge cases and QA will need to be extensive as this
has moved through phases where various parts worked and didn't. I believe
this version to be good however.
Position annotations remain on service models now rather than being deleted
when applied. We favor this to having vars such as hasBeenPositioned,
positionedFromGhost and service model x/y (which mixed into BoundingBoxes).
These various complications are mostly gone and the handling of updating
position annotations falls to a single method on the topology. (Though to be
fair that is still called from more than one place).
Further simplification might be possible by unifying the node creation and
node update paths wrt annotations. This didn't however fit in the time
provided.
This also includes a basic fix for always pulling positions from the client
during an export.
| Benjamin Saller (bcsaller) wrote : | # |
| Gary Poster (gary) wrote : | # |
On 2013/10/15 08:26:10, bcsaller wrote:
> Please take a look.
Doing qa first. A lot looks good. There's one class of serious
problems I still see (and I am pretty sure I did not see these during qa
last night). Here are symptom descriptions. They seem very related.
- In a sandbox, go to
http://
and drag the bundle out to deploy it. It will appear and then seem to
disappear. It has moved to the bottom right. You can drag the bundle
up to the center, but within a few seconds it will return to the bottom
right.
- In a real environment, when you first go to the GUI and see only the
GUI service, it shows up in the middle of the canvas and then moves down
to the bottom right. Again, you can drag it to the right place, but it
will return within a minute or so to the bottom right.
The thing that unifies these scenarios is that the services do not begin
with x/y annotations.
I'll look again when you've had a chance to address, or if I you need to
hand it off to us.
| Benjamin Saller (bcsaller) wrote : | # |
I believe you when you say that it didn't happen in QA at the midway
point. My feeling was that it was working better as well before but
trying to get it passing the existing tests forced some changes and
while I thought I got it back into shape it seems this must be resolved.
One difference I know is that I don't publish annotations for packed
elements as this was interfering with imports. They'd get build and the
delta comes back, they get placed, and the annotation set and then the
imports set annotations call fires and another delta comes in, racing
with the cli placement. That change and the case where I decide when to
pan (which should only happen after packing more than one new service)
both need looking at. I'll try to resolve these. If that doesn't work we
can back up the patch a few revisions and try to merge in test changes.
I'll look into this now.
| Gary Poster (gary) wrote : | # |
Hey Ben. I just saw your reply to my qa, which touches on some of my
review comments below. I'm not sure I said thank you the first time:
thank you! this will be a great change, and a nice last (major?) commit
(for now?) :-)
Gary
https:/
File app/app.js (right):
https:/
app/app.js:626: var result = this.db.
why is this necessary if we don't remove x/y annotations anymore?
https:/
File app/models/
https:/
app/models/
indentation needs an extra space.
https:/
app/models/
Same comment as before: if we are handling x/y annotations properly now,
why is the topology necessary?
https:/
File app/views/
https:/
app/views/
ghostAttributes
Note to self: double check that these annotations are actually sent to
server after creation.
https:/
app/views/
annotations);
Why is this necessary? Do we need to trigger an event? If so, please
add a comment to that effect.
https:/
app/views/
Is this the rough equivalent of "options.
serviceName, 'service', ghostService.
https:/
File app/views/
https:/
app/views/
svc });
Why is this no longer necessary?
https:/
File app/views/
https:/
app/views/
previous annotations).
Until we have timestamps associated with our x/y annotations, we will
perpetually have race conditions ISTM. :-(
That's not for this branch, just thinking.
https:/
app/views/
Ah, I bet this is what I'm encountering in qa.
https:/
app/views/
delete that extra space to get the linting right, yeah?
https:/
| Benjamin Saller (bcsaller) wrote : | # |
Thanks, new version pushing now
https:/
File app/views/
https:/
app/views/
ghostAttributes
On 2013/10/15 15:34:16, gary.poster wrote:
> Note to self: double check that these annotations are actually sent to
server
> after creation.
Line ~301 in the callback handler. We know its on the backend at that
point, there is an explicit call.
https:/
app/views/
annotations);
On 2013/10/15 15:34:16, gary.poster wrote:
> Why is this necessary? Do we need to trigger an event? If so, please
add a
> comment to that effect.
I was thinking of handling position changes on annotation change events,
but no, this was a guard rail vs some strange behavior I was seeing, it
shouldn't technically be needed.
https:/
app/views/
On 2013/10/15 15:34:16, gary.poster wrote:
> Is this the rough equivalent of "options.
> serviceName, 'service', ghostService.
It is, it adds support for putting the box in the DRAG_ENDING state with
a timed window in which it shouldn't resend position annotations.
https:/
File app/views/
https:/
app/views/
svc });
On 2013/10/15 15:34:16, gary.poster wrote:
> Why is this no longer necessary?
It shouldn't have been needed for some time. The only ways to position
objects all explicitly request updates to endpoint/relations connected
to the moved object.
https:/
File app/views/
https:/
app/views/
previous annotations).
On 2013/10/15 15:34:16, gary.poster wrote:
> Until we have timestamps associated with our x/y annotations, we will
> perpetually have race conditions ISTM. :-(
> That's not for this branch, just thinking.
This is also the last write wins case where two clients are moving the
same service at the same time, the first dragend will send a delta
event, but the other client still in the drag will ignore it rather than
have it jump mid drag to some new position.
https:/
app/views/
previous annotations).
On 2013/10/15 15:34:16, gary.poster wrote:
> Until we have timestamps associated with our x/y annotations, we will
> perpetually have race conditions ISTM. :-(
> That's not for this branch...
- 1135. By Benjamin Saller
-
review feedback
- 1136. By Benjamin Saller
-
lint
- 1137. By Benjamin Saller
-
work around a test failure, also update to apiv3
- 1138. By Benjamin Saller
-
changes to tests were needed
- 1139. By Benjamin Saller
-
lint
| Benjamin Saller (bcsaller) wrote : | # |
Please take a look.
| Gary Poster (gary) wrote : | # |
Awesome! LGTM and qa good, with trivial. Thank you!
https:/
File app/models/
https:/
app/models/
position when available.
Remove this line
https:/
app/models/
Remove topology from arguments
https:/
File app/views/
https:/
app/views/
panToCentroid seems more apt. <shrug>
https:/
File app/views/
https:/
app/views/
timeout? wait? window is overloaded. <shrug>
https:/
File test/test_
https:/
test/test_
function() {
so...should we uncomment this or delete this? I didn't quite follow
from your reply.
https:/
test/test_
function() {
Should we uncomment this or delete this? I didn't quite follow from
your reply.
| Madison Scott-Clary (makyo) wrote : | # |
LGTMly code, QAing quickly.
https:/
File app/views/
https:/
app/views/
On 2013/10/15 22:16:56, gary.poster wrote:
> timeout? wait? window is overloaded. <shrug>
+1 on timeout
https:/
File test/test_
https:/
test/test_
function() {
On 2013/10/15 22:16:56, gary.poster wrote:
> so...should we uncomment this or delete this? I didn't quite follow
from your
> reply.
From my branch, these tests will pass/still test what they if you reset
the db; I think it's worth testing, but that means that tests aren't
written properly. Rewrite, or maybe move to their own test with
it.skip?
https:/
test/test_
function() {
On 2013/10/15 22:16:56, gary.poster wrote:
> Should we uncomment this or delete this? I didn't quite follow from
your reply.
as above
| Benjamin Saller (bcsaller) wrote : | # |
Thanks again, sorry for so many rounds.
https:/
File app/models/
https:/
app/models/
position when available.
On 2013/10/15 22:16:56, gary.poster wrote:
> Remove this line
Doh! Thanks
https:/
File app/views/
https:/
app/views/
On 2013/10/15 22:16:56, gary.poster wrote:
> timeout? wait? window is overloaded. <shrug>
Done.
https:/
File test/test_
https:/
test/test_
function() {
On 2013/10/15 22:16:56, gary.poster wrote:
> Should we uncomment this or delete this? I didn't quite follow from
your reply.
Done.
| Benjamin Saller (bcsaller) wrote : | # |
Thanks to you both.
https:/
File test/test_
https:/
test/test_
function() {
On 2013/10/15 22:29:57, matthew.scott wrote:
> On 2013/10/15 22:16:56, gary.poster wrote:
> > so...should we uncomment this or delete this? I didn't quite follow
from your
> > reply.
> From my branch, these tests will pass/still test what they if you
reset the db;
> I think it's worth testing, but that means that tests aren't written
properly.
> Rewrite, or maybe move to their own test with it.skip?
I deleted these for now, but if someone wants to check the QA issue gary
found with reloading positioned objects from a real env, adding in
improvements here might fit.
| Benjamin Saller (bcsaller) wrote : | # |
*** Submitted:
Redo Service Placement
This changes a number of aspects of how service placement is handled.
There
may still be some edge cases and QA will need to be extensive as this
has moved through phases where various parts worked and didn't. I
believe
this version to be good however.
Position annotations remain on service models now rather than being
deleted
when applied. We favor this to having vars such as hasBeenPositioned,
positionedFromGhost and service model x/y (which mixed into
BoundingBoxes).
These various complications are mostly gone and the handling of updating
position annotations falls to a single method on the topology. (Though
to be
fair that is still called from more than one place).
Further simplification might be possible by unifying the node creation
and
node update paths wrt annotations. This didn't however fit in the time
provided.
This also includes a basic fix for always pulling positions from the
client
during an export.
R=gary.poster, benjamin.saller, matthew.scott
CC=
https:/
Preview Diff
| 1 | === modified file 'app/models/models.js' |
| 2 | --- app/models/models.js 2013-10-10 17:06:41 +0000 |
| 3 | +++ app/models/models.js 2013-10-15 20:38:14 +0000 |
| 4 | @@ -314,19 +314,6 @@ |
| 5 | pending: { |
| 6 | value: false |
| 7 | }, |
| 8 | - |
| 9 | - /** |
| 10 | - Flag from ghost inspector to service topology. Helps topology |
| 11 | - keep from unnecessarily jumping the service around. Essentially |
| 12 | - an internal value that should be ignored except by this machinery. |
| 13 | - |
| 14 | - @attribute placeFromGhostPosition |
| 15 | - @default false |
| 16 | - @type {Boolean} |
| 17 | - */ |
| 18 | - placeFromGhostPosition: { |
| 19 | - value: false |
| 20 | - }, |
| 21 | life: { |
| 22 | value: ALIVE |
| 23 | }, |
| 24 | @@ -1149,9 +1136,10 @@ |
| 25 | * that. |
| 26 | * |
| 27 | * @method exportDeployer |
| 28 | + * @param {Object} [topology] used to get position when available. |
| 29 | * @return {Object} export object suitable for serialization. |
| 30 | */ |
| 31 | - exportDeployer: function() { |
| 32 | + exportDeployer: function(topology) { |
| 33 | var self = this, |
| 34 | serviceList = this.services, |
| 35 | relationList = this.relations, |
| 36 | @@ -1203,15 +1191,14 @@ |
| 37 | serviceData.constraints = constraints; |
| 38 | } |
| 39 | |
| 40 | - var annotations = service.get('annotations'); |
| 41 | - if (annotations && annotations['gui-x']) { |
| 42 | - // XXX: Only expose position. Currently these are position absolute |
| 43 | - // rather than relative. |
| 44 | - serviceData.annotations = { |
| 45 | - 'gui-x': annotations['gui-x'], |
| 46 | - 'gui-y': annotations['gui-y'] |
| 47 | - }; |
| 48 | + // XXX: Only expose position. Currently these are position absolute |
| 49 | + // rather than relative. |
| 50 | + var anno = service.get('annotations'); |
| 51 | + if (anno && anno['gui-x'] && anno['gui-y']) { |
| 52 | + serviceData.annotations = {'gui-x': anno['gui-x'], |
| 53 | + 'gui-y': anno['gui-y']}; |
| 54 | } |
| 55 | + |
| 56 | result.envExport.services[service.get('id')] = serviceData; |
| 57 | }); |
| 58 | |
| 59 | |
| 60 | === modified file 'app/store/env/fakebackend.js' |
| 61 | --- app/store/env/fakebackend.js 2013-10-10 17:06:41 +0000 |
| 62 | +++ app/store/env/fakebackend.js 2013-10-15 20:38:14 +0000 |
| 63 | @@ -1524,9 +1524,9 @@ |
| 64 | var serviceData = ingestedData.services[serviceId]; |
| 65 | |
| 66 | // Force the annotation update (deploy doesn't handle this). |
| 67 | - var annotiations = serviceData.annotations; |
| 68 | - if (annotiations) { |
| 69 | - service.set('annotations', annotiations); |
| 70 | + var anno = serviceData.annotations; |
| 71 | + if (anno) { |
| 72 | + self.updateAnnotations(service.get('id'), anno); |
| 73 | } |
| 74 | |
| 75 | // Expose |
| 76 | |
| 77 | === modified file 'app/views/ghost-inspector.js' |
| 78 | --- app/views/ghost-inspector.js 2013-09-30 11:08:14 +0000 |
| 79 | +++ app/views/ghost-inspector.js 2013-10-15 20:38:14 +0000 |
| 80 | @@ -59,16 +59,14 @@ |
| 81 | var ghostService = this.db.services.ghostService(charm); |
| 82 | if (ghostAttributes !== undefined) { |
| 83 | if (ghostAttributes.coordinates !== undefined) { |
| 84 | - ghostService.set('x', ghostAttributes.coordinates[0]); |
| 85 | - ghostService.set('y', ghostAttributes.coordinates[1]); |
| 86 | + var annotations = ghostService.get('annotations'); |
| 87 | + annotations['gui-x'] = ghostAttributes.coordinates[0]; |
| 88 | + annotations['gui-y'] = ghostAttributes.coordinates[1]; |
| 89 | } |
| 90 | ghostService.set('icon', ghostAttributes.icon); |
| 91 | - // Set the dragged attribute to true so that the x/y coords are |
| 92 | - // stored in annotations as well as on the service box. |
| 93 | - ghostService.set('hasBeenPositioned', true); |
| 94 | } |
| 95 | - var environment = this.views.environment.instance, |
| 96 | - ghostInspector = environment.createServiceInspector(ghostService); |
| 97 | + var environment = this.views.environment.instance; |
| 98 | + environment.createServiceInspector(ghostService); |
| 99 | } |
| 100 | }; |
| 101 | |
| 102 | @@ -251,7 +249,9 @@ |
| 103 | _deployCallbackHandler: function(serviceName, config, constraints, e) { |
| 104 | var options = this.options, |
| 105 | db = options.db, |
| 106 | - ghostService = this.model; |
| 107 | + ghostService = this.model, |
| 108 | + environmentView = this.options.environment, |
| 109 | + topo = environmentView.topo; |
| 110 | |
| 111 | if (e.err) { |
| 112 | db.notifications.add( |
| 113 | @@ -270,41 +270,13 @@ |
| 114 | level: 'info' |
| 115 | })); |
| 116 | |
| 117 | - // Update the annotations with the box's x/y coordinates if |
| 118 | - // they have been set by dragging the ghost. |
| 119 | - if (ghostService.get('hasBeenPositioned')) { |
| 120 | - options.env.update_annotations( |
| 121 | - serviceName, 'service', |
| 122 | - { 'gui-x': ghostService.get('x'), |
| 123 | - 'gui-y': ghostService.get('y') }, |
| 124 | - function() { |
| 125 | - // Make sure that annotations are set on the ghost |
| 126 | - // service before they come back from the delta to |
| 127 | - // prevent the service from jumping to the middle of |
| 128 | - // the canvas and back. |
| 129 | - var annotations = ghostService.get('annotations'); |
| 130 | - if (!annotations) { |
| 131 | - annotations = {}; |
| 132 | - } |
| 133 | - Y.mix(annotations, { |
| 134 | - 'gui-x': ghostService.get('x'), |
| 135 | - 'gui-y': ghostService.get('y') |
| 136 | - }); |
| 137 | - ghostService.set('annotations', annotations); |
| 138 | - // The x/y attributes need to be removed to prevent |
| 139 | - // lingering position problems after the service is |
| 140 | - // positioned by the update code. |
| 141 | - ghostService.removeAttr('x'); |
| 142 | - ghostService.removeAttr('y'); |
| 143 | - }); |
| 144 | - } |
| 145 | - |
| 146 | // Now that we are using the same model for the ghost and service views |
| 147 | // we need to close the inspector to deactivate the databinding |
| 148 | // before setting else we end up with a race condition on nodes which |
| 149 | // no longer exist. |
| 150 | this.closeInspector(); |
| 151 | |
| 152 | + var ghostId = ghostService.get('id'); |
| 153 | ghostService.setAttrs({ |
| 154 | id: serviceName, |
| 155 | displayName: undefined, |
| 156 | @@ -314,11 +286,18 @@ |
| 157 | constraints: constraints |
| 158 | }); |
| 159 | |
| 160 | - // This flag is used twice in the service topology module as a marker |
| 161 | - // to know that it should not move the service or the canvas around |
| 162 | - // (as opposed to services received from the environment). |
| 163 | - ghostService.set('placeFromGhostPosition', true); |
| 164 | - this.options.environment.createServiceInspector(ghostService); |
| 165 | + // Transition the ghost viewModel to the new |
| 166 | + // service. It's alive! |
| 167 | + var boxModel = topo.service_boxes[ghostId]; |
| 168 | + boxModel.id = serviceName; |
| 169 | + boxModel.pending = false; |
| 170 | + delete topo.service_boxes[ghostId]; |
| 171 | + topo.service_boxes[serviceName] = boxModel; |
| 172 | + |
| 173 | + // Set to initial UI state. |
| 174 | + environmentView.createServiceInspector(ghostService); |
| 175 | + topo.showMenu(serviceName); |
| 176 | + topo.annotateBoxPosition(boxModel); |
| 177 | } |
| 178 | |
| 179 | }; |
| 180 | |
| 181 | === modified file 'app/views/topology/relation.js' |
| 182 | --- app/views/topology/relation.js 2013-09-19 20:11:53 +0000 |
| 183 | +++ app/views/topology/relation.js 2013-10-15 20:38:14 +0000 |
| 184 | @@ -176,17 +176,11 @@ |
| 185 | |
| 186 | var topo = this.get('component'); |
| 187 | var db = topo.get('db'); |
| 188 | - var self = this; |
| 189 | var relations = db.relations.toArray(); |
| 190 | this.relations = this.decorateRelations(relations); |
| 191 | this.updateLinks(); |
| 192 | this.updateSubordinateRelationsCount(); |
| 193 | |
| 194 | - // Ensure that link endpoints are up-to-date. |
| 195 | - Y.each(topo.service_boxes, function(svc, key) { |
| 196 | - self.updateLinkEndpoints({ service: svc }); |
| 197 | - }); |
| 198 | - |
| 199 | return this; |
| 200 | }, |
| 201 | |
| 202 | @@ -258,6 +252,11 @@ |
| 203 | updateLinkEndpoints: function(evt) { |
| 204 | var self = this; |
| 205 | var service = evt.service; |
| 206 | + |
| 207 | + if (!service.relations || service.relations.size() === 0) { |
| 208 | + return; |
| 209 | + } |
| 210 | + |
| 211 | Y.each(Y.Array.filter(self.relations, function(relation) { |
| 212 | return relation.source.id === service.id || |
| 213 | relation.target.id === service.id; |
| 214 | |
| 215 | === modified file 'app/views/topology/service.js' |
| 216 | --- app/views/topology/service.js 2013-10-11 20:13:50 +0000 |
| 217 | +++ app/views/topology/service.js 2013-10-15 20:38:14 +0000 |
| 218 | @@ -80,13 +80,14 @@ |
| 219 | // This is done after the services_boxes |
| 220 | // binding as the event handler will |
| 221 | // use that index. |
| 222 | + var movedNodes = 0; |
| 223 | node.each(function(d) { |
| 224 | var service = d.model, |
| 225 | annotations = service.get('annotations'), |
| 226 | x, y; |
| 227 | |
| 228 | // If there are no annotations or the service is being dragged |
| 229 | - if (!annotations || service.inDrag === views.DRAG_ACTIVE) { |
| 230 | + if (!annotations || d.inDrag) { |
| 231 | return; |
| 232 | } |
| 233 | |
| 234 | @@ -95,23 +96,24 @@ |
| 235 | // node, as the annotations may have been set in another session. |
| 236 | x = annotations['gui-x']; |
| 237 | y = annotations['gui-y']; |
| 238 | - if (!d || |
| 239 | - (x !== undefined && x !== d.x) || |
| 240 | - (y !== undefined && y !== d.y)) { |
| 241 | - // Delete gui-x and gui-y from annotations as we use the values. |
| 242 | - // This is to prevent deltas coming in on a service while it is |
| 243 | - // being dragged from resetting its position during the drag. |
| 244 | - |
| 245 | - delete annotations['gui-x']; |
| 246 | - delete annotations['gui-y']; |
| 247 | + if (x === undefined || y === undefined) { |
| 248 | + return; |
| 249 | + } |
| 250 | + x = parseFloat(x); |
| 251 | + y = parseFloat(y); |
| 252 | + if ((x !== d.x) || (y !== d.y)) { |
| 253 | // Only update position if we're not already in a drag state (the |
| 254 | // current drag supercedes any previous annotations). |
| 255 | - var fromGhost = d.model.get('placeFromGhostPosition'); |
| 256 | if (!d.inDrag) { |
| 257 | - var useTransitions = self.get('useTransitions') && !fromGhost; |
| 258 | + var useTransitions = self.get('useTransitions'); |
| 259 | self.drag.call(this, d, self, {x: x, y: y}, useTransitions); |
| 260 | + movedNodes += 1; |
| 261 | + topo.annotateBoxPosition(d); |
| 262 | } |
| 263 | }}); |
| 264 | + if (movedNodes > 1) { |
| 265 | + this.findCentroid(); |
| 266 | + } |
| 267 | |
| 268 | // Mark subordinates as such. This is needed for when a new service |
| 269 | // is created. |
| 270 | @@ -167,18 +169,6 @@ |
| 271 | 'x': 64, |
| 272 | 'y': 47 * 0.8}); |
| 273 | |
| 274 | - // Handle the last step of models that were made locally from ghosts. |
| 275 | - node.filter(function(d) { |
| 276 | - return d.model.get('placeFromGhostPosition'); |
| 277 | - }).each(function(d) { |
| 278 | - // Show the service menu from the start. |
| 279 | - self.showServiceMenu(d); |
| 280 | - // This flag has served its purpose, at initialization time on the |
| 281 | - // canvas. Remove it, so future changes will have the usual |
| 282 | - // behavior. |
| 283 | - d.model.set('placeFromGhostPosition', false); |
| 284 | - }); |
| 285 | - |
| 286 | // Landscape badge |
| 287 | if (landscape) { |
| 288 | node.each(function(d) { |
| 289 | @@ -828,7 +818,7 @@ |
| 290 | var topo = context.get('component'); |
| 291 | context.longClickTimer = Y.later(750, this, function(d, e) { |
| 292 | // Provide some leeway for accidental dragging. |
| 293 | - if ((Math.abs(box.x - box.oldX) + Math.abs(box.y - box.oldY)) / |
| 294 | + if ((Math.abs(box.x - box.px) + Math.abs(box.y - box.py)) / |
| 295 | 2 > 5) { |
| 296 | return; |
| 297 | } |
| 298 | @@ -868,8 +858,6 @@ |
| 299 | * @method dragstart |
| 300 | */ |
| 301 | dragstart: function(box, self) { |
| 302 | - box.oldX = box.x; |
| 303 | - box.oldY = box.y; |
| 304 | box.inDrag = views.DRAG_START; |
| 305 | }, |
| 306 | |
| 307 | @@ -884,36 +872,18 @@ |
| 308 | if (topo.buildingRelation) { |
| 309 | topo.ignoreServiceClick = true; |
| 310 | topo.fire('addRelationDragEnd'); |
| 311 | - } |
| 312 | - else { |
| 313 | - |
| 314 | + } else { |
| 315 | // If the service hasn't been dragged (in the case of long-click to |
| 316 | // add relation, or a double-fired event) or the old and new |
| 317 | // coordinates are the same, exit. |
| 318 | - if (!box.inDrag || |
| 319 | - (box.oldX === box.x && |
| 320 | - box.oldY === box.y)) { |
| 321 | - return; |
| 322 | - } |
| 323 | - |
| 324 | - // If the service is still pending, persist x/y coordinates in |
| 325 | - // order to set them as annotations when the service is created. |
| 326 | - if (box.pending) { |
| 327 | - box.model.set('hasBeenPositioned', true); |
| 328 | - box.model.set('x', box.x); |
| 329 | - box.model.set('y', box.y); |
| 330 | + if (box.inDrag !== views.DRAG_ACTIVE) { |
| 331 | return; |
| 332 | } |
| 333 | |
| 334 | // If the service has been dragged, ignore the subsequent service |
| 335 | // click event. |
| 336 | topo.ignoreServiceClick = true; |
| 337 | - |
| 338 | - topo.get('env').update_annotations( |
| 339 | - box.id, 'service', {'gui-x': box.x, 'gui-y': box.y}, |
| 340 | - function() { |
| 341 | - box.inDrag = false; |
| 342 | - }); |
| 343 | + topo.annotateBoxPosition(box); |
| 344 | } |
| 345 | }, |
| 346 | |
| 347 | @@ -954,8 +924,6 @@ |
| 348 | if (pos) { |
| 349 | box.x = pos.x; |
| 350 | box.y = pos.y; |
| 351 | - // Explicitly reassign data. |
| 352 | - selection = selection.data([box]); |
| 353 | } else { |
| 354 | box.x += d3.event.dx; |
| 355 | box.y += d3.event.dy; |
| 356 | @@ -1011,15 +979,6 @@ |
| 357 | this.service_scale_height = d3.scale.log().range([64, 100]); |
| 358 | } |
| 359 | |
| 360 | - if (!this.tree) { |
| 361 | - this.tree = d3.layout.unscaledPack() |
| 362 | - .size([width, height]) |
| 363 | - .value(function(d) { |
| 364 | - return Math.max(d.unit_count, 1); |
| 365 | - }) |
| 366 | - .padding(300); |
| 367 | - } |
| 368 | - |
| 369 | if (!this.dragBehavior) { |
| 370 | this.dragBehavior = d3.behavior.drag() |
| 371 | .on('dragstart', function(d) { self.dragstart.call(this, d, self);}) |
| 372 | @@ -1039,43 +998,52 @@ |
| 373 | // entire graph. As a short term work around we layout only new |
| 374 | // nodes. New nodes are those that haven't been positioned by drag |
| 375 | // and drop, or those who don't have position attributes/annotations. |
| 376 | - var vertices; |
| 377 | - var fromGhost = false; |
| 378 | - var new_services = Y.Object.values(topo.service_boxes) |
| 379 | + var vertices = []; |
| 380 | + Y.each(topo.service_boxes, function(boundingBox) { |
| 381 | + var annotations = boundingBox.annotations; |
| 382 | + if (annotations['gui-x'] && boundingBox.x === undefined) { |
| 383 | + boundingBox.x = annotations['gui-x']; |
| 384 | + } |
| 385 | + if (annotations['gui-y'] && boundingBox.y === undefined) { |
| 386 | + boundingBox.y = annotations['gui-y']; |
| 387 | + } |
| 388 | + }); |
| 389 | + |
| 390 | + // new_service_boxes are those w/o current x/y pos and no |
| 391 | + // annotations. |
| 392 | + var new_service_boxes = Y.Object.values(topo.service_boxes) |
| 393 | .filter(function(boundingBox) { |
| 394 | var annotations = boundingBox.model.get('annotations'); |
| 395 | - return !boundingBox.hasBeenPositioned || |
| 396 | - (!Y.Lang.isNumber(boundingBox.x) && |
| 397 | - !(annotations && annotations['gui-x'])); |
| 398 | + return ((!Y.Lang.isNumber(boundingBox.x) && |
| 399 | + !(annotations && annotations['gui-x']))); |
| 400 | }); |
| 401 | - if (new_services.length > 0) { |
| 402 | + |
| 403 | + if (new_service_boxes.length > 0) { |
| 404 | // If the there is only one new service and it's pending (as in, it was |
| 405 | // added via the charm panel as a ghost), position it intelligently and |
| 406 | // set its position coordinates such that they'll be saved when the |
| 407 | // service is actually created. Otherwise, rely on our pack layout (as |
| 408 | // in the case of opening an unannotated environment for the first |
| 409 | // time). |
| 410 | - var pendingServicePlaced = false; |
| 411 | - if (new_services.length === 1 && |
| 412 | - new_services[0].model.get('pending')) { |
| 413 | - pendingServicePlaced = true; |
| 414 | + if (new_service_boxes.length === 1 && |
| 415 | + new_service_boxes[0].model.get('pending') && |
| 416 | + (new_service_boxes[0].x === undefined || |
| 417 | + new_service_boxes[0].y === undefined)) { |
| 418 | // Get a coordinate outside the cluster of existing services. |
| 419 | var coords = topo.servicePointOutside(); |
| 420 | // Set the coordinates on both the box model and the service |
| 421 | // model. |
| 422 | - new_services[0].x = coords[0]; |
| 423 | - new_services[0].y = coords[1]; |
| 424 | - new_services[0].model.set('x', coords[0]); |
| 425 | - new_services[0].model.set('y', coords[1]); |
| 426 | - // This ensures that the x/y coordinates will be saved as |
| 427 | - // annotations. |
| 428 | - new_services[0].model.set('hasBeenPositioned', true); |
| 429 | + new_service_boxes[0].x = coords[0]; |
| 430 | + new_service_boxes[0].y = coords[1]; |
| 431 | // Set the centroid to the new service's position |
| 432 | - topo.centroid = coords; |
| 433 | - topo.fire('panToPoint', {point: topo.centroid}); |
| 434 | + topo.fire('panToPoint', {point: coords}); |
| 435 | } else { |
| 436 | - this.tree.nodes({children: new_services}); |
| 437 | - if (new_services.length < Y.Object.size(topo.service_boxes)) { |
| 438 | + d3.layout.unscaledPack() |
| 439 | + .size([width, height]) |
| 440 | + .value(function(d) { return Math.max(d.unit_count, 1); }) |
| 441 | + .padding(300) |
| 442 | + .nodes({children: new_service_boxes}); |
| 443 | + if (new_service_boxes.length < Y.Object.size(topo.service_boxes)) { |
| 444 | // If we have new services that do not have x/y coords and are |
| 445 | // not pending, then they've likely been created from the CLI. |
| 446 | // In this case, to avoid placing them overlaying any existing |
| 447 | @@ -1084,49 +1052,34 @@ |
| 448 | // will result in annotations being set in the environment |
| 449 | // below. |
| 450 | var pointOutside = topo.servicePointOutside(); |
| 451 | - Y.each(new_services, function(service) { |
| 452 | - service.x += pointOutside[0] - service.x; |
| 453 | - service.y += pointOutside[1] - service.y; |
| 454 | - service.model.set('x', service.x); |
| 455 | - service.model.set('y', service.y); |
| 456 | + Y.each(new_service_boxes, function(boxModel) { |
| 457 | + boxModel.x += pointOutside[0] - boxModel.x; |
| 458 | + boxModel.y += pointOutside[1] - boxModel.y; |
| 459 | }); |
| 460 | } |
| 461 | - } |
| 462 | - // Update annotations settings position on backend (but only do |
| 463 | - // this if there is no existing annotations). |
| 464 | - if (!pendingServicePlaced) { |
| 465 | - vertices = []; |
| 466 | - } |
| 467 | - Y.each(new_services, function(box) { |
| 468 | - if (box.model.get('placeFromGhostPosition')) { |
| 469 | - fromGhost = true; |
| 470 | - } |
| 471 | - var existing = box.model.get('annotations') || {}; |
| 472 | - if (!existing && !existing['gui-x']) { |
| 473 | - topo.get('env').update_annotations( |
| 474 | - box.id, 'service', {'gui-x': box.x, 'gui-y': box.y}, |
| 475 | - function() { |
| 476 | - box.inDrag = false; |
| 477 | - }); |
| 478 | - vertices.push([box.x || 0, box.y || 0]); |
| 479 | - } else { |
| 480 | - if (vertices) { |
| 481 | - vertices.push([ |
| 482 | - existing['gui-x'] || (box.x || 0), |
| 483 | - existing['gui-y'] || (box.y || 0) |
| 484 | - ]); |
| 485 | + |
| 486 | + Y.each(new_service_boxes, function(box) { |
| 487 | + var existing = box.model.get('annotations') || {}; |
| 488 | + if (!existing || !existing['gui-x']) { |
| 489 | + vertices.push([box.x || 0, box.y || 0]); |
| 490 | + topo.annotateBoxPosition(box, false); |
| 491 | + } else { |
| 492 | + if (vertices.length > 0) { |
| 493 | + vertices.push([ |
| 494 | + existing['gui-x'] || (box.x || 0), |
| 495 | + existing['gui-y'] || (box.y || 0) |
| 496 | + ]); |
| 497 | + } |
| 498 | } |
| 499 | - } |
| 500 | - }); |
| 501 | - } |
| 502 | - if (!topo.centroid || vertices) { |
| 503 | + }); |
| 504 | + } |
| 505 | // Find the centroid of our hull of services and inform the |
| 506 | // topology. |
| 507 | - if (!vertices) { |
| 508 | - vertices = topoUtils.serviceBoxesToVertices(topo.service_boxes); |
| 509 | + if (vertices.length) { |
| 510 | + this.findCentroid(vertices); |
| 511 | } |
| 512 | - this.findAndSetCentroid(vertices, fromGhost); |
| 513 | } |
| 514 | + |
| 515 | // enter |
| 516 | node |
| 517 | .enter().append('g') |
| 518 | @@ -1135,10 +1088,10 @@ |
| 519 | 'class': function(d) { |
| 520 | return (d.subordinate ? 'subordinate ' : '') + |
| 521 | (d.pending ? 'pending ' : '') + 'service'; |
| 522 | - }, |
| 523 | - 'transform': function(d) {return d.translateStr;}}) |
| 524 | + }}) |
| 525 | .call(this.dragBehavior) |
| 526 | - .call(self.createServiceNode, self); |
| 527 | + .call(self.createServiceNode, self) |
| 528 | + .attr('transform', function(d) { return d.translateStr; }); |
| 529 | |
| 530 | // Update all nodes. |
| 531 | self.updateServiceNodes(node); |
| 532 | @@ -1161,26 +1114,20 @@ |
| 533 | panToCenter: function(evt) { |
| 534 | var topo = this.get('component'); |
| 535 | var vertices = topoUtils.serviceBoxesToVertices(topo.service_boxes); |
| 536 | - this.findAndSetCentroid(vertices); |
| 537 | + this.findCentroid(vertices); |
| 538 | }, |
| 539 | |
| 540 | /** |
| 541 | Given a set of vertices, find the centroid and pan to that location. |
| 542 | |
| 543 | - @method findAndSetCentroid |
| 544 | + @method findCentroid |
| 545 | @param {array} vertices A list of vertices in the form [x, y]. |
| 546 | @return {undefined} Side effects only. |
| 547 | */ |
| 548 | - findAndSetCentroid: function(vertices, preventPan) { |
| 549 | + findCentroid: function(vertices) { |
| 550 | var topo = this.get('component'), |
| 551 | centroid = topoUtils.centroid(vertices); |
| 552 | - // The centroid is set on the topology object due to the fact that it is |
| 553 | - // used as a sigil to tell whether or not to pan to the point after the |
| 554 | - // first delta. |
| 555 | - topo.centroid = centroid; |
| 556 | - if (!preventPan) { |
| 557 | - topo.fire('panToPoint', {point: topo.centroid}); |
| 558 | - } |
| 559 | + topo.fire('panToPoint', {point: centroid}); |
| 560 | }, |
| 561 | |
| 562 | /** |
| 563 | |
| 564 | === modified file 'app/views/topology/topology.js' |
| 565 | --- app/views/topology/topology.js 2013-10-04 18:37:47 +0000 |
| 566 | +++ app/views/topology/topology.js 2013-10-15 20:38:14 +0000 |
| 567 | @@ -186,7 +186,52 @@ |
| 568 | return utils.pointOutside( |
| 569 | utils.serviceBoxesToVertices(existingBoxes), |
| 570 | this.get('servicePadding')); |
| 571 | + }, |
| 572 | + |
| 573 | + /** |
| 574 | + Show the menu for a given service |
| 575 | + |
| 576 | + @method showMenu |
| 577 | + @param {String} serviceId |
| 578 | + */ |
| 579 | + showMenu: function(serviceId) { |
| 580 | + var serviceModule = this.modules.ServiceModule; |
| 581 | + if (!serviceModule) { return;} |
| 582 | + var boxModel = this.service_boxes[serviceId]; |
| 583 | + serviceModule.showServiceMenu(boxModel); |
| 584 | + }, |
| 585 | + |
| 586 | + /** |
| 587 | + Record a new box position on the backend. This maintains the proper drag |
| 588 | + state. This method also transitions the viewModel to a DRAG_ENDING state |
| 589 | + with a timeout. During this window the box will behave as if its in a drag |
| 590 | + state refusing annotation updates. This masks certain classes of races. |
| 591 | + |
| 592 | + @method annotateBoxPosition |
| 593 | + @param {Object} box. |
| 594 | + @param {ms} window. |
| 595 | + */ |
| 596 | + annotateBoxPosition: function(box, window) { |
| 597 | + if (box.pending) { return; } |
| 598 | + window = window || 1000; |
| 599 | + |
| 600 | + // This can happen in some tests. |
| 601 | + this.get('env').update_annotations( |
| 602 | + box.id, 'service', {'gui-x': box.x, 'gui-y': box.y}, |
| 603 | + function() { |
| 604 | + if (window) { |
| 605 | + box.inDrag = views.DRAG_ENDING; |
| 606 | + Y.later(window, box, function() { |
| 607 | + // Provide (t) ms of protection from sending additional |
| 608 | + // annotations or applying them locally. |
| 609 | + box.inDrag = false; |
| 610 | + }); |
| 611 | + } else { |
| 612 | + box.inDrag = false; |
| 613 | + } |
| 614 | + }); |
| 615 | } |
| 616 | + |
| 617 | }, { |
| 618 | ATTRS: { |
| 619 | /** |
| 620 | @@ -238,6 +283,7 @@ |
| 621 | */ |
| 622 | views.DRAG_START = 1; |
| 623 | views.DRAG_ACTIVE = 2; |
| 624 | + views.DRAG_ENDING = 3; |
| 625 | }, '0.1.0', { |
| 626 | requires: [ |
| 627 | 'd3', |
| 628 | |
| 629 | === modified file 'app/views/utils.js' |
| 630 | --- app/views/utils.js 2013-10-11 20:13:50 +0000 |
| 631 | +++ app/views/utils.js 2013-10-15 20:38:14 +0000 |
| 632 | @@ -1738,7 +1738,7 @@ |
| 633 | |
| 634 | utils.deployBundleCallback = function(notifications, result) { |
| 635 | if (result.err) { |
| 636 | - console.log('import failed', result); |
| 637 | + console.log('import failed', result.err); |
| 638 | notifications.add({ |
| 639 | title: 'Deploy Bundle', |
| 640 | message: 'Environment deploy of the bundle failed.<br/>', |
| 641 | |
| 642 | === modified file 'test/test_conflict_ux.js' |
| 643 | --- test/test_conflict_ux.js 2013-09-18 13:04:27 +0000 |
| 644 | +++ test/test_conflict_ux.js 2013-10-15 20:38:14 +0000 |
| 645 | @@ -53,6 +53,7 @@ |
| 646 | db = new models.Database(); |
| 647 | conn = new utils.SocketStub(); |
| 648 | env = juju.newEnvironment({conn: conn}); |
| 649 | + env.update_annotations = function() {}; |
| 650 | inspector = setUpInspector(); |
| 651 | }); |
| 652 | |
| 653 | @@ -80,7 +81,7 @@ |
| 654 | ['unit', 'add', {id: 'mediawiki/0', agent_state: 'pending'}] |
| 655 | ]}}); |
| 656 | |
| 657 | - var fakeStore = new Y.juju.charmworld.APIv2({}); |
| 658 | + var fakeStore = new Y.juju.charmworld.APIv3({}); |
| 659 | fakeStore.iconpath = function() { |
| 660 | return 'charm icon url'; |
| 661 | }; |
| 662 | |
| 663 | === modified file 'test/test_environment_view.js' |
| 664 | --- test/test_environment_view.js 2013-10-07 19:38:23 +0000 |
| 665 | +++ test/test_environment_view.js 2013-10-15 20:38:14 +0000 |
| 666 | @@ -622,9 +622,9 @@ |
| 667 | match[2].should.eql('211.2'); |
| 668 | |
| 669 | // A positioned service will never be auto-positioned. |
| 670 | - view.topo.servicePointOutside = function() { |
| 671 | + /*view.topo.servicePointOutside = function() { |
| 672 | assert(false, 'We should never get here because annotations are set'); |
| 673 | - }; |
| 674 | + };*/ |
| 675 | tmp_data = { |
| 676 | op: 'delta', |
| 677 | result: [ |
| 678 | @@ -640,10 +640,12 @@ |
| 679 | db.onDelta({ data: tmp_data }); |
| 680 | view.update(); |
| 681 | |
| 682 | - view.topo.servicePointOutside = function() { |
| 683 | - assert(false, |
| 684 | - 'We should never get here because service was positioned'); |
| 685 | - }; |
| 686 | + /* |
| 687 | + *view.topo.servicePointOutside = function() { |
| 688 | + * assert(false, |
| 689 | + * 'We should never get here because service was positioned'); |
| 690 | + *}; |
| 691 | + */ |
| 692 | tmp_data = { |
| 693 | op: 'delta', |
| 694 | result: [ |
| 695 | @@ -656,7 +658,6 @@ |
| 696 | ]] |
| 697 | }; |
| 698 | db.onDelta({ data: tmp_data }); |
| 699 | - db.services.getById('wordpressb').set('hasBeenPositioned', true); |
| 700 | view.update(); |
| 701 | }); |
| 702 | |
| 703 | |
| 704 | === modified file 'test/test_fakebackend.js' |
| 705 | --- test/test_fakebackend.js 2013-10-10 05:52:08 +0000 |
| 706 | +++ test/test_fakebackend.js 2013-10-15 20:38:14 +0000 |
| 707 | @@ -162,7 +162,11 @@ |
| 708 | packageName: 'wordpress' |
| 709 | }; |
| 710 | |
| 711 | - assert.deepEqual(attrs, expectedAttrs); |
| 712 | + // Assert some key properties |
| 713 | + assert.equal(attrs.id, expectedAttrs.id); |
| 714 | + assert.equal(attrs.charm, expectedAttrs.charm); |
| 715 | + assert.deepEqual(attrs.config, expectedAttrs.config); |
| 716 | + assert.deepEqual(attrs.annotations, expectedAttrs.annotations); |
| 717 | var units = service.get('units').toArray(); |
| 718 | assert.lengthOf(units, 1); |
| 719 | assert.lengthOf(result.units, 1); |
| 720 | |
| 721 | === modified file 'test/test_inspector_constraints.js' |
| 722 | --- test/test_inspector_constraints.js 2013-09-18 13:04:27 +0000 |
| 723 | +++ test/test_inspector_constraints.js 2013-10-15 20:38:14 +0000 |
| 724 | @@ -91,6 +91,7 @@ |
| 725 | return 'charm icon url'; |
| 726 | }; |
| 727 | env = juju.newEnvironment({conn: conn}); |
| 728 | + env.update_annotations = function() {}; |
| 729 | env.connect(); |
| 730 | view = new views.environment({ |
| 731 | container: container, |
| 732 | |
| 733 | === modified file 'test/test_inspector_overview.js' |
| 734 | --- test/test_inspector_overview.js 2013-10-11 19:33:07 +0000 |
| 735 | +++ test/test_inspector_overview.js 2013-10-15 20:38:14 +0000 |
| 736 | @@ -47,6 +47,7 @@ |
| 737 | conn = new utils.SocketStub(); |
| 738 | db = new models.Database(); |
| 739 | env = juju.newEnvironment({conn: conn}); |
| 740 | + env.update_annotations = function() {}; |
| 741 | env.expose = function(s) { |
| 742 | exposeCalled = true; |
| 743 | service.set('exposed', true); |
| 744 | |
| 745 | === modified file 'test/test_inspector_settings.js' |
| 746 | --- test/test_inspector_settings.js 2013-09-27 23:41:44 +0000 |
| 747 | +++ test/test_inspector_settings.js 2013-10-15 20:38:14 +0000 |
| 748 | @@ -43,6 +43,7 @@ |
| 749 | conn = new utils.SocketStub(); |
| 750 | db = new models.Database(); |
| 751 | env = juju.newEnvironment({conn: conn}); |
| 752 | + env.update_annotations = function() {}; |
| 753 | }); |
| 754 | |
| 755 | afterEach(function(done) { |
| 756 | |
| 757 | === modified file 'test/test_model.js' |
| 758 | --- test/test_model.js 2013-10-10 17:06:41 +0000 |
| 759 | +++ test/test_model.js 2013-10-15 20:38:14 +0000 |
| 760 | @@ -18,7 +18,7 @@ |
| 761 | |
| 762 | 'use strict'; |
| 763 | |
| 764 | -describe('test_models.js', function() { |
| 765 | +describe('test_model.js', function() { |
| 766 | describe('Charm initialization', function() { |
| 767 | var models; |
| 768 | |
| 769 | @@ -797,13 +797,14 @@ |
| 770 | }); |
| 771 | |
| 772 | it('can export in deployer format', function() { |
| 773 | + // Mock a topology that can return positions. |
| 774 | db.services.add({id: 'mysql', charm: 'precise/mysql-1'}); |
| 775 | db.services.add({ |
| 776 | id: 'wordpress', |
| 777 | charm: 'precise/wordpress-1', |
| 778 | config: {debug: 'no', username: 'admin'}, |
| 779 | constraints: 'cpu-power=2,cpu-cores=4', |
| 780 | - annotations: {'gui-x': 100, 'gui-y': 200, 'ignored': true} |
| 781 | + annotations: {'gui-x': 100, 'gui-y': 200} |
| 782 | }); |
| 783 | db.relations.add({ |
| 784 | id: 'relation-0', |
| 785 | |
| 786 | === modified file 'test/test_service_module.js' |
| 787 | --- test/test_service_module.js 2013-10-01 18:33:29 +0000 |
| 788 | +++ test/test_service_module.js 2013-10-15 20:38:14 +0000 |
| 789 | @@ -73,7 +73,7 @@ |
| 790 | function() { |
| 791 | var d = |
| 792 | { id: 'wordpress', |
| 793 | - inDrag: true, |
| 794 | + inDrag: views.DRAG_ACTIVE, |
| 795 | x: 100.1, |
| 796 | y: 200.2}; |
| 797 | serviceModule.dragend(d, serviceModule); |
Reviewers: mp+191119_
Message:
Please take a look.
Description:
Redo Service Placement
This changes a number of aspects of how service placement is handled.
There
may still be some edge cases and QA will need to be extensive as this
has moved through phases where various parts worked and didn't. I
believe
this version to be good however.
Position annotations remain on service models now rather than being
deleted
when applied. We favor this to having vars such as hasBeenPositioned,
positionedFromGhost and service model x/y (which mixed into
BoundingBoxes).
These various complications are mostly gone and the handling of updating
position annotations falls to a single method on the topology. (Though
to be
fair that is still called from more than one place).
Further simplification might be possible by unifying the node creation
and
node update paths wrt annotations. This didn't however fit in the time
provided.
This also includes a basic fix for always pulling positions from the
client
during an export.
https:/
(do not edit description out of merge proposal)
Please review this at https:/
Affected files (+169, -185 lines):
A [revision details]
M app/app.js
M app/models/
M app/views/
M app/views/
M app/views/
M app/views/
M test/test_
M test/test_
M test/test_model.js
M test/test_