Send settings changes to environment

Send settings changes to environment

Sends settings changes that the user makes to the
environment. This branch also includes some styling
changes and a driveby fix to the viewlet instantiation
code.

https://code.launchpad.net/~hatch/juju-gui/config-settings-hookup/+merge/174030

(do not edit description out of merge proposal)

[jeff.pihach, 2013-07-10 19:28:29]

Please take a look.

[benji, 2013-07-10 19:51:26]

LGTM, I just had a question or two and some suggestions.

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

https://codereview.appspot.com/10872045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:561: @param {String} viewletName viewlet name to clear
the changed values.
Shouldn't "viewlet" be capitalized?

https://codereview.appspot.com/10872045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:565: //var viewlet = this.getViewlet(viewletName);
Accidentally left commented-out code.

https://codereview.appspot.com/10872045/diff/1/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/10872045/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:61: 
I am confused by this code.  The old charm panel code expected the charm config
schema to be in charm.set('options', xxx) but my understanding was that the new
config bits expect it to be in charm.set('config', {options: xxx}).

I used to think it was just because we were transitioning [for some unknown
reason] from having the schema in one place to the other, but now it feels like
there is some duplication going on here.

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

https://codereview.appspot.com/10872045/diff/1/app/views/service.js#newcode1272
app/views/service.js:1272: @method saveConfig
It would be nice to have an @return so we don't have to guess as to whether or
not it was intentionally omitted because there is no return or accidentally
forgotten.

https://codereview.appspot.com/10872045/diff/1/app/views/service.js#newcode1299
app/views/service.js:1299: // we have no error handling story yet
What about notifications?  Wouldn't they work for this?

https://codereview.appspot.com/10872045/diff/1/app/views/service.js#newcode1309
app/views/service.js:1309: @param {Y.EventFacade} e yui event object.
@return, as above.

https://codereview.appspot.com/10872045/diff/1/app/views/service.js#newcode1328
app/views/service.js:1328: // we have no story for this yet
I'm confused.  The comment says that we should show a notification but we don't
have a mechanism, then we show a notification with our existing mechanism.

https://codereview.appspot.com/10872045/diff/1/app/views/view-container.js
File app/views/view-container.js (right):

https://codereview.appspot.com/10872045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:144: @param {Array} dirtyFields an array of keys
representing changed fields.
@return

https://codereview.appspot.com/10872045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:146: unsyncedFields: function(dirtyFields) {},
@return

https://codereview.appspot.com/10872045/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:152: @method syncedFields
@return

https://codereview.appspot.com/10872045/diff/1/lib/views/juju-inspector.less
File lib/views/juju-inspector.less (right):

https://codereview.appspot.com/10872045/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:55: /* below hyphen code was taken from:
Using the word "hyphenation" might communicate more clearly.

[jeff.pihach, 2013-07-10 20:45:40]

Please take a look.

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

https://codereview.appspot.com/10872045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:561: @param {String} viewletName viewlet name to clear
the changed values.
Nope, it's a string so it's not instantiable. Only instantiable classes get the
first letter capitalized.

https://codereview.appspot.com/10872045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:565: //var viewlet = this.getViewlet(viewletName);
Thanks, gone

https://codereview.appspot.com/10872045/diff/1/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/10872045/diff/1/app/views/ghost-inspector.js#n...
app/views/ghost-inspector.js:61: 
Both areas require this change because it uses the same util method to parse the
config params for the templates. I would assume that when/if we completely
transition this will need to be revisited.

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

https://codereview.appspot.com/10872045/diff/1/app/views/service.js#newcode1299
app/views/service.js:1299: // we have no error handling story yet
Yes I will add a notification here - see the comment below for the reasoning
behind the confusing comments.

https://codereview.appspot.com/10872045/diff/1/app/views/service.js#newcode1328
app/views/service.js:1328: // we have no story for this yet
Sorry that was confusing, this basically means that we don't have a new story
for these notifications with the new UI.

https://codereview.appspot.com/10872045/diff/1/lib/views/juju-inspector.less
File lib/views/juju-inspector.less (right):

https://codereview.appspot.com/10872045/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:55: /* below hyphen code was taken from:
On 2013/07/10 19:51:26, benji wrote:
> Using the word "hyphenation" might communicate more clearly.

Done.

[benji, 2013-07-10 20:50:48]

Looks good.  I had one clarification.

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

https://codereview.appspot.com/10872045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:561: @param {String} viewletName viewlet name to clear
the changed values.
On 2013/07/10 20:45:40, jeff.pihach wrote:
> Nope, it's a string so it's not instantiable. Only instantiable classes get
the
> first letter capitalized. 

I didn't mean "viewletName", I meant the first word of the sentence, "viewlet".

[gary.poster, 2013-07-10 22:14:46]

LGTM.  Would like to QA but need to run.  Please make sure it is QAd. :-)

https://codereview.appspot.com/10872045/diff/10001/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/10872045/diff/10001/app/views/databinding.js#n...
app/views/databinding.js:563: BindingEngine.prototype.clearChangedValues =
function(viewletName) {
Ah-ha!  Cool.

https://codereview.appspot.com/10872045/diff/10001/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/10872045/diff/10001/app/views/ghost-inspector....
app/views/ghost-inspector.js:59: charm.set('options',
charm.get('config').options);
I think this is a new/old charm model thing.  A comment as to the cause would be
great.  A bug as to the proper fix would also be great. :-)

https://codereview.appspot.com/10872045/diff/10001/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/10872045/diff/10001/app/views/service.js#newco...
app/views/service.js:1301: message: 'Error saving service config',
Can we put the full details here?

https://codereview.appspot.com/10872045/diff/10001/app/views/service.js#newco...
app/views/service.js:1335: // we have no story for this yet
Please raise these issues (success and error notification) on email to Luca

[jeff.pihach, 2013-07-10 22:26:34]

Thanks for the reviews!

https://codereview.appspot.com/10872045/diff/10001/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/10872045/diff/10001/app/views/databinding.js#n...
app/views/databinding.js:563: BindingEngine.prototype.clearChangedValues =
function(viewletName) {
:-) thanks

https://codereview.appspot.com/10872045/diff/10001/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/10872045/diff/10001/app/views/ghost-inspector....
app/views/ghost-inspector.js:59: charm.set('options',
charm.get('config').options);
I'll add a comment here

I don't think we have decided a course of action yet regarding the various charm
models but I have created a generic bug report below - feel free to elaborate on
the ticket if you like.

https://bugs.launchpad.net/juju-gui/+bug/1200008

https://codereview.appspot.com/10872045/diff/10001/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/10872045/diff/10001/app/views/service.js#newco...
app/views/service.js:1301: message: 'Error saving service config',
Yes I can add the error description here but I still think we should log out the
actual error object as it will have a lot more details.

https://codereview.appspot.com/10872045/diff/10001/app/views/service.js#newco...
app/views/service.js:1335: // we have no story for this yet
On 2013/07/10 22:14:46, gary.poster wrote:
> Please raise these issues (success and error notification) on email to Luca

Done.

[jeff.pihach, 2013-07-10 22:33:51]

*** Submitted:

Send settings changes to environment

Sends settings changes that the user makes to the
environment. This branch also includes some styling
changes and a driveby fix to the viewlet instantiation
code.

R=benji, gary.poster
CC=
https://codereview.appspot.com/10872045