[Sugar-devel] Just wondering

[Sam P.]

Hi James,

On Mon, Dec 22, 2014 at 7:24 AM, James Cameron <quozl at laptop.org> wrote:

> On Sun, Dec 21, 2014 at 08:36:00AM +1100, Sam P. wrote:
> > I have submitted a new patch which work both inside sugar and in
> > other environments.
> >
> > Please review:  [1]https://github.com/sugarlabs/sugar/pull/454
>
>
By the way the commit has been pushed:
https://github.com/sugarlabs/sugar/commit/64b4b2fba1c37a9ad92ed30eb669b68552b62415


> Looks good, but haven't looked at it relative to the original.
>
> Comments:
>
> 1.  is SUGAR_PROFILE a correct environment variable to use?  The
> exception posted by Martin was for SUGAR_MIME_DEFAULTS.
>

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.


>
> 2.  patch description doesn't say anything about checking for an
> environment variable,
>

> 3.  comment "signals are sent to the activity list" doesn't say why
> these signals are useful,
>

> 4.  a more general design that uses exception handling rather than
> pre-checking, is to change the check for SUGAR_PROFILE to a try except
> clause:
>
> bundle = ActivityBundle(sys.argv[1])
> try:
>     from jarabe.model.bundleregistry import get_registry
>     registry = get_registry()
> except KeyError: # sugar might not be running
>     registry = None
>
> if registry:
>     registry.install(bundle, force_downgrade=True)
> else:
>     bundle.install()
>
> 5.  an overall design question; why are there two methods for
> installing a bundle?  Can the bundle install method be changed to
> accept a completion callback instead?  Can the registry class be
> changed to accept a "bundle was installed" method instead?
>
>
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.

Thanks,
Sam
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20141222/d43d89ea/attachment.html>