#4906 BLOC Update.: Downloading a higher version of an activity doesn't upgrade the one in the XO

[Zarro Boogs per Child]

#4906: Downloading a higher version of an activity doesn't upgrade the one in the
XO
--------------------------+-------------------------------------------------
  Reporter:  gnu          |       Owner:  rwh      
      Type:  enhancement  |      Status:  new      
  Priority:  blocker      |   Milestone:  Update.1 
 Component:  sugar        |     Version:  Build 623
Resolution:               |    Keywords:  review-  
  Verified:  0            |  
--------------------------+-------------------------------------------------
Changes (by tomeu):

  * keywords:  review? => review-


Comment:

 {{{
       def remove_bundle(self, bundle_path):
 +         self._service_name_to_activity_info.clear()
 +         self._mime_type_to_activities.clear()
           return self._registry.RemoveBundle(bundle_path)
 }}}

 We are already invalidating the cache on ActivityRemoved and
 ActivityAdded. That's not enough?

 {{{
 +     def compare_version(self):
 +         """Compare this bundle with installed bundle.
 +         Returns -1 if older, 0 if same and 1 if newer"""
 }}}

 This API looks a bit too cumbersome to me. I would prefer if from the name
 of the method it was clear how it needs to be used. It also is currently
 only used by need_upgrade(), do you have any other use case in mind? If
 not, I would just merge compare_version() into need_upgrade().

 {{{
 +     def get_installed_path(self):
 +         act = activity.get_registry().get_activity(self._bundle_id)
 +         if act is None:
 +             return None
 +         else:
 +             return act.path
 }}}

 Why add this method to the public API? AFAICS, that functionality is
 already provided by the activity registry, so that method is just a
 convenience one. But I don't see how this method is called by other
 classes. I would move that method to be private if it makes the code
 clearer, or just merge into upgrade() if not.

 {{{
 -     def uninstall(self):
 +     def uninstall(self, install_path=None):
 }}}

 Why do we need to add that install_path arg to a public method? Any class
 outside from ActivityBundle will need to pass an install_path that
 ActivityBundle itself cannot determine? If possible, I would maintain the
 API and just add logic inside uninstall() to calculate the correct
 install_path to use.

-- 
Ticket URL: <http://dev.laptop.org/ticket/4906#comment:13>
One Laptop Per Child <http://dev.laptop.org>
OLPC bug tracking system