Merge lp:~gary-lasker/software-center/handle-trans-cancel-lp1027209 into lp:software-center
- handle-trans-cancel-lp1027209
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 3186 |
Proposed branch: | lp:~gary-lasker/software-center/handle-trans-cancel-lp1027209 |
Merge into: | lp:software-center |
Diff against target: |
183 lines (+94/-3) 3 files modified
softwarecenter/backend/installbackend_impl/aptd.py (+12/-0) softwarecenter/ui/gtk3/panes/availablepane.py (+12/-1) tests/gtk3/test_unity_launcher_integration.py (+70/-2) |
To merge this branch: | bzr merge lp:~gary-lasker/software-center/handle-trans-cancel-lp1027209 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michael Vogt | Needs Information | ||
Review via email: mp+124536@code.launchpad.net |
Commit message
* lp:~gary-lasker/software-center/handle-trans-cancel-lp1027209:
- don't add an application to the Unity launcher for the case
where an installation is cancelled (LP: #1027209)
Description of the change
This branch contains a proposed fix for bug 1027209. To test, follow the steps outlined in comment #9 in the bug, as shown below:
Follow the below steps to see the issue:
#. Open software-center
#. Click on install on a big application like wesnoth
#. Wait until the "In Progress" appears in the toplevel toolbar of s-c
#. Click on the "In Progress" button in the toolbar
#. Click on the "cancel" button of the install of the application
Result (current trunk USC): At the moment of the cancellation, the icon for the application is incorrectly added to the Unity launcher.
Expected (this branch): After a cancellation, no icon is added to the Unity launcher.
I've also included a new unit test for this bug to insure we don't regress on it.
Many thanks for your review!
Michael Vogt (mvo) wrote : | # |
Michael Vogt (mvo) : | # |
- 3179. By Gary Lasker
-
merge trunk
- 3180. By Gary Lasker
-
consolidate code to a configurable single simulate_
install_ events utility method - 3181. By Gary Lasker
-
add a new unit test that ensures we do not send an application to the launcher on a transaction error (a transaction-stopped event from aptd)
Gary Lasker (gary-lasker) wrote : | # |
> On Fri, Sep 14, 2012 at 10:53:23PM -0000, Gary Lasker wrote:
> > Gary Lasker has proposed merging lp:~gary-lasker/software-center/handle-
> trans-cancel-
> [..]
> > Follow the below steps to see the issue:
> > #. Open software-center
> > #. Click on install on a big application like wesnoth
> > #. Wait until the "In Progress" appears in the toplevel toolbar of s-c
> > #. Click on the "In Progress" button in the toolbar
> > #. Click on the "cancel" button of the install of the application
> >
> > I've also included a new unit test for this bug to insure we don't regress
> on it.
>
> Thanks for the branch and for adding a unittest (and the adding of
> docstrings, always nice).
>
> It looks fine, the only question I have is what happens if the
> transaction errors, e.g. if the network gets lost during a download of
> a application. Will that work as expected, i.e. not add anything to
> the launcher or do we need to connect the transaction-stopped signal
> for that?
Thanks! I've added a unit test for this case and and shows that the launcher call does not occur. I took the opportunity to consolodate some (now duplicated) code.
>
> [..]
> > === modified file 'tests/
> > --- tests/gtk3/
> +0000
> > +++ tests/gtk3/
> +0000
> > @@ -60,6 +60,20 @@
> > self.available_
> > mock_result)
> > do_events_
> > +
> > + def _simulate_
> > + # pretend we started an install
> > + self.available_
> > + app.pkgname, app.appname,
> > + "testid101",
> > + TransactionType
> > + do_events_
>
> self.assertEqual(
> self.available_
> ["software-
>
> > + # send the signal to cancel the install
> > + mock_result = Mock()
> > + mock_result.pkgname = app.pkgname
> > + self.available_
> > + mock_result)
> > + do_events_
>
> self.assertEqual(
> self.available_
>
>
> [..]
>
> I like the test in general, the above lines may make it even
> better. To also check that the queue behaves in the right way
> (i.e. stuff is added and taken out again).
This diff is a little confusing to me as it appears to add code that is already in the test (in the method _simulate_
- 3182. By Gary Lasker
-
RED: check that the launcher item queue is empty after a transaction is completed for all types, includes a new failing unit test for when a transaction error is encountered
- 3183. By Gary Lasker
-
add a comment to better clarify the error test
- 3184. By Gary Lasker
-
GREEN: handle the transaction error case, unit test now working
Gary Lasker (gary-lasker) wrote : | # |
Ok, I added the check for the launcher queue to be properly emptied for the various types of transaction end cases. The error case, where a "transaction-
Note that clearing the queue isn't strictly required as an item left there will have no ill effect (it's only functional when the corresponding package completes a successful install, and in this case it will "just word". Plus, it's not likely that many items would end up queued in here. Neverthess, it's clean and much nicer to clear it out for all cases, so it does that now.
Should be ready for re-review now. Thanks!
- 3185. By Gary Lasker
-
merge latest trunk
Michael Vogt (mvo) wrote : | # |
On Mon, Sep 17, 2012 at 09:55:23PM -0000, Gary Lasker wrote:
> Ok, I added the check for the launcher queue to be properly emptied for the various types of transaction end cases. The error case, where a "transaction-
>
> Note that clearing the queue isn't strictly required as an item left there will have no ill effect (it's only functional when the corresponding package completes a successful install, and in this case it will "just word". Plus, it's not likely that many items would end up queued in here. Neverthess, it's clean and much nicer to clear it out for all cases, so it does that now.
Thanks for the updated test and code. I agree that while unlikely to
have ill effects its a good idea to clean the queue and the test will
ensure that this is the case. I like that :)
The diff is fine, the only nitpick I have is that in lines 132-138 and
161-168 the tests sets "self.expected_
that the test is about not sending the signal it seems like this is
not needed, is that correct?
Cheers,
Michael
Michael Vogt (mvo) wrote : | # |
I removed the bits around "self.expected_
Gary Lasker (gary-lasker) wrote : | # |
Hi Michael, indeed, those are needed in the tests above, but not in the two new test cases. Thank you for noticing that and removing them!
Preview Diff
1 | === modified file 'softwarecenter/backend/installbackend_impl/aptd.py' | |||
2 | --- softwarecenter/backend/installbackend_impl/aptd.py 2012-09-05 06:46:15 +0000 | |||
3 | +++ softwarecenter/backend/installbackend_impl/aptd.py 2012-09-18 04:57:18 +0000 | |||
4 | @@ -193,6 +193,10 @@ | |||
5 | 193 | 'transaction-stopped': (GObject.SIGNAL_RUN_FIRST, | 193 | 'transaction-stopped': (GObject.SIGNAL_RUN_FIRST, |
6 | 194 | GObject.TYPE_NONE, | 194 | GObject.TYPE_NONE, |
7 | 195 | (GObject.TYPE_PYOBJECT,)), | 195 | (GObject.TYPE_PYOBJECT,)), |
8 | 196 | # emits a TransactionFinished object | ||
9 | 197 | 'transaction-cancelled': (GObject.SIGNAL_RUN_FIRST, | ||
10 | 198 | GObject.TYPE_NONE, | ||
11 | 199 | (GObject.TYPE_PYOBJECT,)), | ||
12 | 196 | # emits with a pending_transactions list object | 200 | # emits with a pending_transactions list object |
13 | 197 | 'transactions-changed': (GObject.SIGNAL_RUN_FIRST, | 201 | 'transactions-changed': (GObject.SIGNAL_RUN_FIRST, |
14 | 198 | GObject.TYPE_NONE, | 202 | GObject.TYPE_NONE, |
15 | @@ -848,6 +852,14 @@ | |||
16 | 848 | self._logger.debug("_on_transaction_finished: %s %s %s" % ( | 852 | self._logger.debug("_on_transaction_finished: %s %s %s" % ( |
17 | 849 | trans, enum, trans.meta_data)) | 853 | trans, enum, trans.meta_data)) |
18 | 850 | 854 | ||
19 | 855 | # first check if there has been a cancellation of | ||
20 | 856 | # the install and fire a transaction-cancelled signal | ||
21 | 857 | # (see LP: #1027209) | ||
22 | 858 | if enum == enums.EXIT_CANCELLED: | ||
23 | 859 | result = TransactionFinishedResult(trans, False) | ||
24 | 860 | self.emit("transaction-cancelled", result) | ||
25 | 861 | return | ||
26 | 862 | |||
27 | 851 | # show error | 863 | # show error |
28 | 852 | if enum == enums.EXIT_FAILED: | 864 | if enum == enums.EXIT_FAILED: |
29 | 853 | # Handle invalid packages separately | 865 | # Handle invalid packages separately |
30 | 854 | 866 | ||
31 | === modified file 'softwarecenter/ui/gtk3/panes/availablepane.py' | |||
32 | --- softwarecenter/ui/gtk3/panes/availablepane.py 2012-09-17 15:15:32 +0000 | |||
33 | +++ softwarecenter/ui/gtk3/panes/availablepane.py 2012-09-18 04:57:18 +0000 | |||
34 | @@ -227,8 +227,11 @@ | |||
35 | 227 | self.on_transactions_changed) | 227 | self.on_transactions_changed) |
36 | 228 | self.backend.connect("transaction-finished", | 228 | self.backend.connect("transaction-finished", |
37 | 229 | self.on_transaction_complete) | 229 | self.on_transaction_complete) |
38 | 230 | self.backend.connect("transaction-cancelled", | ||
39 | 231 | self.on_transaction_cancelled) | ||
40 | 232 | # a transaction error is treated the same as a cancellation | ||
41 | 230 | self.backend.connect("transaction-stopped", | 233 | self.backend.connect("transaction-stopped", |
43 | 231 | self.on_transaction_complete) | 234 | self.on_transaction_cancelled) |
44 | 232 | 235 | ||
45 | 233 | # now we are initialized | 236 | # now we are initialized |
46 | 234 | self.searchentry.set_sensitive(True) | 237 | self.searchentry.set_sensitive(True) |
47 | @@ -392,11 +395,19 @@ | |||
48 | 392 | self._update_action_bar() | 395 | self._update_action_bar() |
49 | 393 | 396 | ||
50 | 394 | def on_transaction_complete(self, backend, result): | 397 | def on_transaction_complete(self, backend, result): |
51 | 398 | """ handle a transaction that has completed successfully | ||
52 | 399 | """ | ||
53 | 395 | if result.pkgname in self.unity_launcher_transaction_queue: | 400 | if result.pkgname in self.unity_launcher_transaction_queue: |
54 | 396 | transaction_details = ( | 401 | transaction_details = ( |
55 | 397 | self.unity_launcher_transaction_queue.pop(result.pkgname)) | 402 | self.unity_launcher_transaction_queue.pop(result.pkgname)) |
56 | 398 | self._add_application_to_unity_launcher(transaction_details) | 403 | self._add_application_to_unity_launcher(transaction_details) |
57 | 399 | 404 | ||
58 | 405 | def on_transaction_cancelled(self, backend, result): | ||
59 | 406 | """ handle a transaction that has been cancelled | ||
60 | 407 | """ | ||
61 | 408 | if result.pkgname in self.unity_launcher_transaction_queue: | ||
62 | 409 | self.unity_launcher_transaction_queue.pop(result.pkgname) | ||
63 | 410 | |||
64 | 400 | def _register_unity_launcher_transaction_started(self, pkgname, appname, | 411 | def _register_unity_launcher_transaction_started(self, pkgname, appname, |
65 | 401 | trans_id, trans_type): | 412 | trans_id, trans_type): |
66 | 402 | # at the start of the transaction, we gather details for use later | 413 | # at the start of the transaction, we gather details for use later |
67 | 403 | 414 | ||
68 | === modified file 'tests/gtk3/test_unity_launcher_integration.py' | |||
69 | --- tests/gtk3/test_unity_launcher_integration.py 2012-08-30 07:45:31 +0000 | |||
70 | +++ tests/gtk3/test_unity_launcher_integration.py 2012-09-18 04:57:18 +0000 | |||
71 | @@ -47,7 +47,8 @@ | |||
72 | 47 | def tearDownClass(cls): | 47 | def tearDownClass(cls): |
73 | 48 | cls.win.destroy() | 48 | cls.win.destroy() |
74 | 49 | 49 | ||
76 | 50 | def _simulate_install_events(self, app): | 50 | def _simulate_install_events(self, app, |
77 | 51 | result_event="transaction-finished"): | ||
78 | 51 | # pretend we started an install | 52 | # pretend we started an install |
79 | 52 | self.available_pane.backend.emit("transaction-started", | 53 | self.available_pane.backend.emit("transaction-started", |
80 | 53 | app.pkgname, app.appname, | 54 | app.pkgname, app.appname, |
81 | @@ -57,7 +58,7 @@ | |||
82 | 57 | # send the signal to complete the install | 58 | # send the signal to complete the install |
83 | 58 | mock_result = Mock() | 59 | mock_result = Mock() |
84 | 59 | mock_result.pkgname = app.pkgname | 60 | mock_result.pkgname = app.pkgname |
86 | 60 | self.available_pane.backend.emit("transaction-finished", | 61 | self.available_pane.backend.emit(result_event, |
87 | 61 | mock_result) | 62 | mock_result) |
88 | 62 | do_events_with_sleep() | 63 | do_events_with_sleep() |
89 | 63 | 64 | ||
90 | @@ -122,6 +123,9 @@ | |||
91 | 122 | self.assertTrue(mock_send_application_to_launcher.called) | 123 | self.assertTrue(mock_send_application_to_launcher.called) |
92 | 123 | args, kwargs = mock_send_application_to_launcher.call_args | 124 | args, kwargs = mock_send_application_to_launcher.call_args |
93 | 124 | self._check_send_application_to_launcher_args(*args, **kwargs) | 125 | self._check_send_application_to_launcher_args(*args, **kwargs) |
94 | 126 | # insure that the unity launcher transaction queue is cleared | ||
95 | 127 | self.assertEqual( | ||
96 | 128 | self.available_pane.unity_launcher_transaction_queue, {}) | ||
97 | 125 | 129 | ||
98 | 126 | @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher' | 130 | @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher' |
99 | 127 | '.send_application_to_launcher') | 131 | '.send_application_to_launcher') |
100 | @@ -141,6 +145,9 @@ | |||
101 | 141 | self.assertTrue(mock_send_application_to_launcher.called) | 145 | self.assertTrue(mock_send_application_to_launcher.called) |
102 | 142 | args, kwargs = mock_send_application_to_launcher.call_args | 146 | args, kwargs = mock_send_application_to_launcher.call_args |
103 | 143 | self._check_send_application_to_launcher_args(*args, **kwargs) | 147 | self._check_send_application_to_launcher_args(*args, **kwargs) |
104 | 148 | # insure that the unity launcher transaction queue is cleared | ||
105 | 149 | self.assertEqual( | ||
106 | 150 | self.available_pane.unity_launcher_transaction_queue, {}) | ||
107 | 144 | 151 | ||
108 | 145 | @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher' | 152 | @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher' |
109 | 146 | '.send_application_to_launcher') | 153 | '.send_application_to_launcher') |
110 | @@ -151,6 +158,9 @@ | |||
111 | 151 | test_pkgname = "software-center" | 158 | test_pkgname = "software-center" |
112 | 152 | self._navigate_to_appdetails_and_install(test_pkgname) | 159 | self._navigate_to_appdetails_and_install(test_pkgname) |
113 | 153 | self.assertFalse(mock_send_application_to_launcher.called) | 160 | self.assertFalse(mock_send_application_to_launcher.called) |
114 | 161 | # insure that the unity launcher transaction queue is cleared | ||
115 | 162 | self.assertEqual( | ||
116 | 163 | self.available_pane.unity_launcher_transaction_queue, {}) | ||
117 | 154 | 164 | ||
118 | 155 | @patch('softwarecenter.ui.gtk3.panes.availablepane' | 165 | @patch('softwarecenter.ui.gtk3.panes.availablepane' |
119 | 156 | '.convert_desktop_file_to_installed_location') | 166 | '.convert_desktop_file_to_installed_location') |
120 | @@ -188,6 +198,64 @@ | |||
121 | 188 | # the previous call(s) | 198 | # the previous call(s) |
122 | 189 | mock_send_application_to_launcher.reset_mock() | 199 | mock_send_application_to_launcher.reset_mock() |
123 | 190 | 200 | ||
124 | 201 | @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher' | ||
125 | 202 | '.send_application_to_launcher') | ||
126 | 203 | def test_unity_launcher_integration_cancelled_install(self, | ||
127 | 204 | mock_send_application_to_launcher): | ||
128 | 205 | # test the automatic add to launcher enabled functionality when | ||
129 | 206 | # installing an app from the details view, and then cancelling | ||
130 | 207 | # the install (see LP: #1027209) | ||
131 | 208 | self.config.add_to_unity_launcher = True | ||
132 | 209 | test_pkgname = "software-center" | ||
133 | 210 | self.expected_pkgname = test_pkgname | ||
134 | 211 | self.expected_launcher_info = UnityLauncherInfo("software-center", | ||
135 | 212 | "softwarecenter", | ||
136 | 213 | 0, 0, 0, 0, # these values are set in availablepane | ||
137 | 214 | "/usr/share/applications/ubuntu-software-center.desktop", | ||
138 | 215 | "") | ||
139 | 216 | app = Application("", test_pkgname) | ||
140 | 217 | self.available_pane.app_view.emit("application-activated", | ||
141 | 218 | app) | ||
142 | 219 | do_events() | ||
143 | 220 | self._simulate_install_events(app, | ||
144 | 221 | result_event="transaction-cancelled") | ||
145 | 222 | # ensure that we do not send the application to the launcher | ||
146 | 223 | # on a cancelled installation | ||
147 | 224 | self.assertFalse(mock_send_application_to_launcher.called) | ||
148 | 225 | # insure that the unity launcher transaction queue is cleared | ||
149 | 226 | self.assertEqual( | ||
150 | 227 | self.available_pane.unity_launcher_transaction_queue, {}) | ||
151 | 228 | |||
152 | 229 | @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher' | ||
153 | 230 | '.send_application_to_launcher') | ||
154 | 231 | def test_unity_launcher_integration_installation_failure(self, | ||
155 | 232 | mock_send_application_to_launcher): | ||
156 | 233 | # test the automatic add to launcher enabled functionality when | ||
157 | 234 | # a failure is detected during the transaction (aptd emits a | ||
158 | 235 | # "transaction-stopped" signal for this case) | ||
159 | 236 | self.config.add_to_unity_launcher = True | ||
160 | 237 | test_pkgname = "software-center" | ||
161 | 238 | self.expected_pkgname = test_pkgname | ||
162 | 239 | self.expected_launcher_info = UnityLauncherInfo("software-center", | ||
163 | 240 | "softwarecenter", | ||
164 | 241 | 0, 0, 0, 0, # these values are set in availablepane | ||
165 | 242 | "/usr/share/applications/ubuntu-software-center.desktop", | ||
166 | 243 | "") | ||
167 | 244 | app = Application("", test_pkgname) | ||
168 | 245 | self.available_pane.app_view.emit("application-activated", | ||
169 | 246 | app) | ||
170 | 247 | do_events() | ||
171 | 248 | # aptd will emit a "transaction-stopped" signal if a transaction | ||
172 | 249 | # error is encountered | ||
173 | 250 | self._simulate_install_events(app, | ||
174 | 251 | result_event="transaction-stopped") | ||
175 | 252 | # ensure that we do not send the application to the launcher | ||
176 | 253 | # on a failed installation | ||
177 | 254 | self.assertFalse(mock_send_application_to_launcher.called) | ||
178 | 255 | # insure that the unity launcher transaction queue is cleared | ||
179 | 256 | self.assertEqual( | ||
180 | 257 | self.available_pane.unity_launcher_transaction_queue, {}) | ||
181 | 258 | |||
182 | 191 | 259 | ||
183 | 192 | class DesktopFilepathConversionTestCase(unittest.TestCase): | 260 | class DesktopFilepathConversionTestCase(unittest.TestCase): |
184 | 193 | 261 |
On Fri, Sep 14, 2012 at 10:53:23PM -0000, Gary Lasker wrote:
> Gary Lasker has proposed merging lp:~gary-lasker/software-center/handle-trans-cancel-lp1027209 into lp:software-center.
[..]
> Follow the below steps to see the issue:
> #. Open software-center
> #. Click on install on a big application like wesnoth
> #. Wait until the "In Progress" appears in the toplevel toolbar of s-c
> #. Click on the "In Progress" button in the toolbar
> #. Click on the "cancel" button of the install of the application
>
> I've also included a new unit test for this bug to insure we don't regress on it.
Thanks for the branch and for adding a unittest (and the adding of
docstrings, always nice).
It looks fine, the only question I have is what happens if the
transaction errors, e.g. if the network gets lost during a download of
a application. Will that work as expected, i.e. not add anything to
the launcher or do we need to connect the transaction-stopped signal
for that?
[..] gtk3/test_ unity_launcher_ integration. py' test_unity_ launcher_ integration. py 2012-08-30 07:45:31 +0000 test_unity_ launcher_ integration. py 2012-09-14 22:52:21 +0000 pane.backend. emit("transacti on-finished" , with_sleep( ) install_ cancellation( self, app): pane.backend. emit("transacti on-started" , s.INSTALL) with_sleep( )
> === modified file 'tests/
> --- tests/gtk3/
> +++ tests/gtk3/
> @@ -60,6 +60,20 @@
> self.available_
> mock_result)
> do_events_
> +
> + def _simulate_
> + # pretend we started an install
> + self.available_
> + app.pkgname, app.appname,
> + "testid101",
> + TransactionType
> + do_events_
self. assertEqual(
self. available_ pane.unity_ launcher_ transaction_ queue.keys( ),
[" software- center" ])
> + # send the signal to cancel the install pane.backend. emit("transacti on-cancelled" , with_sleep( )
> + mock_result = Mock()
> + mock_result.pkgname = app.pkgname
> + self.available_
> + mock_result)
> + do_events_
[..]
I like the test in general, the above lines may make it even
better. To also check that the queue behaves in the right way
(i.e. stuff is added and taken out again).
Cheers,
Michael