change charm store data structures

change charm store data structures

This change is hopefully the last round of changes, at least for a long while, to the underlying charm store infrastructure.  It is more deletes than additions, and changes the code to take advantage of the changes Kapil made to the charm store.

The sorting code is simplified yet again.

https://code.launchpad.net/~gary/juju-gui/simplifycharmstore/+merge/131086

(do not edit description out of merge proposal)

[gary.poster, 2012-10-23 20:09:18]

Please take a look.

[benjamin.saller, 2012-10-26 07:56:19]

Even though this is a large branch I think it removes more code than it adds
which is always a plus. I had some minor feedback but this LGTM. 

I'd still rather Kapil get a chance to look this over but I think the general
architectural issues that were present before (and which I didn't take into
account either) are no longer present.

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js#newcode56
app/models/charm.js:56: this.on('load', function() { this.loaded = true; });
Don't you need to either use self here or pass this as context to the 'on' call?

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js#newcode84
app/models/charm.js:84: options.get_charm(
Why the two naming styles on these_twoMethods? get_charm, loadByPath?

https://codereview.appspot.com/6733067/diff/1/app/templates/charm-search-resu...
File app/templates/charm-search-result.handlebars (right):

https://codereview.appspot.com/6733067/diff/1/app/templates/charm-search-resu...
app/templates/charm-search-result.handlebars:13: {{#if
owner}}{{owner}}/{{/if}}{{package_name}}</a>
I thought you didn't need the #if when there is no content other than the var
which can default to null, no?

[gary.poster, 2012-10-26 12:11:21]

*** Submitted:

change charm store data structures

This change is hopefully the last round of changes, at least for a long while,
to the underlying charm store infrastructure.  It is more deletes than
additions, and changes the code to take advantage of the changes Kapil made to
the charm store.

The sorting code is simplified yet again.

R=benjamin.saller
CC=
https://codereview.appspot.com/6733067

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js#newcode56
app/models/charm.js:56: this.on('load', function() { this.loaded = true; });
On 2012/10/26 07:56:19, benjamin.saller wrote:
> Don't you need to either use self here or pass this as context to the 'on'
call?
No, because the listener is on "this" so the context is what I want.  I already
have a test that verifies, and I doublechecked in the chromium debugger as well.

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js#newcode84
app/models/charm.js:84: options.get_charm(
On 2012/10/26 07:56:19, benjamin.saller wrote:
> Why the two naming styles on these_twoMethods? get_charm, loadByPath?
As we discussed, it's because we haven't standardized one way or the other
across all our files.  Within a file, we are consistent.  This uses get_charm,
from the older env js, and loadByPath, from the newer charm store js.  I got
your agreement in person that this is fine.
OTOH I switched charmIDRe and idElements in this file, based on your reminder.

https://codereview.appspot.com/6733067/diff/1/app/templates/charm-search-resu...
File app/templates/charm-search-result.handlebars (right):

https://codereview.appspot.com/6733067/diff/1/app/templates/charm-search-resu...
app/templates/charm-search-result.handlebars:13: {{#if
owner}}{{owner}}/{{/if}}{{package_name}}</a>
On 2012/10/26 07:56:19, benjamin.saller wrote:
> I thought you didn't need the #if when there is no content other than the var
> which can default to null, no?
Yes, but I have the slash after the owner.

[gary.poster, 2012-10-26 12:14:35]

Thank you for the review, Ben.  I have comments in-line, largely in line with
what we discussed in person.

You gave me permission to go ahead and land this.  Kapil mentioned that he
questioned the use of the Model load/sync code given that it is of minimal value
to our use case, but if I need to address this I will do it in a subsequent
branch.

Thanks

Gary