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

Proposed by Gary Lasker
Status: Merged
Approved by: Michael Vogt
Approved revision: 3065
Merge reported by: Michael Vogt
Merged at revision: not available
Proposed branch: lp:~gary-lasker/software-center/fix-lp969907-for-5.2
Merge into: lp:software-center/5.2
Diff against target: 89 lines (+19/-19)
1 file modified
softwarecenter/ui/gtk3/widgets/ (+19/-19)
To merge this branch: bzr merge lp:~gary-lasker/software-center/fix-lp969907-for-5.2
Reviewer Review Type Date Requested Status
Michael Vogt Approve
Review via email:

Description of the change

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 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!

To post a comment you must log in.
Revision history for this message
Michael Vogt (mvo) wrote :

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.

Revision history for this message
Michael Vogt (mvo) wrote :

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.

Revision history for this message
Michael Vogt (mvo) wrote :

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

Revision history for this message
Michael Vogt (mvo) wrote :

All looks fine, thanks for this fix.

review: Approve
Revision history for this message
Gary Lasker (gary-lasker) wrote :

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!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'softwarecenter/ui/gtk3/widgets/'
--- softwarecenter/ui/gtk3/widgets/ 2012-04-30 22:27:29 +0000
+++ softwarecenter/ui/gtk3/widgets/ 2012-07-18 01:03:24 +0000
@@ -126,6 +126,9 @@
126 @property126 @property
127 def appmodel(self):127 def appmodel(self):
128 model = self.get_model()128 model = self.get_model()
129 # FIXME: would be nice if that would be less ugly,
130 # because we use a treefilter we need to get the "real"
131 # model first
129 if isinstance(model, Gtk.TreeModelFilter):132 if isinstance(model, Gtk.TreeModelFilter):
130 return model.get_model()133 return model.get_model()
131 return model134 return model
@@ -505,7 +508,7 @@
505 model, it = sel.get_selected()508 model, it = sel.get_selected()
506 path = model.get_path(it)509 path = model.get_path(it)
507 if path:510 if path:
508 self._init_activated(btn, self.get_model(), path)511 self._init_activated(btn, model, path)
509 btn.has_focus = False512 btn.has_focus = False
510 r = True513 r = True
511 break514 break
@@ -521,7 +524,6 @@
521 btn,524 btn,
523 app,526 app,
524 model,
525 path)527 path)
527 def _cell_data_func_cb(self, col, cell, model, it, user_data):529 def _cell_data_func_cb(self, col, cell, model, it, user_data):
@@ -555,24 +557,22 @@
555 text = ""557 text = ""
556 cell.set_property('text', text)558 cell.set_property('text', text)
558 def _app_activated_cb(self, btn, btn_id, app, store, path):560 def _app_activated_cb(self, btn, btn_id, app, path):
559 if self.rowref_is_category(app):561 if self.rowref_is_category(app):
560 return562 return
561563 model = self.appmodel
562 # FIXME: would be nice if that would be more elegant564 # don't continue if we don't have a valid model (LP: #969907)
563 # because we use a treefilter we need to get the "real"565 if not model:
564 # model first566 return
565 if type(store) is Gtk.TreeModelFilter:567
566 store = store.get_model()568 pkgname = model.get_pkgname(app)
568 pkgname = self.appmodel.get_pkgname(app)
570 if btn_id == CellButtonIDs.INFO:570 if btn_id == CellButtonIDs.INFO:
571 self.app_view.emit("application-activated",571 self.app_view.emit("application-activated",
572 self.appmodel.get_application(app))572 model.get_application(app))
573 elif btn_id == CellButtonIDs.ACTION:573 elif btn_id == CellButtonIDs.ACTION:
574 btn.set_sensitive(False)574 btn.set_sensitive(False)
575 store.row_changed(path, store.get_iter(path))575 model.row_changed(path, model.get_iter(path))
576 app_manager = get_appmanager()576 app_manager = get_appmanager()
577 # be sure we dont request an action for a pkg with577 # be sure we dont request an action for a pkg with
578 # pre-existing actions578 # pre-existing actions
@@ -581,19 +581,19 @@
581 " '%s'" % pkgname)581 " '%s'" % pkgname)
582 return False582 return False
583 self._action_block_list.append(pkgname)583 self._action_block_list.append(pkgname)
584 if self.appmodel.is_installed(app):584 if model.is_installed(app):
585 action = AppActions.REMOVE585 action = AppActions.REMOVE
586 elif self.appmodel.is_purchasable(app):586 elif model.is_purchasable(app):
587 app_manager.buy_app(self.appmodel.get_application(app))587 app_manager.buy_app(model.get_application(app))
588 store.notify_action_request(app, path)588 model.notify_action_request(app, path)
589 return589 return
590 else:590 else:
591 action = AppActions.INSTALL591 action = AppActions.INSTALL
593 store.notify_action_request(app, path)593 model.notify_action_request(app, path)
595 app_manager.request_action(595 app_manager.request_action(
596 self.appmodel.get_application(app), [], [],596 model.get_application(app), [], [],
597 action)597 action)
598 return False598 return False


People subscribed via source and target branches