Merge lp:~gary-lasker/software-center/handle-trans-cancel-lp1027209 into lp:software-center

Proposed by Gary Lasker
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
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!

To post a comment you must log in.
Michael Vogt (mvo) 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-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?

[..]
> === modified file 'tests/gtk3/test_unity_launcher_integration.py'
> --- tests/gtk3/test_unity_launcher_integration.py 2012-08-30 07:45:31 +0000
> +++ tests/gtk3/test_unity_launcher_integration.py 2012-09-14 22:52:21 +0000
> @@ -60,6 +60,20 @@
> self.available_pane.backend.emit("transaction-finished",
> mock_result)
> do_events_with_sleep()
> +
> + def _simulate_install_cancellation(self, app):
> + # pretend we started an install
> + self.available_pane.backend.emit("transaction-started",
> + app.pkgname, app.appname,
> + "testid101",
> + TransactionTypes.INSTALL)
> + do_events_with_sleep()

      self.assertEqual(
            self.available_pane.unity_launcher_transaction_queue.keys(),
            ["software-center"])

> + # send the signal to cancel the install
> + mock_result = Mock()
> + mock_result.pkgname = app.pkgname
> + self.available_pane.backend.emit("transaction-cancelled",
> + mock_result)
> + do_events_with_sleep()

        self.assertEqual(
            self.available_pane.unity_launcher_transaction_queue, {})

[..]

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

Michael Vogt (mvo) :
review: Needs Information
3179. By Gary Lasker on 2012-09-17

merge trunk

3180. By Gary Lasker on 2012-09-17

consolidate code to a configurable single simulate_install_events utility method

3181. By Gary Lasker on 2012-09-17

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 :
Download full text (3.2 KiB)

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

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/gtk3/test_unity_launcher_integration.py'
> > --- tests/gtk3/test_unity_launcher_integration.py 2012-08-30 07:45:31
> +0000
> > +++ tests/gtk3/test_unity_launcher_integration.py 2012-09-14 22:52:21
> +0000
> > @@ -60,6 +60,20 @@
> > self.available_pane.backend.emit("transaction-finished",
> > mock_result)
> > do_events_with_sleep()
> > +
> > + def _simulate_install_cancellation(self, app):
> > + # pretend we started an install
> > + self.available_pane.backend.emit("transaction-started",
> > + app.pkgname, app.appname,
> > + "testid101",
> > + TransactionTypes.INSTALL)
> > + do_events_with_sleep()
>
> self.assertEqual(
> self.available_pane.unity_launcher_transaction_queue.keys(),
> ["software-center"])
>
> > + # send the signal to cancel the install
> > + mock_result = Mock()
> > + mock_result.pkgname = app.pkgname
> > + self.available_pane.backend.emit("transaction-cancelled",
> > + mock_result)
> > + do_events_with_sleep()
>
> self.assertEqual(
> self.available_pane.unity_launcher_transaction_queue, {})
>
>
> [..]
>
> 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_install_cancellation). But I get the gist which is to check that the queue is maintained correcty. This may not be strictly necessary, however, but I'll think about it a bit more and will add this test enhancement in a separate follow-on branch in the interest of getting the current branch merged (we are close to the wire for b2 freeze...

Read more...

3182. By Gary Lasker on 2012-09-17

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 on 2012-09-17

add a comment to better clarify the error test

