Code review - Issue 6856067: Fix initial creation of subordinates.https://codereview.appspot.com/2012-11-21T17:35:56+00:00rietveld
Message from unknown
2012-11-20T00:13:35+00:00matthew.scotturn:md5:f590024c9d6b692b8870d2979d3e3e2b
Message from matthew.scott@canonical.com
2012-11-20T00:13:38+00:00matthew.scotturn:md5:0b0d100c5a2e5e0305d25c392d338fd3
Please take a look.
Message from benjamin.saller@canonical.com
2012-11-20T13:31:33+00:00benjamin.sallerurn:md5:a144f3c7f9e98d9b5c37bcd55a5a63b8
The diff looks fine, but there is behavior we expect on subordinates that isn't be tested, I think we can at minimum check to see if subs get x/y position assignment? What do you think?
Message from bac@canonical.com
2012-11-20T14:50:48+00:00bacurn:md5:a5092ce547e33c96fee72ec43970c9ae
Looks good Matt. Thanks.
https://codereview.appspot.com/6856067/diff/1/app/views/environment.js
File app/views/environment.js (right):
https://codereview.appspot.com/6856067/diff/1/app/views/environment.js#newcode323
app/views/environment.js:323: })
maybe this would be more readable:
min(1, d.unit_count)
Message from matthew.scott@canonical.com
2012-11-20T15:59:08+00:00matthew.scotturn:md5:9a4566187008f48edde4efcd402debca
A test was added as well, but rather than just testing whether or not subordinates had been placed properly, it tests that all services have been placed properly.
https://codereview.appspot.com/6856067/diff/1/app/views/environment.js
File app/views/environment.js (right):
https://codereview.appspot.com/6856067/diff/1/app/views/environment.js#newcode323
app/views/environment.js:323: })
On 2012/11/20 14:50:48, bac wrote:
> maybe this would be more readable:
>
> min(1, d.unit_count)
Thanks! Did Math.max(d.unit_count, 1); (since we want 1 or higher)
Message from unknown
2012-11-20T16:26:41+00:00matthew.scotturn:md5:847ebebf4fbc1ed77f76e606005c0209
Message from matthew.scott@canonical.com
2012-11-20T16:26:44+00:00matthew.scotturn:md5:ca6e382d83768a3a99acd5bd1223ed9b
Please take a look.
Message from bac@canonical.com
2012-11-21T17:27:11+00:00bacurn:md5:85ff2e9b51ba6b909f17ce2f21277b86
The new test makes sense to me Matt.
Message from unknown
2012-11-21T17:33:21+00:00matthew.scotturn:md5:b6b622f267d28ceb60c3b40942785efc
Message from matthew.scott@canonical.com
2012-11-21T17:35:56+00:00matthew.scotturn:md5:3fd0ba67fadc967101c6f6cbe843417c
*** Submitted:
Fix initial creation of subordinates.
When creating a subordinate service initially, it was not being laid out properly by the pack layout, nor was it seeming to have drag events attached. Pack layout was expecting a value accessor function to return some number greater than zero when constructing its radius, which informs the x/y coordinates. Returning zero results in NaN for all three values, thus positioning the service at (0,0) and causing drag events to fail when updating the service's coordinates with dx/dy + NaN. This branch fixes this by ensuring that zero is never returned as the node's value. This fix is quick and intended to make the UI function with the current layout algorithm. Future layout algorithms, if they're designed by hand, will need to keep this in mind as well.
R=benjamin.saller, bac
CC=
https://codereview.appspot.com/6856067