<div dir="ltr">Hi James,<br><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 22, 2014 at 7:24 AM, James Cameron <span dir="ltr"><<a href="mailto:quozl@laptop.org" target="_blank">quozl@laptop.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">On Sun, Dec 21, 2014 at 08:36:00AM +1100, Sam P. wrote:<br>
> I have submitted a new patch which work both inside sugar and in<br>
> other environments.<br>
><br>
</span>> Please review:  [1]<a href="https://github.com/sugarlabs/sugar/pull/454" target="_blank">https://github.com/sugarlabs/sugar/pull/454</a><br>
<br></blockquote><div><br></div><div>By the way the commit has been pushed:  <a href="https://github.com/sugarlabs/sugar/commit/64b4b2fba1c37a9ad92ed30eb669b68552b62415">https://github.com/sugarlabs/sugar/commit/64b4b2fba1c37a9ad92ed30eb669b68552b62415</a><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Looks good, but haven't looked at it relative to the original.<br>
<br>
Comments:<br>
<br>
1.  is SUGAR_PROFILE a correct environment variable to use?  The<br>
exception posted by Martin was for SUGAR_MIME_DEFAULTS.<br></blockquote><div><br></div><div>I wanted to test if sugar was running.  All of those would be set if sugar is running, but I think that SUGAR_PROFILE better conveys my intent and readability.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
2.  patch description doesn't say anything about checking for an<br>
environment variable, <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
3.  comment "signals are sent to the activity list" doesn't say why<br>
these signals are useful, <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
4.  a more general design that uses exception handling rather than<br>
pre-checking, is to change the check for SUGAR_PROFILE to a try except<br>
clause:<br>
<br>
bundle = ActivityBundle(sys.argv[1])<br>
try:<br>
    from jarabe.model.bundleregistry import get_registry<br>
    registry = get_registry()<br>
except KeyError: # sugar might not be running<br>
    registry = None<br>
<br>
if registry:<br>
    registry.install(bundle, force_downgrade=True)<br>
else:<br>
    bundle.install()<br>
<br>
5.  an overall design question; why are there two methods for<br>
installing a bundle?  Can the bundle install method be changed to<br>
accept a completion callback instead?  Can the registry class be<br>
changed to accept a "bundle was installed" method instead?<br>
<div class=""><div class="h5"><br></div></div></blockquote><div><br></div><div>ActivityBundle.install really should signal the registry.  Maybe the reason it does not is that we need a way to install bundles when sugar is not running.  From memory, all the in sugar bundle installing things (journal...), use the registry.<br><br></div><div>Thanks,<br></div><div>Sam<br></div></div><br></div></div></div>