3184. By Gary Lasker on 2012-09-17

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-stopped" signal is emitted, needed a fix to clear the queue so I've added that as well.

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 on 2012-09-18

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-stopped" signal is emitted, needed a fix to clear the queue so I've added that as well.
>
> 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_{pkgname,launcher_info}". Given
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_{pkgname,launcher_info}" during the merge to unblock this as we want a upload soon to go in before b2 freeze.

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

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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 'transaction-stopped': (GObject.SIGNAL_RUN_FIRST,
6 GObject.TYPE_NONE,
7 (GObject.TYPE_PYOBJECT,)),
8+ # emits a TransactionFinished object
9+ 'transaction-cancelled': (GObject.SIGNAL_RUN_FIRST,
10+ GObject.TYPE_NONE,
11+ (GObject.TYPE_PYOBJECT,)),
12 # emits with a pending_transactions list object
13 'transactions-changed': (GObject.SIGNAL_RUN_FIRST,
14 GObject.TYPE_NONE,
15@@ -848,6 +852,14 @@
16 self._logger.debug("_on_transaction_finished: %s %s %s" % (
17 trans, enum, trans.meta_data))
18
19+ # first check if there has been a cancellation of
20+ # the install and fire a transaction-cancelled signal
21+ # (see LP: #1027209)
22+ if enum == enums.EXIT_CANCELLED:
23+ result = TransactionFinishedResult(trans, False)
24+ self.emit("transaction-cancelled", result)
25+ return
26+
27 # show error
28 if enum == enums.EXIT_FAILED:
29 # Handle invalid packages separately
30
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 self.on_transactions_changed)
36 self.backend.connect("transaction-finished",
37 self.on_transaction_complete)
38+ self.backend.connect("transaction-cancelled",
39+ self.on_transaction_cancelled)
40+ # a transaction error is treated the same as a cancellation
41 self.backend.connect("transaction-stopped",
42- self.on_transaction_complete)
43+ self.on_transaction_cancelled)
44
45 # now we are initialized
46 self.searchentry.set_sensitive(True)
47@@ -392,11 +395,19 @@
48 self._update_action_bar()
49
50 def on_transaction_complete(self, backend, result):
51+ """ handle a transaction that has completed successfully
52+ """
53 if result.pkgname in self.unity_launcher_transaction_queue:
54 transaction_details = (
55 self.unity_launcher_transaction_queue.pop(result.pkgname))
56 self._add_application_to_unity_launcher(transaction_details)
57
58+ def on_transaction_cancelled(self, backend, result):
59+ """ handle a transaction that has been cancelled
60+ """
61+ if result.pkgname in self.unity_launcher_transaction_queue:
62+ self.unity_launcher_transaction_queue.pop(result.pkgname)
63+
64 def _register_unity_launcher_transaction_started(self, pkgname, appname,
65 trans_id, trans_type):
66 # at the start of the transaction, we gather details for use later
67
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 def tearDownClass(cls):
73 cls.win.destroy()
74
75- def _simulate_install_events(self, app):
76+ def _simulate_install_events(self, app,
77+ result_event="transaction-finished"):
78 # pretend we started an install
79 self.available_pane.backend.emit("transaction-started",
80 app.pkgname, app.appname,
81@@ -57,7 +58,7 @@
82 # send the signal to complete the install
83 mock_result = Mock()
84 mock_result.pkgname = app.pkgname
85- self.available_pane.backend.emit("transaction-finished",
86+ self.available_pane.backend.emit(result_event,
87 mock_result)
88 do_events_with_sleep()
89
90@@ -122,6 +123,9 @@
91 self.assertTrue(mock_send_application_to_launcher.called)
92 args, kwargs = mock_send_application_to_launcher.call_args
93 self._check_send_application_to_launcher_args(*args, **kwargs)
94+ # insure that the unity launcher transaction queue is cleared
95+ self.assertEqual(
96+ self.available_pane.unity_launcher_transaction_queue, {})
97
98 @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher'
99 '.send_application_to_launcher')
100@@ -141,6 +145,9 @@
101 self.assertTrue(mock_send_application_to_launcher.called)
102 args, kwargs = mock_send_application_to_launcher.call_args
103 self._check_send_application_to_launcher_args(*args, **kwargs)
104+ # insure that the unity launcher transaction queue is cleared
105+ self.assertEqual(
106+ self.available_pane.unity_launcher_transaction_queue, {})
107
108 @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher'
109 '.send_application_to_launcher')
110@@ -151,6 +158,9 @@
111 test_pkgname = "software-center"
112 self._navigate_to_appdetails_and_install(test_pkgname)
113 self.assertFalse(mock_send_application_to_launcher.called)
114+ # insure that the unity launcher transaction queue is cleared
115+ self.assertEqual(
116+ self.available_pane.unity_launcher_transaction_queue, {})
117
118 @patch('softwarecenter.ui.gtk3.panes.availablepane'
119 '.convert_desktop_file_to_installed_location')
120@@ -188,6 +198,64 @@
121 # the previous call(s)
122 mock_send_application_to_launcher.reset_mock()
123
124+ @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher'
125+ '.send_application_to_launcher')
126+ def test_unity_launcher_integration_cancelled_install(self,
127+ mock_send_application_to_launcher):
128+ # test the automatic add to launcher enabled functionality when
129+ # installing an app from the details view, and then cancelling
130+ # the install (see LP: #1027209)
131+ self.config.add_to_unity_launcher = True
132+ test_pkgname = "software-center"
133+ self.expected_pkgname = test_pkgname
134+ self.expected_launcher_info = UnityLauncherInfo("software-center",
135+ "softwarecenter",
136+ 0, 0, 0, 0, # these values are set in availablepane
137+ "/usr/share/applications/ubuntu-software-center.desktop",
138+ "")
139+ app = Application("", test_pkgname)
140+ self.available_pane.app_view.emit("application-activated",
141+ app)
142+ do_events()
143+ self._simulate_install_events(app,
144+ result_event="transaction-cancelled")
145+ # ensure that we do not send the application to the launcher
146+ # on a cancelled installation
147+ self.assertFalse(mock_send_application_to_launcher.called)
148+ # insure that the unity launcher transaction queue is cleared
149+ self.assertEqual(
150+ self.available_pane.unity_launcher_transaction_queue, {})
151+
152+ @patch('softwarecenter.ui.gtk3.panes.availablepane.UnityLauncher'
153+ '.send_application_to_launcher')
154+ def test_unity_launcher_integration_installation_failure(self,
155+ mock_send_application_to_launcher):
156+ # test the automatic add to launcher enabled functionality when
157+ # a failure is detected during the transaction (aptd emits a
158+ # "transaction-stopped" signal for this case)
159+ self.config.add_to_unity_launcher = True
160+ test_pkgname = "software-center"
161+ self.expected_pkgname = test_pkgname
162+ self.expected_launcher_info = UnityLauncherInfo("software-center",
163+ "softwarecenter",
164+ 0, 0, 0, 0, # these values are set in availablepane
165+ "/usr/share/applications/ubuntu-software-center.desktop",
166+ "")
167+ app = Application("", test_pkgname)
168+ self.available_pane.app_view.emit("application-activated",
169+ app)
170+ do_events()
171+ # aptd will emit a "transaction-stopped" signal if a transaction
172+ # error is encountered
173+ self._simulate_install_events(app,
174+ result_event="transaction-stopped")
175+ # ensure that we do not send the application to the launcher
176+ # on a failed installation
177+ self.assertFalse(mock_send_application_to_launcher.called)
178+ # insure that the unity launcher transaction queue is cleared
179+ self.assertEqual(
180+ self.available_pane.unity_launcher_transaction_queue, {})
181+
182
183 class DesktopFilepathConversionTestCase(unittest.TestCase):
184

Subscribers

People subscribed via source and target branches