Merge lp:~gary-lasker/software-center/fix-lp969907-for-5.2 into lp:software-center/5.2

[Gary Lasker]

This branch should fix the crasher in bug 969907. Unfortunately, I could not find a way to reproduce this crash, but it has been consistently appearing on errors.ubuntu.com so we need to fix this. Basically, the fix consists in checking for a null model on the call to _app_activated_cb(), and returning if we don't have a model. There should be no unwanted side effect for this fix in the case of the error because we can't view the details for an application if we don't have a model available.

I also made a change earlier in the chain, in the _on_key_release_event() method (which eventually calls self._init_activated and finally results in the callback being called). In this there was a direct call to this.get_model(), which is not valid as it does not check for Gtk.TreeModelFilter as is done in the appmodel property. Instead, I just use the model passed in with the event.

I also did a small amount of cleanup to the code in _app_activated_cb(). There is a lot more cleanup that could and should be done in general in this class, but this branch is targeted for an SRU and so makes minimal changes.

Testing should be done in the various listviews, that is, to use both the "More Info" and the "Install/Remove" buttons in the listviews for both the "All Software" and "Installed" views (the latter tests the treeview case).

Thanks for your review!

[Michael Vogt (mvo)]

Thanks a lot for this branch! I have a bit of a busy morning so I will review it a bit later today, this is just a quick note to say that its great that you looked at the problem and analyzed it.

[Michael Vogt (mvo)]

Hi, I think this looks good and the check for a valid model is a good idea.

The only thing that makes me slightly nervous is that the "appmodel" property now returns the underlying model and never a filter model. This property is used in the cellrender and the appview (and internally in the apptreeview) so I wonder if that does not change the semantic? I will have a closer look to better understand this.

[Michael Vogt (mvo)]

Oh, sorry, its just the comment for def appmodel() that changed, so no semantic change. Please ignore the previous bit abotu that.

[Michael Vogt (mvo)]

All looks fine, thanks for this fix.

review: Approve

[Gary Lasker (gary-lasker)]

Ha! I was reading the comments in email and saw the one from two comments back and I had a tiny moment of panic "Yikes, I don't think I changed def appmodel???". I definitely would *not* touch that (until we find a way to properly not need such a thing). ;)

Yeah, I just added the FIXME there.

Thanks as always for the careful review!

=== modified file 'softwarecenter/ui/gtk3/widgets/apptreeview.py'
--- softwarecenter/ui/gtk3/widgets/apptreeview.py	2012-04-30 22:27:29 +0000
+++ softwarecenter/ui/gtk3/widgets/apptreeview.py	2012-07-18 01:03:24 +0000
@@ -126,6 +126,9 @@
     @property
     def appmodel(self):
         model = self.get_model()
+        # FIXME: would be nice if that would be less ugly,
+        # because we use a treefilter we need to get the "real"
+        # model first
         if isinstance(model, Gtk.TreeModelFilter):
             return model.get_model()
         return model
@@ -505,7 +508,7 @@
                     model, it = sel.get_selected()
                     path = model.get_path(it)
                     if path:
-                        self._init_activated(btn, self.get_model(), path)
+                        self._init_activated(btn, model, path)
                         btn.has_focus = False
                         r = True
                     break
@@ -521,7 +524,6 @@
                             btn,
                             btn.name,
                             app,
-                            model,
                             path)
 
     def _cell_data_func_cb(self, col, cell, model, it, user_data):
@@ -555,24 +557,22 @@
             text = ""
         cell.set_property('text', text)
 
-    def _app_activated_cb(self, btn, btn_id, app, store, path):
+    def _app_activated_cb(self, btn, btn_id, app, path):
         if self.rowref_is_category(app):
             return
-
-        # FIXME: would be nice if that would be more elegant
-        # because we use a treefilter we need to get the "real"
-        # model first
-        if type(store) is Gtk.TreeModelFilter:
-            store = store.get_model()
-
-        pkgname = self.appmodel.get_pkgname(app)
+        model = self.appmodel
+        # don't continue if we don't have a valid model (LP: #969907)
+        if not model:
+            return
+
+        pkgname = model.get_pkgname(app)
 
         if btn_id == CellButtonIDs.INFO:
             self.app_view.emit("application-activated",
-                               self.appmodel.get_application(app))
+                               model.get_application(app))
         elif btn_id == CellButtonIDs.ACTION:
             btn.set_sensitive(False)
-            store.row_changed(path, store.get_iter(path))
+            model.row_changed(path, model.get_iter(path))
             app_manager = get_appmanager()
             # be sure we dont request an action for a pkg with
             # pre-existing actions
@@ -581,19 +581,19 @@
                               " '%s'" % pkgname)
                 return False
             self._action_block_list.append(pkgname)
-            if self.appmodel.is_installed(app):
+            if model.is_installed(app):
                 action = AppActions.REMOVE
-            elif self.appmodel.is_purchasable(app):
-                app_manager.buy_app(self.appmodel.get_application(app))
-                store.notify_action_request(app, path)
+            elif model.is_purchasable(app):
+                app_manager.buy_app(model.get_application(app))
+                model.notify_action_request(app, path)
                 return
             else:
                 action = AppActions.INSTALL
 
-            store.notify_action_request(app, path)
+            model.notify_action_request(app, path)
 
             app_manager.request_action(
-                self.appmodel.get_application(app), [], [],
+                model.get_application(app), [], [],
                 action)
         return False