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

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.

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.

Michael Vogt (mvo) wrote :

All looks fine, thanks for this fix.

review: Approve
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
1=== modified file 'softwarecenter/ui/gtk3/widgets/'
2--- softwarecenter/ui/gtk3/widgets/ 2012-04-30 22:27:29 +0000
3+++ softwarecenter/ui/gtk3/widgets/ 2012-07-18 01:03:24 +0000
4@@ -126,6 +126,9 @@
5 @property
6 def appmodel(self):
7 model = self.get_model()
8+ # FIXME: would be nice if that would be less ugly,
9+ # because we use a treefilter we need to get the "real"
10+ # model first
11 if isinstance(model, Gtk.TreeModelFilter):
12 return model.get_model()
13 return model
14@@ -505,7 +508,7 @@
15 model, it = sel.get_selected()
16 path = model.get_path(it)
17 if path:
18- self._init_activated(btn, self.get_model(), path)
19+ self._init_activated(btn, model, path)
20 btn.has_focus = False
21 r = True
22 break
23@@ -521,7 +524,6 @@
24 btn,
26 app,
27- model,
28 path)
30 def _cell_data_func_cb(self, col, cell, model, it, user_data):
31@@ -555,24 +557,22 @@
32 text = ""
33 cell.set_property('text', text)
35- def _app_activated_cb(self, btn, btn_id, app, store, path):
36+ def _app_activated_cb(self, btn, btn_id, app, path):
37 if self.rowref_is_category(app):
38 return
40- # FIXME: would be nice if that would be more elegant
41- # because we use a treefilter we need to get the "real"
42- # model first
43- if type(store) is Gtk.TreeModelFilter:
44- store = store.get_model()
46- pkgname = self.appmodel.get_pkgname(app)
47+ model = self.appmodel
48+ # don't continue if we don't have a valid model (LP: #969907)
49+ if not model:
50+ return
52+ pkgname = model.get_pkgname(app)
54 if btn_id == CellButtonIDs.INFO:
55 self.app_view.emit("application-activated",
56- self.appmodel.get_application(app))
57+ model.get_application(app))
58 elif btn_id == CellButtonIDs.ACTION:
59 btn.set_sensitive(False)
60- store.row_changed(path, store.get_iter(path))
61+ model.row_changed(path, model.get_iter(path))
62 app_manager = get_appmanager()
63 # be sure we dont request an action for a pkg with
64 # pre-existing actions
65@@ -581,19 +581,19 @@
66 " '%s'" % pkgname)
67 return False
68 self._action_block_list.append(pkgname)
69- if self.appmodel.is_installed(app):
70+ if model.is_installed(app):
71 action = AppActions.REMOVE
72- elif self.appmodel.is_purchasable(app):
73- app_manager.buy_app(self.appmodel.get_application(app))
74- store.notify_action_request(app, path)
75+ elif model.is_purchasable(app):
76+ app_manager.buy_app(model.get_application(app))
77+ model.notify_action_request(app, path)
78 return
79 else:
80 action = AppActions.INSTALL
82- store.notify_action_request(app, path)
83+ model.notify_action_request(app, path)
85 app_manager.request_action(
86- self.appmodel.get_application(app), [], [],
87+ model.get_application(app), [], [],
88 action)
89 return False


People subscribed via source and target branches