Make charms able to be dragged to the environment.

Make charms able to be dragged to the environment.



https://code.launchpad.net/~benji/juju-gui/charm-dnd-2/+merge/171084

(do not edit description out of merge proposal)

[benji, 2013-06-24 13:45:57]

Please take a look.

[benji, 2013-06-24 13:50:12]

On 2013/06/24 13:45:57, benji wrote:
> Please take a look.

But only once I have fixed a couple of small things. ;)

[benji, 2013-06-24 21:36:13]

Please take a look.

[jeff.pihach, 2013-06-24 22:35:08]

This is such a cool feature thanks for implementing it. I added a comment about
deploying to the position that the user dropped as a follow-up branch as it's
probably a bit too much work to tack onto this one :-)

My comments below are mostly just cleanup and few small improvement requests. If
you have any questions don't hesitate to ask!

https://codereview.appspot.com/10500044/diff/5001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/10500044/diff/5001/app/app.js#newcode582
app/app.js:582: Y.on('initiateDeploy', function(charm) {
as per comment in topology/service.js this should be:

this.views.environment.instance.topo.on('initiateDeploy', function() { ... });

Environment is a bubble target for the topology so it might be preferable to do 

this.env.on('initiateDeploy', ... 
-or-
this.env.on('Topology:initiateDeploy', ... 

I'd even entertain the idea of renaming the event to be 'deployFromCharm'

https://codereview.appspot.com/10500044/diff/5001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/10500044/diff/5001/app/views/topology/service....
app/views/topology/service.js:387: canvasDropHandler: function() {
- possible followup branch

This should take note of the drop position and place the service there instead
of the default deploy location.

https://codereview.appspot.com/10500044/diff/5001/app/views/topology/service....
app/views/topology/service.js:388: // XXX Why do we have to dig so deep just to
get to the event?  And why
Typically libraries will wrap native objects with their own convenience layer
and then provide access to the original one on a private property. As for why
it's not passed to the handler....not sure Makyo might know.

https://codereview.appspot.com/10500044/diff/5001/app/views/topology/service....
app/views/topology/service.js:395: Y.fire('initiateDeploy', charm);
This should not be firing on Y, but firing from this class so that the app can
listen on this class for the event.

this.fire('initiateDeploy', charm);

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js
File app/widgets/charm-token.js (right):

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:112: if (!container) {
should no longer be necessary as per my comments in the test script.

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:117: this._makeDraggable(container, container,
charmData);
this._makeDraggable(container.one('a'), container, charmData) will get you to a
visibile UI while dragging without attaching to every child element.

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:119: container.all('*').each(function(child) {
This can be removed as per comment above

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:133: var outerContainer =
container.ancestor('.yui3-charmtoken')
this is the bounding box this.get('boundingBox').addClass('yui3-u');

Although I'm not sure why you need to add this class? It appears to work fine
without.

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
File test/test_charm_token_drag_and_drop.js (right):

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
test/test_charm_token_drag_and_drop.js:37: outerContainer =
Y.namespace('juju-tests.utils').makeContainer()
not necessary, the bounding box will be automatically created when the widget is
rendered.

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
test/test_charm_token_drag_and_drop.js:39: container =
Y.namespace('juju-tests.utils').makeContainer();
you can just use Y['juju-tests'].utils.makeContainer()

Y.juju-tests isn't available the tests will blow up anyways.

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
test/test_charm_token_drag_and_drop.js:44: outerContainer.remove(true);
this can be removed as per above and exchanged for 

container.remove().destroy(true);

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
test/test_charm_token_drag_and_drop.js:44: outerContainer.remove(true);
The charm token instances should be destroyed and removed at the end of the
tests

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
test/test_charm_token_drag_and_drop.js:58: token._makeDraggable =
function(element, dragImage, charmData) {
Should not be required if you call render() as mentioned below.

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
test/test_charm_token_drag_and_drop.js:61: token.renderUI(container);
you render widgets by calling render() on them, renderUI() is only part of the
render cycle.

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
test/test_charm_token_drag_and_drop.js:71: it('can make an element draggable',
function() {
I'm not sure what this is testing - you aren't going to be using these as
utility methods so the previous test that something is draggable should suffice.

https://codereview.appspot.com/10500044/diff/5001/test/test_notifier_widget.js
File test/test_notifier_widget.js (right):

https://codereview.appspot.com/10500044/diff/5001/test/test_notifier_widget.j...
test/test_notifier_widget.js:92: // A timeout of 250 milliseconds is used so
that we ensure the destroying
500ms

https://codereview.appspot.com/10500044/diff/5001/test/test_notifier_widget.j...
test/test_notifier_widget.js:103: // A timeout of 250 milliseconds is used so
that we ensure the destroying
500ms

https://codereview.appspot.com/10500044/diff/5001/test/test_notifier_widget.j...
test/test_notifier_widget.js:114: // A timeout of 250 milliseconds is used so
that we ensure the node is not
500ms

https://codereview.appspot.com/10500044/diff/5001/test/test_service_module.js
File test/test_service_module.js (right):

https://codereview.appspot.com/10500044/diff/5001/test/test_service_module.js...
test/test_service_module.js:372: var eventHandle = Y.on('initiateDeploy',
function(charm) {
see previous comment about firing from the topology

[rharding, 2013-06-25 11:36:52]

Comments below. Jeff has some great feedback and I agree that there's some work
around the container/boundingBox that'll make things a bit easier.

https://codereview.appspot.com/10500044/diff/1/app/widgets/charm-container.js
File app/widgets/charm-container.js (right):

https://codereview.appspot.com/10500044/diff/1/app/widgets/charm-container.js...
app/widgets/charm-container.js:191: _getDropZone: function() {
please make a valueFn so that it's done once and stored. It won't be changing.

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js
File app/widgets/charm-token.js (right):

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:100: element.on('dragstart',
Should we be catching this event to clean up later? This class is using the
Y.Event.EventTracker and you can do a this.addEvent(element.on('dragstart...

Then it'll auto get removed and cleaned up on the widgets destroy.

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:129: renderUI: function(container) {
I'm not sure why we need the container here. If you pass the boundingBox into
the widget cfg on init you'll have the correct space for the content box and
such.

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:135: this._addDraggability(outerContainer);
This should only be run if the charm-token is instructed to be draggable. This
widget is shared among fullscreen, sidebar, and the related charms side data
uses. It doesn't make sense to have them draggable in all cases. An ATTR for
isDraggable should be added and then set from the sidebar related views I'd
think.

[benji, 2013-06-25 20:56:35]

Please take a look.

https://codereview.appspot.com/10500044/diff/5001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/10500044/diff/5001/app/app.js#newcode582
app/app.js:582: Y.on('initiateDeploy', function(charm) {
On 2013/06/24 22:35:08, jeff.pihach wrote:
> as per comment in topology/service.js this should be:
> 
> this.views.environment.instance.topo.on('initiateDeploy', function() { ... });
> 
> Environment is a bubble target for the topology so it might be preferable to
do 
> 
> this.env.on('initiateDeploy', ... 
> -or-
> this.env.on('Topology:initiateDeploy', ... 
> 
> I'd even entertain the idea of renaming the event to be 'deployFromCharm'

After extensive investigation and a call with Jeff we determined that the only
workable solution was the original code, so this is staying as-is.

https://codereview.appspot.com/10500044/diff/5001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/10500044/diff/5001/app/views/topology/service....
app/views/topology/service.js:387: canvasDropHandler: function() {
On 2013/06/24 22:35:08, jeff.pihach wrote:
> - possible followup branch
> 
> This should take note of the drop position and place the service there instead
> of the default deploy location.

+1

https://codereview.appspot.com/10500044/diff/5001/app/views/topology/service....
app/views/topology/service.js:395: Y.fire('initiateDeploy', charm);
On 2013/06/24 22:35:08, jeff.pihach wrote:
> This should not be firing on Y, but firing from this class so that the app can
> listen on this class for the event.
> 
> this.fire('initiateDeploy', charm);

See note on subscribing end.

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js
File app/widgets/charm-token.js (right):

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:100: element.on('dragstart',
On 2013/06/25 11:36:53, rharding wrote:
> Should we be catching this event to clean up later? This class is using the
> Y.Event.EventTracker and you can do a this.addEvent(element.on('dragstart...
> 
> Then it'll auto get removed and cleaned up on the widgets destroy.

Done.

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:117: this._makeDraggable(container, container,
charmData);
On 2013/06/24 22:35:08, jeff.pihach wrote:
> this._makeDraggable(container.one('a'), container, charmData) will get you to
a
> visibile UI while dragging without attaching to every child element.

Not quite.  The charm token is not draggable, and the draggable area is quite a
bit smaller; too small in fact.

I can get the original result if I use the original code with the above in place
of the recursive draggifying.  I'm not real happy with that though because it
means that if we change the structure of the charm tokens we could easily break
the dragging.  I've decided the best approach is to leave the code as-is.

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:119: container.all('*').each(function(child) {
On 2013/06/24 22:35:08, jeff.pihach wrote:
> This can be removed as per comment above

See above.

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:129: renderUI: function(container) {
On 2013/06/25 11:36:53, rharding wrote:
> I'm not sure why we need the container here. If you pass the boundingBox into
> the widget cfg on init you'll have the correct space for the content box and
> such.

This is to facilitate testing (because .set('contentBox', xxx) did not actually
change the value.  I have added a comment to that effect but I would really like
to know why the .set() didn't work.

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:133: var outerContainer =
container.ancestor('.yui3-charmtoken')
On 2013/06/24 22:35:08, jeff.pihach wrote:
> this is the bounding box this.get('boundingBox').addClass('yui3-u');
> 
> Although I'm not sure why you need to add this class? It appears to work fine
> without.

That was in the original code.

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:135: this._addDraggability(outerContainer);
On 2013/06/25 11:36:53, rharding wrote:
> This should only be run if the charm-token is instructed to be draggable. This
> widget is shared among fullscreen, sidebar, and the related charms side data
> uses. It doesn't make sense to have them draggable in all cases. An ATTR for
> isDraggable should be added and then set from the sidebar related views I'd
> think.

Indeed (an earlier version of the branch did that correctly).  I have added that
functionality.

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
File test/test_charm_token_drag_and_drop.js (right):

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
test/test_charm_token_drag_and_drop.js:37: outerContainer =
Y.namespace('juju-tests.utils').makeContainer()
On 2013/06/24 22:35:08, jeff.pihach wrote:
> not necessary, the bounding box will be automatically created when the widget
is
> rendered.

The pre-existing charm token code looks up an ancestor with class
"yui3-charmtoken" so this is required.

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
test/test_charm_token_drag_and_drop.js:39: container =
Y.namespace('juju-tests.utils').makeContainer();
On 2013/06/24 22:35:08, jeff.pihach wrote:
> you can just use Y['juju-tests'].utils.makeContainer()
> 
> Y.juju-tests isn't available the tests will blow up anyways.

I think I'll keep this as-is.  TOOWTDI and all that.

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
test/test_charm_token_drag_and_drop.js:44: outerContainer.remove(true);
On 2013/06/24 22:35:08, jeff.pihach wrote:
> The charm token instances should be destroyed and removed at the end of the
> tests

Done.

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
test/test_charm_token_drag_and_drop.js:58: token._makeDraggable =
function(element, dragImage, charmData) {
On 2013/06/24 22:35:08, jeff.pihach wrote:
> Should not be required if you call render() as mentioned below.

N/A

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
test/test_charm_token_drag_and_drop.js:61: token.renderUI(container);
True.  It's also the part of the render cycle I'm interested in.

https://codereview.appspot.com/10500044/diff/5001/test/test_charm_token_drag_...
test/test_charm_token_drag_and_drop.js:71: it('can make an element draggable',
function() {
On 2013/06/24 22:35:08, jeff.pihach wrote:
> I'm not sure what this is testing - you aren't going to be using these as
> utility methods so the previous test that something is draggable should
suffice.

These are minimal, white box tests that ensure the functions perform the desired
actions.

https://codereview.appspot.com/10500044/diff/5001/test/test_notifier_widget.js
File test/test_notifier_widget.js (right):

https://codereview.appspot.com/10500044/diff/5001/test/test_notifier_widget.j...
test/test_notifier_widget.js:92: // A timeout of 250 milliseconds is used so
that we ensure the destroying
On 2013/06/24 22:35:08, jeff.pihach wrote:
> 500ms

I changed these to omit the specific value.

https://codereview.appspot.com/10500044/diff/5001/test/test_notifier_widget.j...
test/test_notifier_widget.js:103: // A timeout of 250 milliseconds is used so
that we ensure the destroying
On 2013/06/24 22:35:08, jeff.pihach wrote:
> 500ms

Done.

https://codereview.appspot.com/10500044/diff/5001/test/test_notifier_widget.j...
test/test_notifier_widget.js:114: // A timeout of 250 milliseconds is used so
that we ensure the node is not
On 2013/06/24 22:35:08, jeff.pihach wrote:
> 500ms

Done.

https://codereview.appspot.com/10500044/diff/5001/test/test_service_module.js
File test/test_service_module.js (right):

https://codereview.appspot.com/10500044/diff/5001/test/test_service_module.js...
test/test_service_module.js:372: var eventHandle = Y.on('initiateDeploy',
function(charm) {
On 2013/06/24 22:35:08, jeff.pihach wrote:
> see previous comment about firing from the topology

Done.

[rharding, 2013-06-25 21:15:09]

LGTM with the discussion on using boundingBox via cfg vs the container and the
simpler remove below.

https://codereview.appspot.com/10500044/diff/18001/test/test_charm_token_drag...
File test/test_charm_token_drag_and_drop.js (right):

https://codereview.appspot.com/10500044/diff/18001/test/test_charm_token_drag...
test/test_charm_token_drag_and_drop.js:44:
outerContainer.remove().destroy(true);
you can just remove(true) and get it destroyed fully.

[benjamin.saller, 2013-06-25 21:29:18]

Thanks for this, going to be a really nice feature when it lands. I think you
need to merge trunk and maybe look at this again, the ghost handling and the
inspector stuff changes and possibly simplifies some of this.

https://codereview.appspot.com/10500044/diff/18001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/10500044/diff/18001/app/app.js#newcode584
app/app.js:584: }, this.charmPanel);
We can't invoke the function above in cfg.deploy? we already switch the impl
based on the feature flag.

https://codereview.appspot.com/10500044/diff/18001/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/10500044/diff/18001/app/store/env/fakebackend....
app/store/env/fakebackend.js:1232: var importImpl;
This looks like a story for another day.. Not sure what you must have hit here
to need this. :)

https://codereview.appspot.com/10500044/diff/18001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/10500044/diff/18001/app/views/topology/service...
app/views/topology/service.js:390: // is it not just passed into the handler?
I blame YUI :)

https://codereview.appspot.com/10500044/diff/18001/app/views/topology/service...
app/views/topology/service.js:394: var charmData =
Y.JSON.parse(evt.dataTransfer.getData('charmData'));
Why JSON parse at all if its a single string?

https://codereview.appspot.com/10500044/diff/18001/app/views/topology/service...
app/views/topology/service.js:396: Y.fire('initiateDeploy', charm);
If you merge trunk we also have db.services.ghostDeploy(charm) which creates the
ghost model from the charm. The rest of the system has been changed to react to
that, render the proper model, set up the inspector, etc.

https://codereview.appspot.com/10500044/diff/18001/app/widgets/charm-token.js
File app/widgets/charm-token.js (right):

https://codereview.appspot.com/10500044/diff/18001/app/widgets/charm-token.js...
app/widgets/charm-token.js:84: evt.dataTransfer.setDragImage(dragImage._node, 0,
0);
cute with the image

https://codereview.appspot.com/10500044/diff/18001/app/widgets/charm-token.js...
app/widgets/charm-token.js:112: var charmData =
Y.JSON.stringify(this.charmAttributes);
You only need the id here, no? If thats true then no need for JSON

https://codereview.appspot.com/10500044/diff/18001/test/test_notifier_widget.js
File test/test_notifier_widget.js (right):

https://codereview.appspot.com/10500044/diff/18001/test/test_notifier_widget....
test/test_notifier_widget.js:97: }, 500);
That must have been a hard one to track down.

[benji, 2013-06-26 13:43:53]

Thanks guys.  I think I have responded to all the feedback.

I need to merge from trunk and then this will be ready to land.

https://codereview.appspot.com/10500044/diff/1/app/widgets/charm-container.js
File app/widgets/charm-container.js (right):

https://codereview.appspot.com/10500044/diff/1/app/widgets/charm-container.js...
app/widgets/charm-container.js:191: _getDropZone: function() {
On 2013/06/25 11:36:52, rharding wrote:
> please make a valueFn so that it's done once and stored. It won't be changing.

This code has changed now, so this is gone.

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js
File app/widgets/charm-token.js (right):

https://codereview.appspot.com/10500044/diff/5001/app/widgets/charm-token.js#...
app/widgets/charm-token.js:129: renderUI: function(container) {
On 2013/06/25 20:56:36, benji wrote:
> On 2013/06/25 11:36:53, rharding wrote:
> > I'm not sure why we need the container here. If you pass the boundingBox
into
> > the widget cfg on init you'll have the correct space for the content box and
> > such.
> 
> This is to facilitate testing (because .set('contentBox', xxx) did not
actually
> change the value.  I have added a comment to that effect but I would really
like
> to know why the .set() didn't work.

I now know that set didn't work because this is a write-once attribute and YUI
has a policy that setting a write-once attribute will be silently ignored.

https://codereview.appspot.com/10500044/diff/18001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/10500044/diff/18001/app/app.js#newcode584
app/app.js:584: }, this.charmPanel);
On 2013/06/25 21:29:18, benjamin.saller wrote:
> We can't invoke the function above in cfg.deploy? we already switch the impl
> based on the feature flag. 

My reading of the question: "Why is it that this branch doesn't use the same
approach as the above code which stashes the deploy function in sub-app
configuration and uses that?"  Please correct me if my reading is wrong.

That approach is what I originally intended, but in review I was informed that
the views and widgets have been structured with the intent that they have
certain responsibilities and references and using that approach would violate
those.

https://codereview.appspot.com/10500044/diff/18001/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/10500044/diff/18001/app/store/env/fakebackend....
app/store/env/fakebackend.js:1232: var importImpl;
On 2013/06/25 21:29:18, benjamin.saller wrote:
> This looks like a story for another day.. Not sure what you must have hit here
> to need this. :)

It is not strictly needed for this branch, but it came up during development and
it seemed like a good thing to do: transform a generic error into one that has
more context.  I don't feel strongly about keeping it.

https://codereview.appspot.com/10500044/diff/18001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/10500044/diff/18001/app/views/topology/service...
app/views/topology/service.js:390: // is it not just passed into the handler?
On 2013/06/25 21:29:18, benjamin.saller wrote:
> I blame YUI :)

Me too.  And d3. ;)

https://codereview.appspot.com/10500044/diff/18001/app/views/topology/service...
app/views/topology/service.js:394: var charmData =
Y.JSON.parse(evt.dataTransfer.getData('charmData'));
On 2013/06/25 21:29:18, benjamin.saller wrote:
> Why JSON parse at all if its a single string?

The value is a large JSON encoding of the charm data.  The reason we have to
encode/decode it is that the dataTransfer mechanism only allows string values.

I have added comments at the encode and decode points to that effect.

https://codereview.appspot.com/10500044/diff/18001/app/widgets/charm-token.js
File app/widgets/charm-token.js (right):

https://codereview.appspot.com/10500044/diff/18001/app/widgets/charm-token.js...
app/widgets/charm-token.js:112: var charmData =
Y.JSON.stringify(this.charmAttributes);
On 2013/06/25 21:29:18, benjamin.saller wrote:
> You only need the id here, no? If thats true then no need for JSON

That was the way the code was originally structured, but because of the layered
design (referenced in another comment) I had to change it to smuggle the entire
charm data through the event.  I'm not entirely happy with the result either.

https://codereview.appspot.com/10500044/diff/18001/test/test_charm_token_drag...
File test/test_charm_token_drag_and_drop.js (right):

https://codereview.appspot.com/10500044/diff/18001/test/test_charm_token_drag...
test/test_charm_token_drag_and_drop.js:44:
outerContainer.remove().destroy(true);
On 2013/06/25 21:15:09, rharding wrote:
> you can just remove(true) and get it destroyed fully.

That's what I had originally, but Jeff said this was superior. After puzzling
over the docs it seems that this form destroys a bit harder than the other
because outerContainer.remove(true) does not pass true through to .destroy().

https://codereview.appspot.com/10500044/diff/18001/test/test_notifier_widget.js
File test/test_notifier_widget.js (right):

https://codereview.appspot.com/10500044/diff/18001/test/test_notifier_widget....
test/test_notifier_widget.js:97: }, 500);
On 2013/06/25 21:29:18, benjamin.saller wrote:
> That must have been a hard one to track down.

:)

[benji, 2013-06-26 13:52:49]

Please take a look.

[benji, 2013-06-26 14:27:35]

*** Submitted:

Make charms able to be dragged to the environment.

R=jeff.pihach, rharding, benjamin.saller
CC=
https://codereview.appspot.com/10500044